Merge lp:~kalikiana/midori/folderzg into lp:midori

[Cris Dywan]

Don't insert folders into the log

Don't insert folders into the log

[André Auzi (aauzi)]

The remaining risk, if callers don't respect the contract:

 * midori_browser_update_history:
 * @item: a #KatzeItem
 * @type: "website", "bookmark" or "download"
 * @event: "access", "leave", "modify", "delete"

is that KatzeItems of other type are used. For the moment I only think about separators.

It may be more robust for future evolutions to code:

if (!KATZE_ITEM_IS_BOOKMARK (item))
    return;

review: Needs Fixing

[Cris Dywan (kalikiana)]

I don't quite agree with that:

I wanted to fix the assertion quickly, but I didn't want to block on researching how to get a path/to/folder, what format to use and delay the fix - ultimately it's possible and desirable to log folders in zeitgeist as well. I should've pointed that out to begin with.

If a separator is passed most likely that's a bug in any relevant calling code. Maybe g_return_if_fail (KATZE_ITEM_IS_SEPARATOR (item)) instead?

[André Auzi (aauzi)]

> I don't quite agree with that:
>
> I wanted to fix the assertion quickly, but I didn't want to block on
> researching how to get a path/to/folder, what format to use and delay the fix
> - ultimately it's possible and desirable to log folders in zeitgeist as well.
> I should've pointed that out to begin with.
>
> If a separator is passed most likely that's a bug in any relevant calling
> code. Maybe g_return_if_fail (KATZE_ITEM_IS_SEPARATOR (item)) instead?

sorry, I missed the ZEITGEIST_NFO_BOOKMARK_FOLDER existence.

g_return_if_fail (KATZE_ITEM_IS_SEPARATOR (item)) would be a plus.

I would then add a FIXME comment to trace the final intent.

[lp:~kalikiana/midori/folderzg]

6243. By Cris Dywan on 2013-07-07

Assert that separators don't get logged

[Cris Dywan (kalikiana)]

André, can you give this another look? This is a bug fix and I'd like to get it in before freeze.

[André Auzi (aauzi)]

After carefully reading the zeigeist documents, it appears to me that the "subject" uri we use for bookmarks is wrong as well.

We currently use the bookmarked uri while we should probably use an uri identifying the bookmark itself.

I could not find a standard way to identify bookmarks with uri, in our case it could be something like:
midori://Bookmarks/.../bookmark_name

and uri for folders could then become:
midori://Bookmarks/.../folder_name

But that's another story, the fix can go as it is.

review: Approve

[Cris Dywan (kalikiana)]

Filed bug 1200943 for a follow up.

=== modified file 'midori/midori-browser.c'
--- midori/midori-browser.c	2013-07-01 20:20:53 +0000
+++ midori/midori-browser.c	2013-07-07 10:11:26 +0000
@@ -470,6 +470,8 @@
                                const gchar* type,
                                const gchar* event)
 {
+    g_return_if_fail (!KATZE_ITEM_IS_SEPARATOR (item));
+
     #ifdef HAVE_ZEITGEIST
     const gchar* inter;
     if (strstr (event, "access"))
@@ -484,6 +486,11 @@
         inter = ZEITGEIST_ZG_DELETE_EVENT;
     else
         g_assert_not_reached ();
+
+    /* FIXME: Should insert folders into the log (ZEITGEIST_NFO_BOOKMARK_FOLDER) */
+    if (KATZE_ITEM_IS_FOLDER (item))
+        return;
+
     zeitgeist_log_insert_events_no_reply (zeitgeist_log_get_default (),
         zeitgeist_event_new_full (inter, ZEITGEIST_ZG_USER_ACTIVITY,
                                   "application://midori.desktop",