GNOME Bugzilla – Bug 628364
gtk_button_pressed and gtk_button_released are deprecated
Last modified: 2012-01-01 16:31:56 UTC
They are still used to trigger the "clicked" visual effect when middle-clicking items in the toolbar (ephy-link-action, ephy-bookmark-action). We need to remove this: src/bookmarks/ephy-bookmark-action.c: gtk_button_pressed (GTK_BUTTON (widget)); src/ephy-link-action.c: gtk_button_pressed(button); src/bookmarks/ephy-bookmark-properties.c: gtk_button_released (GTK_BUTTON (button)); src/bookmarks/ephy-topic-action.c: gtk_button_released (GTK_BUTTON (button)); src/bookmarks/ephy-bookmark-action.c: gtk_button_released (GTK_BUTTON (widget)); src/ephy-link-action.c: gtk_button_released(button); Chrome achieves this effect using a GtkEventBox wrapping the buttons. I'm investigating that.
Nautilus bug #600475 and GTK+ bug #610515
Created attachment 181517 [details] [review] Remove use of deprecated symbols
(In reply to comment #2) > Created an attachment (id=181517) [details] [review] > Remove use of deprecated symbols Some really high level comments: - Fix indentation, you are breaking it everywhere. - Let's do one patch removing the supposedly useless release calls in the toggle buttons. Why are they useless? Explain in the patch. - Mucking around with private data is really dirty. I prefer we do a button subclass as Mitch suggested in the GTK+ bug report.
Created attachment 181614 [details] [review] Remove the use of deprecated gtk_button_pressed/released This patch introduces a GtkButton subclass handling button 2 (middle) as button 1 while keeping the original event intact.
Created attachment 181615 [details] [review] Remove the use of deprecated gtk_button_pressed/released EphyBookmarkAction was modified to use EphyMiddleClickButton instead of relying on the deprecated GtkButton's methods _pressed and _released.
Created attachment 181616 [details] [review] Remove the use of deprecated gtk_button_pressed/released Remove its usage in EphyLinkAction, leaving this class nearly empty (it sole purpose was to enforce the middle-click policy). At a later stage, common code from its descendants should go down here
Created attachment 181617 [details] [review] Remove the use of deprecated gtk_button_pressed/released Remove useless calls to gtk_button_release. Those calls had no effect, since they were following calls to gtk_toggle_button_set_active (FALSE) doing implicit release.
Created attachment 181618 [details] [review] Remove the use of deprecated gtk_button_pressed/released This patch makes the go action use EphyMiddleClickButton. This makes the button look pressed when middle clicked.
Review of attachment 181614 [details] [review]: Looks reasonable, but please merge this in one patch together with the patch where you use it, no point in over-splitting things. ::: lib/widgets/ephy-middle-click-button.c @@ +1,3 @@ +/* -*- Mode: C; tab-width: 2; indent-tabs-mode: s; c-basic-offset: 2 -*- */ +/* + * Copyright © 2011 Igalia.com Igalia S.L. is the official name :) @@ +22,3 @@ +#include "ephy-middle-click-button.h" + +#include <gdk/gdk.h> You are including gtk.h in the header, so this is not needed I think. @@ +28,3 @@ +G_DEFINE_TYPE (EphyMiddleClickButton, ephy_middle_click_button, GTK_TYPE_BUTTON) + +#define IS_MIDDLE_CLICK(e) (e->button == 2) IMHO not worth defining this. @@ +34,3 @@ +ephy_middle_click_button_handle_mouse_event (GtkWidget * widget, + const GdkEventButton * event, + mouse_event_handler parent_method) Indentation is wrong. const is useless for pointers in C, so don't use it (I think you might get a warning?). @@ +51,3 @@ +static gboolean +ephy_middle_click_button_button_release (GtkWidget * widget, + GdkEventButton * event) Indentation again. @@ +54,3 @@ +{ + return ephy_middle_click_button_handle_mouse_event (widget, event, + GTK_WIDGET_CLASS (ephy_middle_click_button_parent_class)->button_release_event); align the third parameter. @@ +63,3 @@ + return ephy_middle_click_button_handle_mouse_event (widget, event, + GTK_WIDGET_CLASS (ephy_middle_click_button_parent_class)->button_press_event); +} Same comments. ::: lib/widgets/ephy-middle-click-button.h @@ +43,3 @@ +typedef struct _EphyMiddleClickButton EphyMiddleClickButton; +typedef struct _EphyMiddleClickButtonClass EphyMiddleClickButtonClass; + Indentation. @@ +53,3 @@ + GtkButtonClass parent_class; +}; + This is not using 2 spaces for indentation. @@ +57,3 @@ + +GtkWidget *ephy_middle_click_button_new (); + This is not aligned either.
Review of attachment 181615 [details] [review]: Looks great, but merge it as pointed before.
Review of attachment 181617 [details] [review]: Looks good. We usually do Bug #628364 for the bug reference, though.
Review of attachment 181618 [details] [review]: Looks good, merge it with the two first patches too.
Review of attachment 181616 [details] [review]: So, if this code is being removed with replacement it means we are breaking things. We need to figure out how to force all buttons being added to toolbars to be EphyMiddleClickButtons, otherwise we are missing the middle click functionality.
Created attachment 181635 [details] [review] Make EphyHomeAction middle-click aware Make the EphyHomeAction represented by an EphyMiddleClickButton by overriding the tool item factory method.
Created attachment 181643 [details] [review] Remove gtk_buttom_pressed/released deprecated methods usage 1/2 Introduces a GtkButton-derived widget handling middle-click events. Makes EphyHomeAction, EphyGoAction and EphyBookmarkAction use to change the widget state instead relying on the deprecated methods. EphyLink becomes an empty nutshell as its only purpose was to handle the middle-click case. In the future, code will be mutualized there.
Created attachment 181644 [details] [review] Remove deprecated gtk_button_pressed/released methods usage 2/2 Remove useless calls to gtk_button_release. Those calls had no effect, since they were following calls to gtk_toggle_button_set_active (FALSE) doing implicit release.
Created attachment 181646 [details] [review] Replace gtk_buttom_pressed/released deprecated methods Introduces a GtkButton-derived widget handling middle-click events. Makes EphyHomeAction, EphyGoAction and EphyBookmarkAction use to change the widget state instead relying on the deprecated methods. EphyLink becomes an empty nutshell as its only purpose was to handle the middle-click case. In the future, code will be mutualized there.
Created attachment 181647 [details] [review] Remove deprecated gtk_button_pressed/released usage Remove useless calls to gtk_button_release. Those calls had no effect, since they were following calls to gtk_toggle_button_set_active (FALSE) doing implicit release.
Review of attachment 181646 [details] [review]: Some trivial comments. ::: lib/widgets/ephy-middle-click-button.c @@ +21,3 @@ +#include "ephy-middle-click-button.h" + +GtkWidget *ephy_middle_click_button_new (); You shouldn't need this line, since it's in your .h @@ +34,3 @@ + gboolean ret; + + GdkEvent *event_dup = gdk_event_copy ((GdkEvent *)event); What happens if you don't copy the event? @@ +59,3 @@ + GdkEventButton * event) +{ +GtkWidgetClass *widget_class = GTK_WIDGET_CLASS (ephy_middle_click_button_parent_class); indent
(In reply to comment #19) > Review of attachment 181646 [details] [review]: > > Some trivial comments. > > ::: lib/widgets/ephy-middle-click-button.c > @@ +21,3 @@ > +#include "ephy-middle-click-button.h" > + > +GtkWidget *ephy_middle_click_button_new (); > > You shouldn't need this line, since it's in your .h What I thought but gcc complains here if I dont define the prototype here. > > @@ +34,3 @@ > + gboolean ret; > + > + GdkEvent *event_dup = gdk_event_copy ((GdkEvent *)event); > > What happens if you don't copy the event? The link actions check whether the click was form the middle button when activated. So we cannot modify the real event beforehand. > > @@ +59,3 @@ > + GdkEventButton * event) > +{ > +GtkWidgetClass *widget_class = GTK_WIDGET_CLASS > (ephy_middle_click_button_parent_class); > > indent
Created attachment 181685 [details] [review] Replace gtk_buttom_pressed/released deprecated methods Introduces a GtkButton-derived widget handling middle-click events. Makes EphyHomeAction, EphyGoAction and EphyBookmarkAction use to change the widget state instead relying on the deprecated methods. EphyLink becomes an empty nutshell as its only purpose was to handle the middle-click case. In the future, code will be mutualized there.
Created attachment 181686 [details] [review] Remove deprecated gtk_button_pressed/released usage Remove useless calls to gtk_button_release. Those calls had no effect, since they were following calls to gtk_toggle_button_set_active (FALSE) doing implicit release.
Review of attachment 181686 [details] [review]: Looks ok to me.
Review of attachment 181685 [details] [review]: ::: lib/widgets/ephy-middle-click-button.c @@ +23,3 @@ +G_DEFINE_TYPE (EphyMiddleClickButton, ephy_middle_click_button, GTK_TYPE_BUTTON) + +typedef gboolean(*mouse_event_handler)(GtkWidget*, GdkEventButton*); Probably should get rid of the spaces. @@ +36,3 @@ + ((GdkEventButton*)event_dup)->button = 1; + + ret = parent_method (widget, (GdkEventButton*)event_dup); Isn't it easier to just get rid of the parent_method parameter and do: if (press) chain to parent press else chain to parent release That's 4 lines in total, you are using more doing this function passing thing. @@ +66,3 @@ +ephy_middle_click_button_class_init (EphyMiddleClickButtonClass *class) +{ + GtkWidgetClass *widget_class = GTK_WIDGET_CLASS (class); Indentation...
Created attachment 182153 [details] [review] Remove deprecated Gtk+ symbols in link actions _pressed and _released were used to make the link toolbar buttons appear clicked when pressed with the middle mouse button. This is now the responsability of EphyMiddleClickableButton/ToolButton which fake a button 1 event while preserving the original event. EphyLinkAction is now the single place where links are actually opened, and "do the right thing" (open in new tab), when middle button is used. The action representing the "GoHome" is now a pure EphyLinkAction, HomeAction was removed. "NewTab" and "NewWindow" are now instances of EphyDroppableLinkAction, which can act as drop target for URIs. "GoTo" was converted into a DelegatingLinkAction, which delegates its activation to the location action instead of doing extrawork to access the current text in the the location entry. (a better solution would have been to have a proxy widget to the location action, but egg do not like that from my tests). EphyBookmarkAction was modify to handle middle-click via the common EphyLink code-path.
Created attachment 182186 [details] [review] Remove deprecated Gtk+ symbols in link actions _pressed and _released were used to make the link toolbar buttons appear clicked when pressed with the middle mouse button. This is now the responsability of EphyMiddleClickableButton/ToolButton which fake a button 1 event while preserving the original event. EphyLinkAction is now the single place where links are actually opened, and "do the right thing" (open in new tab), when middle button is used. The action representing the "GoHome" is now a pure EphyLinkAction, HomeAction was removed. "NewTab" and "NewWindow" are now instances of EphyDroppableLinkAction, which can act as drop target for URIs. "GoTo" was converted into a DelegatingLinkAction, which delegates its activation to the location action instead of doing extrawork to access the current text in the the location entry. (a better solution would have been to have a proxy widget to the location action, but egg do not like that from my tests). EphyBookmarkAction was modify to handle middle-click via the common EphyLink code-path.
Created attachment 182293 [details] [review] Remove all direct accesses to the ephy_shell global variable
Created attachment 182294 [details] [review] Do not instanciate a global EphyShell instance. Instead set a field of GtkApplication.
Created attachment 182296 [details] [review] Replace ephy_shell_get_default calls by ephy_application_get_shell
Please forget the 3 last attachement, they are unrelated to this bug.
[Removing GNOME3.0 target as decided in release-team meeting on March 03, 2011. "nice-to-have" categorisation for GNOME3.0; Patch available]
Created attachment 182781 [details] [review] Remove usage of deprecated gtk_button_pressed/_release This patch replaces the use of those deprecated symbols by the introduction of a middle-clickable button widget. Also LinkAction is the only toolbar action doing the link opening. All its subclasses delegates to it.
Review of attachment 182781 [details] [review]: Other than those nitpicks and after a rushed review, I'm a bit confused about why we need an empty middle clickable tool button class, and the exact rationale behind the EphyLink changes. Could you expand a bit on a comment? I'll try to review the patch for real later today. ::: lib/widgets/Makefile.am @@ +23,3 @@ + ephy-middle-clickable-button.c \ + ephy-middle-clickable-tool-button.h \ + ephy-middle-clickable-tool-button.c We probably want to keep this in alphabetical order. ::: lib/widgets/ephy-middle-clickable-button.c @@ +25,3 @@ +static gboolean +ephy_middle_clickable_button_handle_event (GtkWidget * widget, + GdkEventButton * event) Indentation is wrong here. Also, no space after '*'. ::: lib/widgets/ephy-middle-clickable-button.h @@ +22,3 @@ + +#include <glib-object.h> +#include <gtk/gtk.h> I suppose <gtk/gtk.h> is enough? ::: src/Makefile.am @@ +24,3 @@ ephy-link-action.h \ + ephy-delegating-link-action.h \ + ephy-droppable-link-action.h \ This is not in the patch, it seems.
Hi Xan, > Other than those nitpicks and after a rushed review, I'm a bit confused about > why we need an empty middle clickable tool button class, Agreed, would you prefer - an override of GtkAction::create_tool_item in EphyLink, creating a GtkToolItem with a MiddleClickableButton - moving the code from MiddleClickableButton to MiddleClickableToolButton I tend to prefer the first solution. > and the exact > rationale behind the EphyLink changes. Could you expand a bit on a comment? The idea is to have a federated handling of the middle-click in EphyLink, so every subclass dont have to listen to the release event on the button. This makes the behaviour of the subclasses more consistent and remove duplicated code. Also, I'd like to avoid having EphyLink::link_open calls everywhere in the code as I plan to remove the implementation of the EphyLink interface in the actions. Replacing that by a property of type EphyLink in EphyLinkAction on which the open_link request will be issued. Most of the time this will be set to the Window instance or the embed container. The idea here is to avoid the wasteful and hard to read/debug "open-link" event bubbling : action -> toolbar -> window -> embed container -> embed. Instead we would have a direct call to the embed container (or the window). Then EphyLink might be renamed to EphyLinkOpener or something to make its intent clearer. Thanks for your feedback ! > I'll try to review the patch for real later today. > > ::: lib/widgets/Makefile.am > @@ +23,3 @@ > + ephy-middle-clickable-button.c \ > + ephy-middle-clickable-tool-button.h \ > + ephy-middle-clickable-tool-button.c > > We probably want to keep this in alphabetical order. Yup. > ::: lib/widgets/ephy-middle-clickable-button.c > @@ +25,3 @@ > +static gboolean > +ephy_middle_clickable_button_handle_event (GtkWidget * widget, > + GdkEventButton * event) > > Indentation is wrong here. Also, no space after '*'. Will fix. > > ::: lib/widgets/ephy-middle-clickable-button.h > @@ +22,3 @@ > + > +#include <glib-object.h> > +#include <gtk/gtk.h> > > I suppose <gtk/gtk.h> is enough? Yep > > ::: src/Makefile.am > @@ +24,3 @@ > ephy-link-action.h \ > + ephy-delegating-link-action.h \ > + ephy-droppable-link-action.h \ > > This is not in the patch, it seems. Yeah, will fix.
FWIW, there's only one call left in the entire codebase now, the one in EphyLinkAction.
Created attachment 204401 [details] [review] ephy-middle-click.diff OK, so here's a patch to fix this. It takes the EphyMiddleClickable(Tool)Button bits from previous patches, and does the minimal thing needed in EphyLinkAction to make things work. I think this is enough for now, since a lot of code was removed since Alexandre's first patches (still some of his ideas like getting of the ephy_link_open all over the place are good IMHO). It's pretty well documented inside the patch itself, so just read on!
Created attachment 204402 [details] [review] ephy-middle-click.diff Minor nitpicks.
Comment on attachment 204402 [details] [review] ephy-middle-click.diff Pushed to master, commit d5b4f4ba1d31e368e23ddd77774e285a13908622
Closing.