Created attachment 63985 [details] [review] diff
Created attachment 63987 [details] [review] libzeitgeist2 branch diff Updated diff after mergin in new changes from master. And just for fun, a diffstat: b/.gitignore | 24 b/Makefile.am | 6 b/configure.ac | 42 + b/data/ontology2code | 6 b/doc/Makefile.am | 6 b/doc/libzeitgeist/Makefile.am | 45 + b/examples/Makefile.am | 41 + b/examples/build.txt | 12 b/examples/data-source-stuff.vala | 79 +++ b/examples/get-events-with-id.vala | 21 b/examples/monitor-events.vala | 33 + b/examples/most-recent-events.vala | 29 + b/extensions/Makefile.am | 6 b/extensions/benchmark.vala | 2 b/extensions/blacklist.vala | 8 b/extensions/ds-registry.vala | 165 +----- b/extensions/fts++/Makefile.am | 8 b/extensions/fts++/test/Makefile.am | 1 b/extensions/fts++/zeitgeist-fts.vala | 8 b/extensions/fts.vala | 22 b/extensions/histogram.vala | 2 b/extensions/storage-monitor.vala | 2 b/libzeitgeist/.gitignore | 2 b/libzeitgeist/API_CHANGES | 33 + b/libzeitgeist/Makefile.am | 76 ++ b/libzeitgeist/data-source-registry.vala | 144 +++++ b/libzeitgeist/data-source.vala | 140 +++++ b/libzeitgeist/datamodel.vala | 798 ++++++++++++++++++++++++++++++ b/libzeitgeist/index.vala | 215 ++++++++ b/libzeitgeist/log.vala | 256 +++++++++ b/libzeitgeist/mimetype.vala | 367 ++++++++++++++ b/libzeitgeist/monitor.vala | 156 ++++++ b/libzeitgeist/ontology-uris.vala.in | 22 b/libzeitgeist/ontology.vala.in | 172 ++++++ b/libzeitgeist/queued-proxy-wrapper.vala | 95 +++ b/libzeitgeist/remote.vala | 157 ++++++ b/libzeitgeist/result-set.vala | 132 +++++ b/libzeitgeist/simple-result-set.vala | 80 +++ b/libzeitgeist/timestamp.vala | 227 ++++++++ b/libzeitgeist/utils.vala | 189 +++++++ b/libzeitgeist/zeitgeist-2.0.pc.in | 11 b/src/Makefile.am | 22 b/src/errors.vala | 2 b/src/extension-collection.vala | 9 b/src/logging.vala | 4 b/src/sql.vala | 19 b/src/zeitgeist-daemon.vala | 53 +- b/test/direct/Makefile.am | 12 b/test/direct/marshalling-test.vala | 8 extensions/fts++/datamodel.vala | 1 extensions/fts++/ontology-uris.vala | 1 extensions/fts++/ontology.vala | 1 extensions/fts++/utils.vala | 1 src/datamodel.vala | 806 ------------------------------- src/mimetype.vala | 358 ------------- src/ontology-uris.vala.in | 22 src/ontology.vala.in | 172 ------ src/remote.vala | 152 ----- src/utils.vala | 203 ------- 59 files changed, 3729 insertions(+), 1957 deletions(-)
Created attachment 64072 [details] [review] Updated patch Finished implementing reconnection
Comment on attachment 64072 [details] [review] Updated patch Review of attachment 64072 [details] [review]: ----------------------------------------------------------------- ::: doc/libzeitgeist/Makefile.am @@ +11,5 @@ > + --pkg sqlite3 \ > + --basedir $(top_srcdir)/libzeitgeist \ > + --package-name libzeitgeist2 \ > + --package-version $(PACKAGE_VERSION) \ > + $(wildcard $(top_srcdir)/libzeitgeist/*.vala) \ Can we split the files into a separate variable? ::: examples/Makefile.am @@ +24,5 @@ > + monitor-events \ > + most-recent-events \ > + $(NULL) > + > +data_source_stuff_SOURCES = data-source-stuff.vala Ultimately we should get rid of generated .c files from the tarball, which means that this will stop working. ::: libzeitgeist/datamodel.vala @@ +23,5 @@ > + */ > + > +namespace Zeitgeist > +{ > + // FIXME: move errors.vala to libzeitgeist/ and put this in there... Seems like it's already there ;) @@ +24,5 @@ > + > +namespace Zeitgeist > +{ > + // FIXME: move errors.vala to libzeitgeist/ and put this in there... > + //[DBus (name = "org.gnome.zeitgeist.DataModelError")] Why is this commented out? This should be thrown over DBus as well, no? @@ +45,5 @@ > + public int64 end { get; private set; } > + > + public TimeRange (int64 start_msec, int64 end_msec) > + { > + start = start_msec; Why doesn't this use standard gobject-like constructor? "Object (start: start_msec, end: end_msec)" ::: libzeitgeist/log.vala @@ +218,5 @@ > + monitor.get_time_range ().to_variant (), > + Events.to_variant (monitor.get_templates ())); > + } > + > + public async void remove_monitor (Monitor monitor) throws Error Looks like something's missing here :) ::: libzeitgeist/monitor.vala @@ +47,5 @@ > + private TimeRange time_range; > + private GenericArray<Event> templates; > + > + // Client side D-Bus path the monitor lives under > + private string monitor_path; Let's use ObjectPath here. ::: libzeitgeist/queued-proxy-wrapper.vala @@ +24,5 @@ > +namespace Zeitgeist > +{ > + > +// FIXME: private or something > +public abstract class QueuedProxyWrapper : Object s/public/internal/ @@ +31,5 @@ > + public bool is_connected { get; private set; default = false; } > + > + protected SList<QueuedMethod> method_dispatch_queue; > + protected IOError? log_error; > + protected DBusProxy dbus_proxy; Uhhh, this will end up being a field in the ProxyWrapper struct, that's not nice :( @@ +40,5 @@ > + public SourceFunc queued_method { public /*owned*/ get; private /*owned*/ set; } > + > + public QueuedMethod (SourceFunc callback) > + { > + queued_method = callback; Looks like delegate copying, we'll need to transfer the ownership here (same for the parameter itself). ::: libzeitgeist/remote.vala @@ +134,5 @@ > + out double[] relevancies, out uint matches, > + Cancellable? cancellable=null) throws Error; > + } > + > + /* FIXME: Remove this! Only here because of a bug in Vala (see ext-fts) */ Can we explain the issue here? I'm pretty sure someone will remove the comment from ext-fts because it's not linking here. @@ +143,5 @@ > + public abstract uint32 state () throws IOError; > + public signal void state_changed (uint32 state); > + } > + > + /* FIXME: Remove this! Only here because of a bug in Vala (see ext-fts) */ Same as above. ::: libzeitgeist/simple-result-set.vala @@ +30,5 @@ > + private GenericArray<Event> events; > + private uint num_estimated_matches; > + private uint cursor; > + > + internal SimpleResultSet (GenericArray<Event> events, uint matches=-1) uint set to -1? @@ +33,5 @@ > + > + internal SimpleResultSet (GenericArray<Event> events, uint matches=-1) > + { > + this.events = events; > + num_estimated_matches = (matches >= 0) ? matches : events.length; (matches >= 0) is always true with uint matches. ::: test/direct/Makefile.am @@ +39,2 @@ > $(NULL) > This doesn't look right, why isn't it liking with libzeitgeist?
Thanks! Updated... http://cgit.freedesktop.org/zeitgeist/zeitgeist/commit/?h=libzeitgeist2&id=bdc9db34f827aae42f1c74f74ce7ee2d8fba8f97 > Ultimately we should get rid of generated .c files > from the tarball, which means that this will stop working. Then ultimately you will update it to work with your "not generating .c file" changes :P. > Looks like something's missing here :) Yey. I guess we can finish that in trunk. >> +// FIXME: private or something >> +public abstract class QueuedProxyWrapper : Object > s/public/internal/ As discussed before, Vala doesn't support increasing visibility in subclasses, so we are stuck there. > Uhhh, this will end up being a field in the ProxyWrapper > struct, that's not nice :( Doesn't work. > This doesn't look right, why isn't it liking with libzeitgeist? Why would it? (Feel free to change it)
>> Uhhh, this will end up being a field in the ProxyWrapper >> struct, that's not nice :( > Doesn't work. Huh.. s/Doesn't work./Not sure what you mean/
(In reply to comment #4) > Thanks! Updated... > http://cgit.freedesktop.org/zeitgeist/zeitgeist/commit/?h=libzeitgeist2&id=bdc9db34f827aae42f1c74f74ce7ee2d8fba8f97 > > > Ultimately we should get rid of generated .c files > > from the tarball, which means that this will stop working. > Then ultimately you will update it to work with your "not generating .c file" > changes :P. > > > Looks like something's missing here :) > Yey. I guess we can finish that in trunk. > Ok. > >> +// FIXME: private or something > >> +public abstract class QueuedProxyWrapper : Object > > s/public/internal/ > As discussed before, Vala doesn't support increasing visibility in subclasses, > so we are stuck there. > Fine. > > Uhhh, this will end up being a field in the ProxyWrapper > > struct, that's not nice :( > Doesn't work. > Make it work! > > This doesn't look right, why isn't it liking with libzeitgeist? > Why would it? (Feel free to change it) I'm not going to block the merge on this...
I think we should review the C API that this brings, but I guess we can do that in trunk, we can consider the libzg2 API unstable for now.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.