GNOME Bugzilla – Bug 781746
Bookmark tags no longer matched against text in address bar
Last modified: 2017-07-17 18:09:08 UTC
After the new bookmarks UI landed in 3.24, typing text into the location bar no longer shows matches based on bookmark tags. This was exceptionally useful when finding bookmarks since it meant I could very easily bring up not only whole classes of bookmarks, but very easily narrow the search down by adding more tags, all without having to remember the page title or URL, or search through through the Bookmarks menu. E.g.: Typing "bugs" would show me all bug trackers I have bookmarked, but "gnome bugs" would narrow it down to GNOME-related trackers, or "geary bugs" to instantly bring up the two Geary-bug related related bookmarks I use frequently. Moreover (IIRC) this worked on sub-string matches of tag names, so I could get away with just typing "gn b" or "ge b", instead of the full strings above. Although in reality I'd usually type something like "bugs" first since that was what I was thinking of, then " g" followed by "n" or "e" to start narrowing the search down to match what I wanted. The new Bookmarks UI doesn't let you form these tag intersections and involves a lot of visual search, and so it's a lot slower to find the bookmarks I want.
Good idea. I think this just requires modifying should_add_bookmark_to_model() in ephy-completion-model.c. Should be straightforward, so tagging as a newcomers bug.
Created attachment 351773 [details] [review] completion-model: match tags against text in address bar This is similarly to how it was implemented in 3.22 https://git.gnome.org/browse/epiphany/tree/src/ephy-completion-model.c?h=gnome-3-22#n294
Review of attachment 351773 [details] [review]: OK, thank you for this patch, and sorry for the delay in reviewing it. It looks good and works properly in my testing. I found two minor code style errors, and I found one memory leak. I'll push this to both master and gnome-3-24 (since it's a relatively small change that fixes a regression) once you make the requested fixes. ::: src/ephy-completion-model.c @@ +307,3 @@ + tags = ephy_bookmark_get_tags (bookmark); + + tag_array = g_malloc0((g_sequence_get_length (tags) + 1) * sizeof (char *)); Remember our code style requires one space before opening parentheses in function calls: g_malloc0 ((g_sequence_get_length (tags) + 1) * sizeof (char *)) @@ +312,3 @@ + !g_sequence_iter_is_end (tag_iter); + i++, tag_iter = g_sequence_iter_next (tag_iter)) { + tag_array[i] = g_sequence_get (tag_iter); My first instinct is that this is way too much code to concatenate a list of strings together. But sadly, I think this might actually be the best way to do it. If the list of tags was an array instead of a GSequence, then you could skip this and just use g_strjoinv() directly, but... it's not. So I think this is fine. @@ +315,3 @@ + } + + tag_string = g_strjoinv(" ", tag_array); tag_string = g_strjoinv (" ", tag_array); @@ +333,3 @@ } + g_free (tag_array); This frees the array that holds the strings, but leaks the strings themselves. You need to ensure that g_free() gets called on each string and then finally the array itself. Fortunately we have a convenience function for that, g_strfreev(), which you can use here because you've already ensured the array is NULL-terminated. I think that should be right, but you should sanity-check by testing the patch once again to make sure it doesn't crash after making this change.
(In reply to Michael Catanzaro from comment #3) > This frees the array that holds the strings, but leaks the strings > themselves. Ah wait, I'm wrong, because the array does not own any of the strings it holds: you get them from g_sequence_get(), which does not transfer ownership. So there should not be any leak. I'll add the two missing space characters and push this now.
Attachment 351773 [details] pushed as cb8f86f - completion-model: match tags against text in address bar
This is going out in today's 3.24.3 release. Better late than never!