GNOME Bugzilla – Bug 671609
ephy-navigation-history-action: restore menus
Last modified: 2012-05-03 23:25:03 UTC
Work as they did before.
Created attachment 209221 [details] [review] ephy-navigation-history-action: restore menus In ebbb1c48197f53b98575b0cb4f6d9fa1e4535abc back/forward drop-downs were removed. This commit brings them back, using the removed code with minor updates.
I am still missing tooltips here. I would just use gtk_widget_set_tooltip so to avoid hacking around EphyEmbed statusbar code. Alternatively I'm exploring using leave-notify-event + query-tooltip to get the statusbar to hide and show when it should.
Review of attachment 209221 [details] [review]: Let's remove about:blank from this menu now, it's pointless to have it there. Also, this seems to get wrong the titles for google search pages, I just get the same string again and again. ::: src/ephy-navigation-history-action.c @@ +365,3 @@ + + web_view = EPHY_GET_WEBKIT_WEB_VIEW_FROM_EMBED (embed); + g_return_val_if_fail (web_view != NULL, NULL); This should not really happen... @@ +414,3 @@ + GtkWidget *menu; + + if (event->button == 3) { We want to do this in a timeout like chrome and firefox, I think. No point in being different here IMHO. @@ +448,2 @@ action_class->activate = action_activate; + action_class->connect_proxy = connect_proxy; No disconnect proxy?
(In reply to comment #2) > I am still missing tooltips here. I would just use gtk_widget_set_tooltip so to > avoid hacking around EphyEmbed statusbar code. We don't really like tooltips anymore, so skip that. > > Alternatively I'm exploring using leave-notify-event + query-tooltip to get the > statusbar to hide and show when it should. And this too.
What is the smartest thing for the timeout? I am conditioning the popup on the mouse still being over the button, I am caching this with enter/leave events, but I guess a more direct and instant way would be to get coords for pointer+widget? What do you suggest?
(In reply to comment #5) > What is the smartest thing for the timeout? > > I am conditioning the popup on the mouse still being over the button, I am > caching this with enter/leave events, but I guess a more direct and instant way > would be to get coords for pointer+widget? > > > What do you suggest? Cancelling the timeout on mouse leave seems reasonable. I would not start it again on mouse enter, though, I think.
Created attachment 210276 [details] [review] ephy-navigation-history-action: restore menus Here's an update. I'm a bit unsure of my solution, though. What do you think?
Comment on attachment 210276 [details] [review] ephy-navigation-history-action: restore menus As suggested on jabber, this should use g_idle_add_full instead of boolean hacks. Also, remember to deal with the "about:blank" stuff, and drop the popup on button 3 :P
Created attachment 210729 [details] [review] ephy-navigation-history-action: restore menus In ebbb1c48197f53b98575b0cb4f6d9fa1e4535abc back/forward drop-downs were removed. This commit brings them back, using the removed code with minor updates.
*** Bug 649368 has been marked as a duplicate of this bug. ***
I've been running with Diego's latest patch for a few days now and it seems to work great. Looking forward to seeeing this committed!
One minor issue: the restored history menu can grow very large and end up being taller than the display. It would be nice to limit the history menu to 20 entries or so, I think.
Created attachment 211515 [details] [review] ephy-navigation-history-action: restore menus In ebbb1c48197f53b98575b0cb4f6d9fa1e4535abc back/forward drop-downs were removed. This commit brings them back, using the removed code with minor updates. -- Fixes Adam comment about length
Created attachment 211516 [details] [review] e-navigation-history-action: enable menu tooltips Use EphyWebView::link-message property to pop messages in the view's statusbar. -- Since I am not 100% sure of the implementation of this, I preferred to attach it as a follow up. This makes tooltips pop in the statusbar, like I think we did before. Chrome doesn't use this. Firefox does. I think we should.
Thanks, Diego - these patches seem to work well. I definitely like seeing the tooltips in the statusbar too. I think displaying only 10 history items is a little bit stingy. Chrome shows 12 and Firefox shows 15 - could we increase this to one of those numbers?
Review of attachment 211515 [details] [review]: ::: src/ephy-navigation-history-action.c @@ +235,3 @@ + + g_return_if_fail (EPHY_IS_EMBED (source)); + g_return_if_fail (EPHY_IS_EMBED (dest)); In general it's a bit odd to have g_return_if_fail in private methods. Is it reasonable for this to happen here? @@ +360,3 @@ + +static GtkWidget * +build_dropdown_menu (EphyNavigationHistoryAction *action) This seems to not add favicons to the menu items. I think it should? @@ +394,3 @@ + title = g_strdup (webkit_web_history_item_get_title (hitem)); + + if ((title == NULL || g_strstrip (title)[0] == '\0')) Extra pair of parenthesis here. @@ +453,3 @@ + action->priv->menu_timeout = g_timeout_add_full (G_PRIORITY_DEFAULT, 500, + (GSourceFunc) menu_timeout_cb, data, + (GDestroyNotify) g_free); The indentation seems to be a bit screwed here. @@ +461,3 @@ + NULL, NULL, + ephy_gui_menu_position_under_widget, button, + event->button, event->time); Small thing, but I think it would be good to have the code to popup the menu in one place, and call it directly here and from the timeout above.
Review of attachment 211516 [details] [review]: We have stopped doing this pretty much everywhere in the UI save for links in the web content. Any strong reason to do it again? The links in that menu have already been visited, so I think the "I have no idea where this link leads to" should not be present anymore.
I think we should show URLs in the statusbar when the user mouses over items in the history menu. Sometimes Web pages have clear, descriptive titles. Sometimes they don't. It sometimes happens that I want to jump back several pages in a site I've been navigating where all recent pages have the very same or similar titles. In those cases the URL can often provide a helpful clue as to which page I want.
(In reply to comment #18) > I think we should show URLs in the statusbar when the user mouses over items in > the history menu. Sometimes Web pages have clear, descriptive titles. > Sometimes they don't. It sometimes happens that I want to jump back several > pages in a site I've been navigating where all recent pages have the very same > or similar titles. In those cases the URL can often provide a helpful clue as > to which page I want. This.
Created attachment 211968 [details] [review] ephy-navigation-history-action: restore menus In ebbb1c48197f53b98575b0cb4f6d9fa1e4535abc back/forward drop-downs were removed. This commit brings them back, using the removed code with minor updates. -- Updated patch for favicons, code style and including the tooltips.
Created attachment 212099 [details] [review] ephy-navigation-history-action: restore menus In ebbb1c48197f53b98575b0cb4f6d9fa1e4535abc back/forward drop-downs were removed. This commit brings them back, using the removed code with minor updates. --- Was missing a button-release to stop the menu from popping when you click back , release and then leave the cursor on top.
Seems to work fine. Looking forward to seeing this committed!
Comment on attachment 212099 [details] [review] ephy-navigation-history-action: restore menus OK, I've pushed this into 3.5.1. There's a small bug that I've found at least, if the menu is created when the favicon has not yet been retrieved it will never show up. I guess it should.
Closing.
*** Bug 675056 has been marked as a duplicate of this bug. ***