Merge lp://qastaging/~aauzi/midori/fix-1179200 into lp://qastaging/midori

- if (item->parent)
- katze_array_update ((KatzeArray*)item->parent);
+ if (item->parent && g_strcmp0(icon, picon))
+ katze_array_update_item ((KatzeArray*)item->parent, item);

katze_array_update_item replaces katze_array_update if I see correctly, so that should be mentioned by marking the old one as Deprecated for clarity.

The checks like g_strcmp0(icon, picon) seem to avoid updating if nothing changed - in that case, why not do this for all values in each function?

A side note on the comment "panels/midori-bookmarks seems to be a view": yes, historically the code is badly mixed, in an ideal world both the panel and the toolbar should be independent views. Much like the Transfers extension these days.

> - if (item->parent)
> - katze_array_update ((KatzeArray*)item->parent);
> + if (item->parent && g_strcmp0(icon, picon))
> + katze_array_update_item ((KatzeArray*)item->parent, item);
>
> katze_array_update_item replaces katze_array_update if I see correctly, so
> that should be mentioned by marking the old one as Deprecated for clarity.

Not exactly... I kept the update for one purpose, the update of the whole data structure.

Basically here, the update in the bookmark panel, clears the whole tree and reloads it.

It thought this use case was still needed for features like bookmark imports, especially the way it is still designed.

Basically, the array_tree is a container. update is signal signaling an item and its containees, update_item only the item.

You probably have noticed that the update_item in the panel basically:
* finds the item in the tree
* unlinks it from where it is
* finds its new parent
* and relinks the item at its new position.

It doesn't do much if the item and its new parent is not visible.

So far, the bookmark bar does not implement such behaviour, it only refresh the whol bar on item change. This would probablye have to be changed for correct DND reordering of the bookmark bar.

> The checks like g_strcmp0(icon, picon) seem to avoid updating if nothing
> changed - in that case, why not do this for all values in each function?

Well, I didn't do it because it would most probably generate a lot of useless signal traffic.
The design I choose make midori/midori-bookmarks robust to poor design that would do multiple updates of the items but I wouldn't not encourage it. I like the speed of midori as it is.

I therefore just kept the automatic signal of the container on containee change where it was in place (I believe it's due to site title and favicon update after page load, isn't it?)

> A side note on the comment "panels/midori-bookmarks seems to be a view": yes,
> historically the code is badly mixed, in an ideal world both the panel and the
> toolbar should be independent views. Much like the Transfers extension these
> days.

Yes, I've identified the Model-View-Controller design pattern in the way the code is organised:
* the data base is the Model
* the midori/midori-bookmarks is the Controller
* and bookmarkbar and bookmarkpanel are Views of this model.

With this design pattern actions on the model should go through the controller and that's what I've implemented for delete and update.
I did not do it for insert for two reasons:
1. I didn't intend to change the whole design. I believe there are historical reasons to designs decision that may not be obvious on quick examination. The less I touch the better everybody is :)
2. I do not have a satisfying solution for the provision of the parentid on the insert of a child item I've seen in the file import. No solution => no change.

> > The checks like g_strcmp0(icon, picon) seem to avoid updating if nothing
> > changed - in that case, why not do this for all values in each function?

> Well, I didn't do it because it would most probably generate a lot of useless signal traffic.
> The design I choose make midori/midori-bookmarks robust to poor design that would do multiple updates of the > items but I wouldn't not encourage it. I like the speed of midori as it is.

> I therefore just kept the automatic signal of the container on containee change where it was in place
> (I believe it's due to site title and favicon update after page load, isn't it?)

What I meant was to wrap the whole of the values that change, for example:

{
     gchar* pname;
     g_return_if_fail (KATZE_IS_ITEM (item));
     pname = item->name;
     if (item->parent && g_strcmp0(name, pname))
     {
         katze_assign (item->name, g_strdup (name));
         katze_array_update((KatzeArray*)item->parent);
         g_object_notify (G_OBJECT (item), "name");
     }
}

I also realize you're doing "pname = item->name" before g_return_if_fail - don't, it will crash if the assertion is hit. Do it as I do above to be on the safe side.

