GNOME Bugzilla – Bug 668702
Bring back Alt+# accelerators.
Last modified: 2012-01-31 18:34:53 UTC
Switching tabs can be really frustrating without accelerators. I understand Web wants to remove the visual clutter of tabs, but the Alt+# accelerators would still be useful as a power-user feature. Any objection? I would like to bring this back using the relevant parts removed in: commit fdf7054db6d84b554b3ea52ed60d065f211765e0 Author: Xan Lopez <xan@igalia.com> Date: Mon Dec 5 00:21:40 2011 +0100 ephy-window: remove the tabs menu It is not present in the new design. The Alt+<num> accelerators are dying with it, but it will be pretty easy to bring them back if needed.
Sure, go ahead.
Created attachment 206238 [details] [review] ephy-window: restore alt+# accels to switch tabs Based on code from gedit-window.c, create a GtkAction for each tab from 1 to 10.
Review of attachment 206238 [details] [review]: I don't think we want a GtkRadioAction or any of the idle stuff here. This won't have any UI, you just want 10 pure accels. Or am I missing something?
Created attachment 206474 [details] [review] ephy-window: restore alt+# accels to switch tabs Create 10 fixed GtkActions that react to the alt+# accelerators. Based on code from gedit-window.c. = Much simpler approach following comments on jabber. My comments: - do I /need/ something like destroy_tab_accels()? Other groups don't do it. - atoi() is safe enough, right? - should I move this to setup_ui_manager()? - GtkNotebook does nothing if the page requested doesn't exist. What do you think now?
Review of attachment 206474 [details] [review]: ::: src/ephy-window.c @@ +1321,3 @@ + /* Tab accels */ + action_group = gtk_action_group_new ("TabAccelsActions"); + gtk_action_group_set_translation_domain (action_group, NULL); Isn't NULL the default? @@ +2962,3 @@ + + gtk_notebook_set_current_page (priv->notebook, tab_number); +} I think it's much safer to have a small method that updates the sensitivity of the actions when you add/remove a tab, based on the number of tabs. Relying on GtkNotebook silently doing nothing seems a bit adventurous. @@ +2973,3 @@ + id = gtk_ui_manager_new_merge_id (priv->manager); + + for (i = 0; i < 10; i++) a #define instead of just '10', since yo use it in more than one place. @@ +3018,3 @@ + + g_list_free (actions); +} I think since you add the action group to the UI manager you probably don't need to do this. Either that or we are leaking everything in EphyWindow currently...
Created attachment 206535 [details] [review] ephy-window: restore alt+# accels to switch tabs Create 10 fixed GtkActions that react to the alt+# accelerators. Based on code from gedit-window.c. = Updated for comments.
Created attachment 206536 [details] [review] ephy-window: restore alt+# accels to switch tabs Create 10 fixed GtkActions that react to the alt+# accelerators. Based on code from gedit-window.c. = Updated yet again with Xan comments.
Review of attachment 206536 [details] [review]: Looks good to me, two nitpicks. ::: src/ephy-window.c @@ +2525,3 @@ + actions = gtk_action_group_list_actions (priv->tab_accels_action_group); + pages = gtk_notebook_get_n_pages (priv->notebook); + i = 0; Initialize it when you declare it? @@ +2532,3 @@ + gtk_action_set_sensitive (action, (i < pages)); + i++; + } I like an empty line here, silly detail.
Created attachment 206538 [details] [review] ephy-window: restore alt+# accels to switch tabs Create 10 fixed GtkActions that react to the alt+# accelerators. Based on code from gedit-window.c. = Nitpicks solved!
Attachment 206538 [details] pushed as 9ead439 - ephy-window: restore alt+# accels to switch tabs