GNOME Bugzilla – Bug 611400
Middle click on back/forward list buttons doesn't work
Last modified: 2010-04-21 16:55:39 UTC
In the past, middle clicking on items in the back/forward menu opened the item in a new tab. Ever since the move to WebKit, it just opens a new blank tab.
The problem is here: static void activate_back_or_forward_menu_item_cb (GtkWidget *menuitem, EphyNavigationAction *action) { WebKitWebHistoryItem *item; EphyEmbed *embed; embed = ephy_embed_container_get_active_child (EPHY_EMBED_CONTAINER (action->priv->window)); g_return_if_fail (embed != NULL); if (ephy_gui_is_middle_click ()) { embed = ephy_link_open (EPHY_LINK (action), "about:blank", NULL, EPHY_LINK_NEW_TAB); g_return_if_fail (embed != NULL); } item = (WebKitWebHistoryItem*)g_object_get_data (G_OBJECT (menuitem), HISTORY_ITEM_DATA_KEY); g_return_if_fail (item != NULL); webkit_web_view_go_to_back_forward_item (EPHY_GET_WEBKIT_WEB_VIEW_FROM_EMBED (embed), item); } BackForward items can't be used with a different WebView. Back and forward buttons are closely linked to webkit's back/forward lists. At least as far as I can recall when I saw this a few weeks ago.
What is the expected behaviour when you middle-click in an element in the middle of one of the lists after opening the new tab? - Back history and forward history shows the elements before and after the selected item, which would remain as the current item. - The back history is kept, the forward history is forgotten and the new item appears as the current one. I ask this since the implementation for those would be, IMHO, slightly different: the second one could be a matter of just creating a new tab from the previous embed and then loading the uri of the selected item, while the first option would need more work to ensure both back and forward lists are coherent with the selected item.
Maybe it's time to re-think Back/Forward behaviour: https://bugzilla.gnome.org/show_bug.cgi?id=161872
(In reply to comment #2) > What is the expected behaviour when you middle-click in an element in the > middle of one of the lists after opening the new tab? > > - Back history and forward history shows the elements before and after the > selected item, which would remain as the current item. I've implemented this, as I thought it was the most sensible way to do it, through two patches. Attaching them right now...
Created attachment 158626 [details] [review] 1. New function to copy the forward history accross EphyWebView's Useful for creating new embeds from history items while keeping the context with regard to the selected item.
Created attachment 158628 [details] [review] 2. Open URIs on new tab when middle-clicking on an history item Open the history element on a new tab while keeping the right context for the selected item with regard to the back and forward lists.
(In reply to comment #3) > Maybe it's time to re-think Back/Forward behaviour: > https://bugzilla.gnome.org/show_bug.cgi?id=161872 Didn't see this, sorry :-( Will take a look tomorrow.
Created attachment 158693 [details] [review] 2. Open URIs on new tab when middle-clicking on an history item New version of the patch addressing some issues pointed out by Diego Escalante (via IRC) and some others I found myself :-)
Review of attachment 158626 [details] [review]: Looks good to me, just the nit pick on the docstring. Poke Xan for an oktc. ::: embed/ephy-web-view.c @@ +2155,3 @@ + * @dest: the #EphyWebView to copy the history to + * + * Sets the forward history (from to the current item) of @source as the The w00t? I think that () is a bit confusing, I think you can remove it and it's still easy to see what this function does.
Review of attachment 158693 [details] [review]: Looks good to me. Poke Xan for an oktc.
Created attachment 158790 [details] [review] 1. New function to copy the forward history accross EphyWebView's (In reply to comment #9) > Review of attachment 158626 [details] [review]: > > Looks good to me, just the nit pick on the docstring. Poke Xan for an oktc. > > ::: embed/ephy-web-view.c > @@ +2155,3 @@ > + * @dest: the #EphyWebView to copy the history to > + * > + * Sets the forward history (from to the current item) of @source as the > > The w00t? I think that () is a bit confusing, I think you can remove it and > it's still easy to see what this function does. :-) Yeah... doesn't sound very well I guess. I think somethign such "(starting after the current item)" would make more sense. Attaching reviewed patch
Review of attachment 158790 [details] [review]: It looks good otherwise, but the functions are so similar I feel they might be refactored into just one with a flag parameter... can do that afterwards. ::: embed/ephy-web-view.c @@ +2173,3 @@ + g_return_if_fail(EPHY_IS_WEB_VIEW(source)); + g_return_if_fail(EPHY_IS_WEB_VIEW(dest)); + You need spaces between parenthesis here. @@ +2195,3 @@ + + /* Select the current item */ + webkit_web_back_forward_list_go_to_item (dest_bflist, current_item); I think current_item can be NULL here if the list was empty? At least I added a check for this in the mirror function of this one.
Pressed the publish button too much, sorry about that...
Review of attachment 158693 [details] [review]: Mmmm, why not do the history copying in the EphyLink open implementation? Seems you are duplicating its logic about creating new embeds for no good reason here. Then you wouldn't need to change any code in EphyNavigationAction.
Created attachment 158874 [details] [review] 1. New function to copy the forward history accross EphyWebView's (In reply to comment #14) > Review of attachment 158790 [details] [review]: > > It looks good otherwise, but the functions are so similar I feel they might be > refactored into just one with a flag parameter... can do that afterwards. Done. > ::: embed/ephy-web-view.c > @@ +2173,3 @@ > + g_return_if_fail(EPHY_IS_WEB_VIEW(source)); > + g_return_if_fail(EPHY_IS_WEB_VIEW(dest)); > + > > You need spaces between parenthesis here. Done. > @@ +2195,3 @@ > + > + /* Select the current item */ > + webkit_web_back_forward_list_go_to_item (dest_bflist, current_item); > > I think current_item can be NULL here if the list was empty? At least I added a > check for this in the mirror function of this one. Sure, it was a mistake. Done. Attaching patch with changes
(In reply to comment #16) > Review of attachment 158693 [details] [review]: > > Mmmm, why not do the history copying in the EphyLink open implementation? Seems > you are duplicating its logic about creating new embeds for no good reason > here. Then you wouldn't need to change any code in EphyNavigationAction. IMHO, I think the main reason not to do it there is because the situation of needing to copy the forward history when open a new link is very special and tied to the case of open something from the history list, regardless it happens in the same view, a new tab or a new window. I was checking the open link implementation (in ephy-window.c) and I found I could do it there but then I would need to check whether the action of opening a link comes from the history buttons or not, leading to basically add some extra code there fully dependant on that thing. Hence I think it's better just to do it inside EphyNavigationAction as, in the end, is the only place where you'd like to get your forward history copied. Does this make sense? Of course, take this opinion with a grain of salt, as I'm relatively new in here and perhaps I'm missing something from the whole picture :-)
(In reply to comment #18) > IMHO, I think the main reason not to do it there is because the situation of > needing to copy the forward history when open a new link is very special and > tied to the case of open something from the history list, regardless it happens > in the same view, a new tab or a new window. > > I was checking the open link implementation (in ephy-window.c) and I found I > could do it there but then I would need to check whether the action of opening > a link comes from the history buttons or not, leading to basically add some > extra code there fully dependant on that thing. > > Hence I think it's better just to do it inside EphyNavigationAction as, in the > end, is the only place where you'd like to get your forward history copied. > > Does this make sense? > > Of course, take this opinion with a grain of salt, as I'm relatively new in > here and perhaps I'm missing something from the whole picture :-) EphyLink already has a flags parameter you can extend to cover exactly this case. I think it makes much more sense to centralize the creation of embeds in as few places as possible instead of scattering it all over the place.
(In reply to comment #19) > [...] > EphyLink already has a flags parameter you can extend to cover exactly this > case. I think it makes much more sense to centralize the creation of embeds in > as few places as possible instead of scattering it all over the place. Ah, ok. Didn't know of it. Thanks for pointing it out. I'll work on that asap, then.
Created attachment 159194 [details] [review] 1. Allow not copying history when creating a new embed (In reply to comment #20) > (In reply to comment #19) > > [...] > > EphyLink already has a flags parameter you can extend to cover exactly this > > case. I think it makes much more sense to centralize the creation of embeds in > > as few places as possible instead of scattering it all over the place. > > Ah, ok. Didn't know of it. Thanks for pointing it out. > > I'll work on that asap, then. After some discussion over IRC with Xan, we realized that using this approach was not so immediate thing as we thought before, and decided to keep working in the line of encapsulating all this thing in EphyNavigationHistoryAction class. Also I rebased my work against the last master since important changes were done there lately wrt the NavigationAction class (now with two subclasses), so expect a bit different patches here. Now attaching the first one...
Created attachment 159195 [details] [review] 2. Open URIs on new tab when middle-clicking on an history item Attaching the second (and last) patch
I've pushed both patches to master, thank you!