[Zeitgeist-bugs] [Bug 51875] Merge request for libzeitgeist2

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=51875

--- Comment #3 from Michal Hruby <michal.mhr at gmail.com> 2012-07-30 13:52:58 UTC ---
Comment on attachment 64072
  --> https://bugs.freedesktop.org/attachment.cgi?id=64072
Updated patch

Review of attachment 64072:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=51875&attachment=64072)
-----------------------------------------------------------------

::: 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?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.