Bug 51875 - Merge request for libzeitgeist2
Summary: Merge request for libzeitgeist2
Status: RESOLVED FIXED
Alias: None
Product: Zeitgeist
Classification: Unclassified
Component: libzeitgeist (show other bugs)
Version: 0.9.x
Hardware: Other All
: medium normal
Assignee: zeitgeist-bugs@lists.freedesktop.org
QA Contact: zeitgeist-bugs@lists.freedesktop.org
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-08 13:08 UTC by Siegfried-Angel Gevatter Pujals
Modified: 2012-07-31 17:12 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
diff (273.82 KB, patch)
2012-07-08 13:08 UTC, Siegfried-Angel Gevatter Pujals
Details | Splinter Review
libzeitgeist2 branch diff (243.05 KB, patch)
2012-07-08 13:19 UTC, Siegfried-Angel Gevatter Pujals
Details | Splinter Review
Updated patch (242.29 KB, patch)
2012-07-10 14:49 UTC, Siegfried-Angel Gevatter Pujals
Details | Splinter Review

Description Siegfried-Angel Gevatter Pujals 2012-07-08 13:08:08 UTC
Created attachment 63985 [details] [review]
diff
Comment 1 Siegfried-Angel Gevatter Pujals 2012-07-08 13:19:50 UTC
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(-)
Comment 2 Siegfried-Angel Gevatter Pujals 2012-07-10 14:49:18 UTC
Created attachment 64072 [details] [review]
Updated patch

Finished implementing reconnection
Comment 3 Michal Hruby 2012-07-30 13:52:58 UTC
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?
Comment 4 Siegfried-Angel Gevatter Pujals 2012-07-31 10:22:38 UTC
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)
Comment 5 Siegfried-Angel Gevatter Pujals 2012-07-31 10:58:35 UTC
>> 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/
Comment 6 Michal Hruby 2012-07-31 11:05:18 UTC
(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...
Comment 7 Michal Hruby 2012-07-31 17:09:12 UTC
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.