Merge lp:~aauzi/midori/fix-bookmarks-menu-update-regression into lp:midori

[André Auzi]

bookmarks-db singleton was not completely connected to the application, as a result changes done in the panel were not properly propagated to the menu.

Additionnally import was not done in the database cache parent item, as a result the update of the root item, the dookmarks-db singleton, was not done.

Fix that.

The regression was already present in 0.5.7.

bookmarks-db singleton was not completely connected to the application, as a result changes done in the panel were not properly propagated to the menu.

Additionnally import was not done in the database cache parent item, as a result the update of the root item, the dookmarks-db singleton, was not done.

[Cris Dywan (kalikiana)]

Please replace DEBUG_UPDATE with midori_debug("bookmarks") or a check for "bookmarks" being in MIDORI_DEBUG if the code cannot use the function (will need to move the API to a lower level some day), build-time debugging is not easily accessible to most users.

I'm a little bit surprised that midori_bookmarks_db_populate_folder is completely new here, surely there already was a way to enlist the items. Is there code that is too specialized?

review: Needs Fixing

[André Auzi (aauzi)]

> Please replace DEBUG_UPDATE with midori_debug("bookmarks") or a check for
> "bookmarks" being in MIDORI_DEBUG if the code cannot use the function (will
> need to move the API to a lower level some day), build-time debugging is not
> easily accessible to most users.

OK, I will change that

> I'm a little bit surprised that midori_bookmarks_db_populate_folder is
> completely new here, surely there already was a way to enlist the items.

As far as I can remember there was not.

Unless I missed something in the reverse engineering process, former midori_array_query_items_recursive was returning a freshly created array which wich was recursively populated, only when requested, with it's subfolders.

The recursion was the only point were the arrays were filled.

UI wigets were filled "on demand" without recursion and held the tree structure of the bookmarks database

The point was to avoid the creation of distinct arrays and I missed the fact that query_items_recursive did not fill the root array.

> Is there code that is too specialized?

Or maybe not specialized enough. I realize that midori_bookmarks_query_items_recursive maintains the lookup table of the cached item, it may as well insure that folder items are kept in sync.

In fact there are two use cases for bookmarks items.

1- The UI entries, where queries are done to fill UI widgets "on demand". They must be in cache to insure events propagation between widgets but the tree structure is in the widgets

2- The entries used for import and exports operations for which the tree structure is important.

So far midori_bookmarks_query_items_recursive serves the two purposes but we can imagine future distinction.

The fact is that UI items should be obsolescent when they're in a closed folder, this is not the case yet.

The import/export items may not be collected in the cache.

[Eric Le Lay (neric27)]

There is bug when exporting bookmarks twice: folder contents will get duplicated.
Duplication happens at "midori-bookmarks-db.c":959.
This renders the recursive use of midori_bookmarks_query_items_recursive unusable.

[André Auzi (aauzi)]

> There is bug when exporting bookmarks twice: folder contents will get
> duplicated.
> Duplication happens at "midori-bookmarks-db.c":959.
> This renders the recursive use of midori_bookmarks_query_items_recursive
> unusable.
Correct.
If I apply the same design pattern as I used for the folder import, the recursive import misses a
katze_array_clear (item)
before line 959

Thanks, Eric

[lp:~aauzi/midori/fix-bookmarks-menu-update-regression]

6574. By André Auzi on 2014-04-25

merge lp:midori

6575. By André Auzi on 2014-04-25

merge lp:midori

6576. By André Auzi on 2014-04-25

fix duplication of entries in midori_bookmarks_db_query_recursive

6577. By André Auzi on 2014-04-25

use midori_debug("bookmarks") instead of DEBUG_UPDATE macro

[Cris Dywan (kalikiana)]

Thanks for the improvements. As usual, my review is a bit on the theoretical side. Maybe we can get another pair of eyes on it?

review: Abstain

[gue5t gue5t (gue5t)]

