Merge lp:~evfool/midori/lp1170850 into lp:midori

[Robert Roth]

Display locationbar suggestions in the correct order

Copied from the commit message:

    Display locationbar suggestions in the correct order (lp:1170850)

    This consists of the following changes:
    * Add a priority property to the suggestion objects, initialized to the
    position of the parent completion item
    * When adding the results of an async call of a completion to get the
    suggestions, find the index of the last item with higher priority, and
    add the new suggestions after that.

    This is required, as the current async loading order can not guarantee
    that the suggestions will appear in the correct order, as it depends
    on the execution time of the completion loading, when suggestions of a
    specific completion will be added. Calculating the start index based on
    the position of the completion in itself, as we would also need to know
    how many items the other completions (appearing before this one) will add.

[Cris Dywan (kalikiana)]

I'm liking this priority concept! Just two comments here:

Might it have been simpler to use the List instead of TreeModelForeachFunc? Though no strong opinion, it looks sensible just unexpected.

One thing I do find awkward is adding the position to every Suggestion - there's nothing useful to do with it for a Completion subclass. Given that Autocompleter gets one complete List<Suggestion> always and knows where it comes from I'd prefer it to handle this behind the scenes.

review: Needs Fixing

[Robert Roth (evfool)]

I'm not sure I can follow you: I am aware that the Autocompleter gets one complete List<Suggestion>, which gets added to the full list, but finding the position where it needs to be inserted depends on the items already in the list. Consider the following usecase:
* Async tabs completion starts
* Async history completion starts
* Async search completion starts
* Async tabs completion finishes with 3 results, the model is empty, we simply add the elements of the list
* Async search completes before the history, now we have to find where to insert the results: we have the model having 3 elements, and we have the current list of elements,knowing that their position (from the completion) is 2. Unfortunately we don't know what that means, whether the 3 items in the model are the aggregated list of the Completions with position 0 and 1, or are coming from only one Completion, and the other one has not finished yet.
* Async history completion finishes, with Completion position 1, but we still don't know what to do with it.

Are you sure finding the correct position is possible without knowing the origin of each model element currently in the model?

[Cris Dywan (kalikiana)]

I didn't mean to never save the position but it should not be visible to the individual completion classes. At the point where the List<Suggestion> is passed to Autocompleter it will know what Completion it came from so position can be taken from that, no? If it has to be stored on the Suggestion, fair enough, but I'd prefer it to be handled transparently by Autocompleter.

[lp:~evfool/midori/lp1170850]

6557. By Robert Roth on 2014-02-16

Merged from trunk.

[Cris Dywan (kalikiana)]

Well, I'm gonna say let's get this in as-is, I really like this fix despite that little quirk.

review: Approve

[Robert Roth (evfool)]

Thanks, I'm still thinking on a better solution, but haven't found it yet. I have tried various solutions, but none of them worked as expected, so I have dropped them. Anyway, when I'll have a better solution, I'll propose another merge. Thanks for accepting this though until a better solution.

=== modified file 'midori/midori-completion.vala'
--- midori/midori-completion.vala	2013-06-25 17:13:34 +0000
+++ midori/midori-completion.vala	2014-02-16 07:32:20 +0000
@@ -17,12 +17,13 @@
         public string? background { get; set; }
         public GLib.Icon? icon {  get; set; }
         public bool action { get; set; default = false; }
+        public int priority {get; set; default = 0; }
 
         public Suggestion (string? uri, string? markup, bool use_markup=false,
-            string? background=null, GLib.Icon? icon=null) {
+            string? background=null, GLib.Icon? icon=null, int? priority=0) {
 
             GLib.Object (uri: uri, markup: markup, use_markup: use_markup,
-                         background: background, icon: icon);
+                         background: background, icon: icon, priority: priority);
         }
     }
 
@@ -53,6 +54,7 @@
             BACKGROUND,
             YALIGN,
             SIZE,
+            PRIORITY,
             N
         }
 
@@ -62,7 +64,7 @@
             next_position = 0;
             model = new Gtk.ListStore (Columns.N,
                 typeof (GLib.Icon), typeof (string), typeof (string),
-                typeof (string), typeof (float), typeof (uint));
+                typeof (string), typeof (float), typeof (uint), typeof (int));
         }
 
         public void add (Completion completion) {
@@ -96,20 +98,34 @@
                 need_to_clear = false;
                 current_count = 0;
             }