> What I meant was to wrap the whole of the values that change, for example:
>
> {
> gchar* pname;
> g_return_if_fail (KATZE_IS_ITEM (item));
> pname = item->name;
> if (item->parent && g_strcmp0(name, pname))
> {
> katze_assign (item->name, g_strdup (name));
> katze_array_update((KatzeArray*)item->parent);
> g_object_notify (G_OBJECT (item), "name");
> }
> }
>
> I also realize you're doing "pname = item->name" before g_return_if_fail -
> don't, it will crash if the assertion is hit. Do it as I do above to be on the
> safe side.

OK, understood and agreed.
I will change that.

> > A side note on the comment "panels/midori-bookmarks seems to be a view":
> yes,
> > historically the code is badly mixed, in an ideal world both the panel and
> the
> > toolbar should be independent views. Much like the Transfers extension these
> > days.
>
> Yes, I've identified the Model-View-Controller design pattern in the way the
> code is organised:
> * the data base is the Model
> * the midori/midori-bookmarks is the Controller
> * and bookmarkbar and bookmarkpanel are Views of this model.
>
> With this design pattern actions on the model should go through the controller
> and that's what I've implemented for delete and update.
> I did not do it for insert for two reasons:

Let's do a little follow up on this topic.

Thanks to your remark in Bug #1185595 I felt encouraged to investigate the way bookmarks are retrieved from the database.

My investigations led me to two observations:
 1- katze_array_query_recursive is specialized to "bookmarks" table query due to the fact that the bookmarks table name is hard encoded in it
 2- katze_array_query_recursize lacks the feature of identifying folders item and populate them as katze_array of type KATZE_TYPE_ITEM.

My conclusions are the following:
 1- katze_array_query_recursive should be replaced by something like midori_bookmarks_query_recursive
 2- midori_bookmarks_query_recursive should allways select the column 'uri' of bookmarks in order to identify a folder item
 3- it should then parse the result rows for 'uri' value in order to determine if it can populate a katze_item or a katze_array of type KATZE_TYPE_ITEM

The connection with this bug fix proposal is the following.

The new function midori_bookmarks_query_recursive should be implemented in what I identify as the data base controller: midory/midory-bookmarks.c.

This would allow us to implement one additional steps forward in the direction of convergence of the bookmarks data and fix a concern pfor and I share, the triplication of data between bookmarks menu, bookmarkbar item and bookmarks side panel.

The hash table I used to implement packing of updates, or something similar, can also be used for items retrieval, before populating new items, midori_bookmarks_query_recursive should search the bookmarks array for their existence and only populate a new one a last resort.

Of course, the rows data should be inserted in an existing item, different client view may need different metadata.

To integrate well with the existing fix proposal, midori_bookmarks_query_recursive should flush the pending updates.

pfor, again, tipped me on how this flush could be sped up further: updates should be enclosed into SQLite transactions.

Finally, I was mentioning I did not have a solution for the insert, I think it is not the case anymore.

The katze_array_add_item callback of midori/midori-bookmarks.c should actually process the item added in order to make them available in the bookmarks array hash table.

This callback should change a folder implemented in katze_item into an empty katze_array of type
 KATZE_ARRAY_ITEM before inserting it in the database

It should also go through a katze_array of type KATZE_ARRAY:
 1- insert all child bookm...

> The consequences on the client views are the following:
> 1- they should ignore add_item of katze_array which are not type
> KATZE_TYPE_ITEM
> 2- they should ignore add_item of katze_item of a folder
>

Additional note:

The consequence on client side may be totally removed if midori_bookmarks derives from katze_array and implements the preprocessing of folders (convert in katze_array of type KATZE_ARRAY_ITEM) and the population of the content of a katze_array is done in a specialization of katze_array_add_item.

By doing so we could ensure that *valid* items would be signaled to the client views.

Ok, I've prototyped the ideas shared in the previous comment.

Now, all the database operations are implemented in midori/midori-bookmarks.c using transactions.

The result is impressive, my 883 bookmarks are now imported in #500ms where it was previously taking more than 6s.

The katze_array_query(_recursive) are now replaced by a fixed midori_bookmarks_query_recursive which ensures that the items are not duplicated between the different views.

Well, this is not clean code yet (far from it) but I was so excited by the impressive improvement that I wanted to share it for maybe some stress test feedback.

I therefore resubmit it as it is and will implement suggested changes in a clean update.

Well, this is now clean enough.