This does seem to get the bookmarks menu updating properly, and the code looks reasonable overally, though I'm not intimately familiar with the files changed. And it's probably better to have work on bookmarks landing than leaving branches out to bitrot.

review: Approve

=== modified file 'katze/katze-array.c'
--- katze/katze-array.c	2014-03-02 20:05:17 +0000
+++ katze/katze-array.c	2014-04-25 18:25:46 +0000
@@ -33,6 +33,15 @@
     GList* items;
 };
 
+enum
+{
+    PROP_0,
+
+    PROP_TYPE,
+
+    N_PROPERTIES
+};
+
 enum {
     ADD_ITEM,
     REMOVE_ITEM,
@@ -53,8 +62,19 @@
 static void
 _katze_array_update (KatzeArray* array)
 {
+/* FIXME: remove this declaration when midory_debug is made accessible */
+    extern gboolean midori_debug (const gchar* token);
+
     g_object_set_data (G_OBJECT (array), "last-update",
                        GINT_TO_POINTER (time (NULL)));
+    if (midori_debug ("bookmarks") && KATZE_IS_ITEM (array))
+    {
+        const gchar *name = katze_item_get_name (KATZE_ITEM (array));
+        if (name && *name)
+        {
+            g_print ("_katze_array_update: %s\n", name);
+        }
+    }
 }
 
 static void
@@ -105,6 +125,27 @@
 }
 
 static void
+_katze_array_set_property (GObject *object,
+    guint property_id,
+    const GValue *value,
+    GParamSpec *pspec)
+{
+    KatzeArray *array = KATZE_ARRAY (object);
+
+    switch (property_id)
+    {
+        case PROP_TYPE:
+            array->priv->type = g_value_get_gtype (value);
+            break;
+
+        default:
+            /* We don't have any other property... */
+            G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
+            break;
+    }
+}
+
+static void
 katze_array_class_init (KatzeArrayClass* class)
 {
     GObjectClass* gobject_class;
@@ -187,6 +228,7 @@
 
     gobject_class = G_OBJECT_CLASS (class);
     gobject_class->finalize = katze_array_finalize;
+    gobject_class->set_property = _katze_array_set_property;
 
     class->add_item = _katze_array_add_item;
     class->remove_item = _katze_array_remove_item;
@@ -194,6 +236,16 @@
     class->clear = _katze_array_clear;
     class->update = _katze_array_update;
 
+
+    g_object_class_install_property (gobject_class,
+        PROP_TYPE,
+        g_param_spec_gtype (
+            "type",
+            "Type",
+            "The array item type",
+            G_TYPE_NONE,
+            G_PARAM_WRITABLE|G_PARAM_CONSTRUCT_ONLY));
+
     g_type_class_add_private (class, sizeof (KatzeArrayPrivate));
 }
 
@@ -238,8 +290,7 @@
 
     g_return_val_if_fail (g_type_is_a (type, G_TYPE_OBJECT), NULL);
 
-    array = g_object_new (KATZE_TYPE_ARRAY, NULL);
-    array->priv->type = type;
+    array = g_object_new (KATZE_TYPE_ARRAY, "type", type, NULL);
 
     return array;
 }

=== modified file 'katze/katze-arrayaction.c'
--- katze/katze-arrayaction.c	2013-12-26 17:41:17 +0000
+++ katze/katze-arrayaction.c	2014-04-25 18:25:46 +0000
@@ -815,12 +815,14 @@
                               KatzeArray*       array)
 {
     GSList* proxies;
+    KatzeArray *old_array = NULL;
 
     g_return_if_fail (KATZE_IS_ARRAY_ACTION (array_action));
     g_return_if_fail (!array || katze_array_is_a (array, KATZE_TYPE_ITEM));
 
     /* FIXME: Disconnect old array */
 
+    old_array = array_action->array;
     if (array)
         g_object_ref (array);
     katze_object_assign (array_action->array, array);
@@ -841,7 +843,15 @@
 
     do
     {
+        KatzeArray* item = g_object_get_data (G_OBJECT (proxies->data), "KatzeItem");
+
+        if (item && (item == old_array))
+            g_object_set_data (G_OBJECT (proxies->data), "KatzeItem", array);
+
         gtk_widget_set_sensitive (proxies->data, array != NULL);
     }
     while ((proxies = g_slist_next (proxies)));
+
+    if (array)
+        katze_array_update (KATZE_ARRAY (array));
 }