+            int start = 0;
+            // find the first index with priority greater than the current one
+            Gtk.TreeModelForeachFunc find_index = (model, path, iter) => {
+                GLib.Value priority;
 
+                model.get_value (iter,Columns.PRIORITY, out priority);
+                if ((int)priority < completion.position) {
+                    start++;
+                    return false;
+                }
+                return true;
+            };
+            model.foreach(find_index);
+            int count = 0;
 #if HAVE_GRANITE
             if (completion.description != null) {
-                model.insert_with_values (null, completion.position,
+                model.insert_with_values (null, start,
                     Columns.URI, "about:completion-description",
                     Columns.MARKUP, "<b>%s</b>\n".printf (Markup.escape_text (completion.description)),
                     Columns.ICON, null,
                     Columns.SIZE, Gtk.IconSize.MENU,
                     Columns.BACKGROUND, null,
-                    Columns.YALIGN, 0.25);
+                    Columns.YALIGN, 0.25,
+                    Columns.PRIORITY, completion.position);
+                count++;
             }
 #endif
 
-            int count = 1;
             foreach (var suggestion in suggestions) {
                 if (suggestion.uri == null) {
                     warning ("suggestion.uri != null");
@@ -119,14 +135,15 @@
                     warning ("suggestion.markup != null");
                     continue;
                 }
-                model.insert_with_values (null, completion.position + count,
+                model.insert_with_values (null, start + count,
                     Columns.URI, suggestion.uri,
                     Columns.MARKUP, suggestion.use_markup
                     ? suggestion.markup : Markup.escape_text (suggestion.markup),
                     Columns.ICON, scale_if_needed (suggestion.icon),
                     Columns.SIZE, Gtk.IconSize.MENU,
                     Columns.BACKGROUND, suggestion.background,
-                    Columns.YALIGN, 0.25);
+                    Columns.YALIGN, 0.25,
+                    Columns.PRIORITY, completion.position);
 
                 count++;
                 if (count > completion.max_items)

=== modified file 'midori/midori-historycompletion.vala'
--- midori/midori-historycompletion.vala	2013-09-13 21:24:57 +0000
+++ midori/midori-historycompletion.vala	2014-02-16 07:32:20 +0000
@@ -51,7 +51,7 @@
                 else if (item is Midori.HistorySearch) {
                     var search = item as Midori.HistorySearch;
                     suggestions.append (new Suggestion (search.uri, search.title + "\n" + search.uri,
-                        false, "gray", Midori.Paths.get_icon (search.uri, null)));
+                        false, "gray", Midori.Paths.get_icon (search.uri, null), this.position));
                 }
                 else
                     warn_if_reached ();

=== modified file 'midori/midori-locationaction.c'
--- midori/midori-locationaction.c	2013-11-05 14:51:49 +0000
+++ midori/midori-locationaction.c	2014-02-16 07:32:20 +0000
@@ -661,12 +661,12 @@
         g_object_unref (app);
         midori_autocompleter_add (action->autocompleter,
             MIDORI_COMPLETION (midori_view_completion_new ()));
-        midori_autocompleter_add (action->autocompleter,
-            MIDORI_COMPLETION (midori_search_completion_new ()));
         /* FIXME: Currently HistoryCompletion doesn't work in memory */
         if (action->history != NULL)
             midori_autocompleter_add (action->autocompleter,
                 MIDORI_COMPLETION (midori_history_completion_new ()));
+        midori_autocompleter_add (action->autocompleter,
+            MIDORI_COMPLETION (midori_search_completion_new ()));
     }
 
     if (!midori_autocompleter_can_complete (action->autocompleter, action->key))

=== modified file 'midori/midori-searchcompletion.vala'
--- midori/midori-searchcompletion.vala	2013-08-08 23:31:08 +0000
+++ midori/midori-searchcompletion.vala	2014-02-16 07:32:20 +0000
@@ -56,6 +56,7 @@
                 if (n == 3 && action == null) {
                     suggestion = new Suggestion ("complete:more/search", _("Search with…"), false, background);
                     suggestion.action = true;
+                    suggestion.priority = this.position;
                     suggestions.append (suggestion);
                     break;
                 }

=== modified file 'midori/midori-viewcompletion.vala'
--- midori/midori-viewcompletion.vala	2013-08-08 23:31:08 +0000
+++ midori/midori-viewcompletion.vala	2014-02-16 07:32:20 +0000
@@ -72,7 +72,7 @@
                     Gdk.Pixbuf? icon = Midori.Paths.get_icon (uri, null);
                     /* FIXME: Theming? Win32? */
                     string background = "gray";
-                    var suggestion = new Suggestion (uri, title + "\n" + uri, false, background, icon);
+                    var suggestion = new Suggestion (uri, title + "\n" + uri, false, background, icon, this.position);
                     suggestions.append (suggestion);
 
                     n++;