GNOME Bugzilla – Bug 392979
Location entry shows duplicate bookmarks and history entries
Last modified: 2007-12-26 18:36:38 UTC
1. Bookmark www.gnome.org 2. Type gnome in the location entry What happens: Your bookmark and your history item are displayed What should happen: You see only your bookmark (note that if you have gnome.org/about it is shown because it's not the same as gnome.org/) See next comment.
Created attachment 79420 [details] [review] Avoids duplicates in the location entry popup In ephy_completion_model_get_value () by if'ing I determine that the current processed node is a bookmark: bmknode = ephy_bookmarks_find_bookmark (bookmarks, url); Then I change the group variable to IGNORE_GROUP that comes from: enum { HISTORY_GROUP, - BOOKMARKS_GROUP + BOOKMARKS_GROUP, + IGNORE_GROUP, + N_GROUPS }; This way, the switch()'s in the init_*_col functions do the default: action instead of anything else. This way I avoid having to manage the "empty" need for this duplicated node. It's a pretty simple hack.
Created attachment 79421 [details] [review] Ignore dups in completion popup Oops, I included a ephy-window.c patch by mistake. Sorry. This one is the clean patch.
Created attachment 79514 [details] [review] Removed some comments
Is it possible to fix #324662 too with this patch? Or would it be far more complicated?
Just braindumping here; sorry for the spam. There's another bug that I can't find right now that's about smart bookmarks appearing in the normal bookmarks dropdown AND in the smart bookmarks panel of the dropdown. Selecting one of those (say "Search the web") would submit %s to Google. That kind of duplicate should be eliminated as well (though maybe it's outside the scope of this bug).
About bug #324662, the patch itself is a bit non-elegant because I couldn't find a place to iterate over the completion matches and the closest place is in lib/ instead of src/ so I couldn't use ephy-embed.h and ephy-bookmarks.h, at least as far as I understand I shouldn't. So if I'm understanding correctly there's no way to know in advance where to put a completion option... unless there's something like insert_completion_option_at_the_top() (maybe with -1 or something like that?). And about the smart bookmarks thing, it should be easy to do too. I'll check.
Created attachment 79581 [details] [review] Fixes the smart bookmarks thing. :)
I'm not sure I see how this patch addresses the bug... Shouldn't this require a modification to the _completion match function_ to just make the history entries for bookmarks be non-matches?
I couldn't find a way of accessing the bookmark* functions from src/bookmarks/ in lib/. But that's because I lack C skills :).
You can't access them in lib/. If you really do need to access them, the function should move to src/. Marking needs-work based on comment 8.
And how do I do that? O_O
The ephy-location-entry.h should export a function to set the completion func, and you implement the func in ephy-completion-model.c or in ephy-location-action.c ?
(In reply to comment #8) > I'm not sure I see how this patch addresses the bug... > > Shouldn't this require a modification to the _completion match function_ to > just make the history entries for bookmarks be non-matches? > I was looking at this again, happens that I need an EphyShell on lib/widgets/ephy-location-entry.c so I can get the bookmarks and therefore check for a matching bookmark. I'm attaching an updated version, what is your suggestion Christian?
Created attachment 95830 [details] [review] Updated version
Created attachment 101532 [details] [review] 20071223_bgo_392979_duped-bkmks-history Moves the completion_func from lib/widgets/ephy-location-entry.c to src/ephy-location-action.c. Adds a function to set the completion_func in lib/widgets/ephy-location-entry.c|h. Now, about the actual bug: I'm still trying to figure out how to know for sure that the node is ONLY an history item: + bookmarks = ephy_shell_get_bookmarks (ephy_shell); + bmknode = ephy_bookmarks_find_bookmark (bookmarks, url); Here I know if the url has an equivalent bookmark, now, this can be true for the bookmark and for the history item with the url of the bookmark. Hence we need another check: + history = ephy_history_get_pages (EPHY_HISTORY ( + ephy_embed_shell_get_global_history ( + embed_shell + ) + ) + ); This is stolen from ephy-completion-model.c: static void ephy_completion_model_get_value ... group = (iter->user_data2 == model->priv->history) ? HISTORY_GROUP : BOOKMARKS_GROUP; user_data2 is "root node", I don't know if that's reliable in my case... suggestions welcome!.
If I see this right, this patch only moves the function, without changes? If so, oktc. How about this: if you have a history URL (HISTORY_GROUP) and find_bookmark(url) == NULL then we show this item; otherwise there'll be a bookmark for it so it'll be present again on a different row... ?
Ok, committed the part to move the function. I'll try to have the bookmark filtering working. ------------------------------------------------------------------------ r7821 | diegoe | 2007-12-25 18:38:48 -0500 (Tue, 25 Dec 2007) | 8 lines Moves the completion_func from lib/widgets/ephy-location-entry.c to src/ephy-location-action.c. Adds a function to set the completion_func in lib/widgets/ephy-location-entry.c|h. Part of bug #392979.
Created attachment 101602 [details] [review] 20071225_bgo_392979_duped-bkmks-history.diff: Actually filter out history entries with an existing bookmark Filters like this: + if (ephy_bookmarks_find_bookmark (bookmarks, url) != NULL && + (iter2.user_data2 == history)) + ret = FALSE; That reads: "if there's a bookmark for $url AND $url is in history, then skip this history entry, so the bookmark is shown". I'm a bit worried if it's unwise to get bookmarks and history each time completion_func is called: + bookmarks = ephy_shell_get_bookmarks (ephy_shell); + history = ephy_history_get_pages (EPHY_HISTORY ( + ephy_embed_shell_get_global_history ( + embed_shell + ) + )); Should it be catched or something?. One last thing, this doesn't totally remove dupe'd entries, take for example google, if you type 'google.com' in the location bar you will be redirected to 'www.google.com' and then to 'www.google.com.pe' (in my case) so I'll have: |go_________________| |google.com |www.google.com |Google (the bookmark'd www.google.com.pe) For other sites like osnews.com, this doesn't happen. It's not a bug, just a nitpick we can't avoid -i think-.
+ history = ephy_history_get_pages (EPHY_HISTORY ( + ephy_embed_shell_get_global_history ( + embed_shell + ) + )); Try to fit that in 2 lines :) + if (ephy_bookmarks_find_bookmark (bookmarks, url) != NULL && + (iter2.user_data2 == history)) + ret = FALSE; Should add a comment on what the idea behind this is (i.e. that we'll get the same URL again as a bookmark). With the nits fixed, ok to commit. Thanks!
Committed. ------------------------------------------------------------------------ r7826 | diegoe | 2007-12-26 13:32:54 -0500 (Wed, 26 Dec 2007) | 5 lines Avoid duplicated history and bookmark entries for the same url in the completion popup. Fixes bug #392979.