=== modified file 'midori/midori-array.c'
--- midori/midori-array.c	2013-08-05 19:52:52 +0000
+++ midori/midori-array.c	2014-04-25 18:25:46 +0000
@@ -1031,9 +1031,10 @@
           || g_str_equal (name, "last_visit") || g_str_equal (name, "visit_count")
           || g_str_equal (name, "pos_panel") || g_str_equal (name, "pos_bar"))
     {
-        gint value;
-        value = sqlite3_column_int64 (stmt, column);
-        katze_item_set_meta_integer (item, name, value);
+        /* use text to properly handle NULL values */
+        const unsigned char* text;
+        text = sqlite3_column_text (stmt, column);
+        katze_item_set_meta_string (item, name, (gchar*)text);
     }
     else if (g_str_equal (name, "desc"))
     {

=== modified file 'midori/midori-bookmarks-db.c'
--- midori/midori-bookmarks-db.c	2014-03-16 20:25:59 +0000
+++ midori/midori-bookmarks-db.c	2014-04-25 18:25:46 +0000
@@ -181,14 +181,27 @@
 midori_bookmarks_db_get_item_parent (MidoriBookmarksDb* bookmarks,
                                      gpointer    item)
 {
+    gint64 parentid = katze_item_get_meta_integer (KATZE_ITEM (item), "parentid");
+    KatzeItem *search = katze_item_new ();
     KatzeArray* parent;
-    gint64 parentid;
-
-    parentid = katze_item_get_meta_integer (KATZE_ITEM (item), "parentid");
-
-    if (parentid == 0)
+
+    if (!parentid)
+    {
+        parentid = katze_item_get_meta_integer (KATZE_ITEM (bookmarks), "id");
+        katze_item_set_meta_integer (KATZE_ITEM (item), "parentid", parentid);
+    }
+
+    katze_item_set_meta_integer(search, "id", parentid);
+
+    parent = KATZE_ARRAY (g_hash_table_lookup (bookmarks->all_items, search));
+
+    g_object_unref (search);
+
+    if (!parent)
     {
         parent = KATZE_ARRAY (bookmarks);
+        katze_item_set_meta_integer (KATZE_ITEM (item), "parentid",
+            katze_item_get_meta_integer (KATZE_ITEM (bookmarks), "id"));
     }
     else
     {
@@ -238,12 +251,12 @@
         return;
     }
 
-    if (IS_MIDORI_BOOKMARKS_DB (parent))
-        KATZE_ARRAY_CLASS (midori_bookmarks_db_parent_class)->add_item (parent, item);
-    else if (KATZE_IS_ARRAY (parent))
-        katze_array_add_item (parent, item);
+    if (IS_MIDORI_BOOKMARKS_DB (db_parent))
+        KATZE_ARRAY_CLASS (midori_bookmarks_db_parent_class)->add_item (db_parent, item);
+    else if (KATZE_IS_ARRAY (db_parent))
+        katze_array_add_item (db_parent, item);
 
-    g_assert (parent == katze_item_get_parent (KATZE_ITEM (item)));
+    g_assert (db_parent == katze_item_get_parent (KATZE_ITEM (item)));
 }
 
 /**
@@ -267,7 +280,10 @@
 
     g_return_if_fail (parent);
 
-    katze_array_update (parent);
+    if (IS_MIDORI_BOOKMARKS_DB (parent))
+        KATZE_ARRAY_CLASS (midori_bookmarks_db_parent_class)->update (parent);
+    else
+        katze_array_update (parent);
 }
 
 /**
@@ -703,7 +719,7 @@
     g_return_val_if_fail (errmsg != NULL, NULL);
 
     database = midori_bookmarks_database_new (&error);
-    
+
     if (error != NULL)
     {
 	*errmsg = g_strdup (error->message);
@@ -714,7 +730,10 @@
     db = midori_database_get_db (MIDORI_DATABASE (database));
     g_return_val_if_fail (db != NULL, NULL);
 
-    bookmarks = MIDORI_BOOKMARKS_DB (g_object_new (TYPE_MIDORI_BOOKMARKS_DB, NULL));
+    bookmarks = MIDORI_BOOKMARKS_DB (g_object_new (TYPE_MIDORI_BOOKMARKS_DB,
+            "type", KATZE_TYPE_ITEM,
+            NULL));
+
     bookmarks->db = db;
 
     g_object_set_data (G_OBJECT (bookmarks), "db", db);
@@ -926,6 +945,8 @@
             KatzeItem* subitem;
             GList* sublist;
 
+            katze_array_clear (KATZE_ARRAY (item));
+
             KATZE_ARRAY_FOREACH_ITEM_L (subitem, subarray, sublist)
             {
                 katze_array_add_item (KATZE_ARRAY (item), subitem);
@@ -1087,3 +1108,54 @@
                                                       value, id,
                                                       recursive);
 }
+
+/**
+ * midori_bookmarks_db_populate_folder:
+ **/
+
+void
+midori_bookmarks_db_populate_folder (MidoriBookmarksDb* bookmarks,
+    KatzeArray *folder)
+{
+    const gchar* id = katze_item_get_meta_string (KATZE_ITEM (folder), "id");
+    const gchar *condition;
+    KatzeArray* array;
+    KatzeItem* item;
+    GList* list;
+
+    if (id == NULL)
+    {
+        condition = "parentid is NULL";
+    }
+    else
+    {
+        condition = "parentid = %q";
+    }
+
+    array = midori_bookmarks_db_query_recursive (bookmarks,
+        "id, title, parentid, uri, app, pos_panel, pos_bar", condition, id, FALSE);
+
+    if (IS_MIDORI_BOOKMARKS_DB (folder))
+    {
+        KATZE_ARRAY_FOREACH_ITEM_L (item, folder, list)
+        {
+            KATZE_ARRAY_CLASS (midori_bookmarks_db_parent_class)->remove_item (folder, item);
+        }
+
+        KATZE_ARRAY_FOREACH_ITEM_L (item, array, list)
+        {
+            KATZE_ARRAY_CLASS (midori_bookmarks_db_parent_class)->add_item (folder, item);
+        }
+    }
+    else
+    {
+        katze_array_clear(folder);
+
+        KATZE_ARRAY_FOREACH_ITEM_L (item, array, list)
+        {
+            katze_array_add_item (folder, item);
+        }
+    }
+
+    g_object_unref (array);
+}

=== modified file 'midori/midori-bookmarks-db.h'
--- midori/midori-bookmarks-db.h	2013-09-17 19:34:23 +0000
+++ midori/midori-bookmarks-db.h	2014-04-25 18:25:46 +0000
@@ -71,10 +71,9 @@
 				     KatzeItem*          folder,
 				     gboolean            recursive);
 
