After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 611400 - Middle click on back/forward list buttons doesn't work
Middle click on back/forward list buttons doesn't work
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Controls
2.29.x
Other Linux
: Normal minor
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-02-28 14:40 UTC by Bruce Cowan
Modified: 2010-04-21 16:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
1. New function to copy the forward history accross EphyWebView's (3.57 KB, patch)
2010-04-13 18:06 UTC, Mario Sánchez Prada
none Details | Review
2. Open URIs on new tab when middle-clicking on an history item (3.47 KB, patch)
2010-04-13 18:06 UTC, Mario Sánchez Prada
none Details | Review
2. Open URIs on new tab when middle-clicking on an history item (3.32 KB, patch)
2010-04-14 07:50 UTC, Mario Sánchez Prada
needs-work Details | Review
1. New function to copy the forward history accross EphyWebView's (3.57 KB, patch)
2010-04-15 08:55 UTC, Mario Sánchez Prada
needs-work Details | Review
1. New function to copy the forward history accross EphyWebView's (6.13 KB, patch)
2010-04-16 08:20 UTC, Mario Sánchez Prada
none Details | Review
1. Allow not copying history when creating a new embed (1.99 KB, patch)
2010-04-20 20:37 UTC, Mario Sánchez Prada
none Details | Review
2. Open URIs on new tab when middle-clicking on an history item (5.60 KB, patch)
2010-04-20 20:38 UTC, Mario Sánchez Prada
none Details | Review

Description Bruce Cowan 2010-02-28 14:40:50 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.
Comment 1 Diego Escalante Urrelo (not reading bugmail) 2010-04-12 19:33:01 UTC
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.
Comment 2 Mario Sánchez Prada 2010-04-13 15:42:57 UTC
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.
Comment 3 Reinout van Schouwen 2010-04-13 16:03:23 UTC
Maybe it's time to re-think Back/Forward behaviour: 
https://bugzilla.gnome.org/show_bug.cgi?id=161872
Comment 4 Mario Sánchez Prada 2010-04-13 18:04:53 UTC
(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...
Comment 5 Mario Sánchez Prada 2010-04-13 18:06:02 UTC
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.
Comment 6 Mario Sánchez Prada 2010-04-13 18:06:50 UTC
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.
Comment 7 Mario Sánchez Prada 2010-04-13 18:07:15 UTC
(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.
Comment 8 Mario Sánchez Prada 2010-04-14 07:50:26 UTC
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 :-)
Comment 9 Diego Escalante Urrelo (not reading bugmail) 2010-04-15 03:26:34 UTC
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.
Comment 10 Diego Escalante Urrelo (not reading bugmail) 2010-04-15 03:30:27 UTC
Review of attachment 158693 [details] [review]:

Looks good to me. Poke Xan for an oktc.
Comment 11 Mario Sánchez Prada 2010-04-15 08:55:39 UTC
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
Comment 12 Xan Lopez 2010-04-15 16:46:07 UTC
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.
Comment 13 Xan Lopez 2010-04-15 16:46:07 UTC
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.
Comment 14 Xan Lopez 2010-04-15 16:46:15 UTC
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.
Comment 15 Xan Lopez 2010-04-15 16:47:08 UTC
Pressed the publish button too much, sorry about that...
Comment 16 Xan Lopez 2010-04-15 16:55:46 UTC
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.
Comment 17 Mario Sánchez Prada 2010-04-16 08:20:58 UTC
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
Comment 18 Mario Sánchez Prada 2010-04-16 09:18:28 UTC
(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 :-)
Comment 19 Xan Lopez 2010-04-19 12:55:23 UTC
(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.
Comment 20 Mario Sánchez Prada 2010-04-19 13:19:17 UTC
(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.
Comment 21 Mario Sánchez Prada 2010-04-20 20:37:52 UTC
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...
Comment 22 Mario Sánchez Prada 2010-04-20 20:38:42 UTC
Created attachment 159195 [details] [review]
2. Open URIs on new tab when middle-clicking on an history item

Attaching the second (and last) patch
Comment 23 Xan Lopez 2010-04-21 16:55:39 UTC
I've pushed both patches to master, thank you!