-gint64
-midori_bookmarks_insert_item_db (sqlite3*   db,
-                                 KatzeItem* item,
-                                 gint64     parentid);
+void
+midori_bookmarks_db_populate_folder (MidoriBookmarksDb* bookmarks,
+    KatzeArray *folder);
 
 #endif /* !__MIDORI_BOOKMARKS_DB_H__ */
 

=== modified file 'midori/midori-browser.c'
--- midori/midori-browser.c	2014-04-16 21:00:49 +0000
+++ midori/midori-browser.c	2014-04-25 18:25:46 +0000
@@ -935,10 +935,10 @@
     /* FIXME: here we should have the root bookmark array's name and id, not hard encoded values */
 
     gtk_tree_store_insert_with_values (model, &tree_iter, NULL, G_MAXINT,
-        0, _("Bookmarks"), 1, (gint64)0, -1);
+        0, _("Bookmarks"), 1, (gint64)-1, -1);
     gtk_combo_box_set_active_iter (GTK_COMBO_BOX (combo), &tree_iter);
 
-    current_parentid = 0;
+    current_parentid = -1;
     parent_iter = NULL;
     n = 1;
     while (g_list_first (folders))
@@ -959,7 +959,7 @@
                 {
                     /* folder's parent is the stree store root */
 
-                    current_parentid = 0;
+                    current_parentid = -1;
                     parent_iter = NULL;
                 }
                 else if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (model), &tree_iter))
@@ -1036,7 +1036,7 @@
 static gint64
 midori_bookmark_folder_button_get_active (GtkWidget* combo)
 {
-    gint64 id = 0;
+    gint64 id = -1;
     GtkTreeIter iter;
 
     g_return_val_if_fail (GTK_IS_COMBO_BOX (combo), 0);
@@ -3178,29 +3178,17 @@
                                    KatzeArray*    folder,
                                    MidoriBrowser* browser)
 {
-    KatzeArray* bookmarks;
-    const gchar* id = katze_item_get_meta_string (KATZE_ITEM (folder), "id");
-    gchar* condition;
-
     if (browser->bookmarks == NULL)
         return FALSE;
 
-    if (id == NULL)
-        condition = "parentid is NULL";
-    else
-        condition = "parentid = %q";
-
-    bookmarks = midori_bookmarks_db_query_recursive (browser->bookmarks,
-        "id, title, parentid, uri, app, pos_panel, pos_bar", condition, id, FALSE);
-    if (!bookmarks)
-        return FALSE;
+    midori_bookmarks_db_populate_folder (browser->bookmarks, folder);
 
     /* Clear items from dummy array here */
     gtk_container_foreach (GTK_CONTAINER (menu),
         (GtkCallback)(gtk_widget_destroy), NULL);
 
     /* "Import Bookmarks" and "Export Bookmarks" at the top */
-    if (id == NULL)
+    if (folder == KATZE_ARRAY (browser->bookmarks))
     {
         GtkWidget* menuitem;
         menuitem = gtk_action_create_menu_item (_action_by_name (browser, "BookmarksImport"));
@@ -3214,7 +3202,7 @@
         gtk_widget_show (menuitem);
     }
 
-    if (katze_array_is_empty (bookmarks))
+    if (katze_array_is_empty (folder))
     {
         GtkWidget* menuitem = gtk_image_menu_item_new_with_label (_("Empty"));
         gtk_widget_set_sensitive (menuitem, FALSE);
@@ -3223,7 +3211,7 @@
         return TRUE;
     }
 
-    katze_array_action_generate_menu (KATZE_ARRAY_ACTION (action), bookmarks,
+    katze_array_action_generate_menu (KATZE_ARRAY_ACTION (action), folder,
                                       menu, GTK_WIDGET (browser));
     return TRUE;
 }
@@ -6820,6 +6808,10 @@
             midori_bookmarkbar_remove_item_cb, browser);
     }
 
+    g_object_set (G_OBJECT (_action_by_name (browser, "Bookmarks")),
+        "array", KATZE_ARRAY (bookmarks),
+        NULL);
+
     settings = midori_browser_get_settings (browser);
     g_signal_handlers_disconnect_by_func (settings,
         midori_browser_show_bookmarkbar_notify_value_cb, browser);

=== modified file 'panels/midori-bookmarks.c'
--- panels/midori-bookmarks.c	2014-01-24 23:04:05 +0000
+++ panels/midori-bookmarks.c	2014-04-25 18:25:46 +0000
@@ -370,7 +370,7 @@
     GtkTreeModel* model = gtk_tree_view_get_model (GTK_TREE_VIEW (bookmarks->treeview));
     GtkTreeIter iter;
 
-    if (!parentid)
+    if (parentid == -1)
     {
         midori_bookmarks_add_item_to_model (GTK_TREE_STORE (model), NULL, item);
     }