GNOME Bugzilla – Bug 725062
Support Alt+Left for going back
Last modified: 2016-03-31 13:22:07 UTC
It will help accessibility if we allow users to click the 'back' button through Alt-Left shortcut.
Created attachment 270581 [details] [review] Added simple possibility to go back via ALT + Left shortcut. This would be a simple solution. Here are some thoughts and discussion points: - I did leave out the back button in the DISPLAY view. - Should the shortcut close the SearchBar if its active? (E.g. from turning from selection mode to collection view.) - Is this the wrong approach? We could track the history of UI states. Instead of just store previous_ui_state we could store a List or Array of previous states. (in ui.vala) - What about forwarding after going back?
Review of attachment 270581 [details] [review]: ::: src/app-window.vala @@ +204,3 @@ + } else if (event.keyval == Gdk.Key.Left && // ALT + Left -> back + (event.state & default_modifiers) == Gdk.ModifierType.MOD1_MASK) { + switch (App.app.ui_state) { I don't think you need all these switches or set the state even. Just call 'clicked' on appropriate back button on this event in CREDS, DISPLAY, PROPERTIES OR WIZARD states (i-e where button is shown). @@ +205,3 @@ + (event.state & default_modifiers) == Gdk.ModifierType.MOD1_MASK) { + switch (App.app.ui_state) { + case UIState.CREDS: // no back operation on credit window Indeed it is and thats actually the state about which this bug is. :) @@ +208,3 @@ + case UIState.NONE: + case UIState.DISPLAY: + case UIState.SETTINGS: // not yet implemented Did you build this patch? There is no such enum as UIState.SETTINGS so this code shouldn't build. Also there is no need to handle all states here.
(In reply to comment #2) > Review of attachment 270581 [details] [review]: > > ::: src/app-window.vala > @@ +204,3 @@ > + } else if (event.keyval == Gdk.Key.Left && // ALT + Left -> back > + (event.state & default_modifiers) == > Gdk.ModifierType.MOD1_MASK) { > + switch (App.app.ui_state) { > > I don't think you need all these switches or set the state even. Just call > 'clicked' on appropriate back button on this event in CREDS, DISPLAY, > PROPERTIES OR WIZARD states (i-e where button is shown). I will take a look at it. > @@ +205,3 @@ > + (event.state & default_modifiers) == > Gdk.ModifierType.MOD1_MASK) { > + switch (App.app.ui_state) { > + case UIState.CREDS: // no back operation on credit window > > Indeed it is and thats actually the state about which this bug is. :) > > @@ +208,3 @@ > + case UIState.NONE: > + case UIState.DISPLAY: > + case UIState.SETTINGS: // not yet implemented > > Did you build this patch? There is no such enum as UIState.SETTINGS so this > code shouldn't build. Also there is no need to handle all states here. The idea was to list every state there so that if someone changes something about the state and greps for it he'll find it and does not forget to change it here. Yes, I build this patch. I have a branch on top of SHA 7421c2e3136879a14a2450fd01d59720390cb1f5. (Which is a commit from you.) When I check this out I have private enum Boxes.UIState { NONE, COLLECTION, CREDS, DISPLAY, SETTINGS, WIZARD, PROPERTIES } in ui.vala. I thought this would be a planned feature since I found no references (grep -R SETTINGS src/*.vala) except in ui.vala.
Created attachment 270603 [details] [review] Added forward and backward keyboard shortcuts. Added forward support for the wizard. (I think this is the only section where it makes sense.) I also added the invocations of clicked. I think checking the UI state is a good option at the time since I assume that sometimes several buttons are active and one is just lying over the other. (So I can not check via visible nor sensitive. I tried that and it was bad. That is the reason for my assumption.) At first I had a few problems with some debug messages (assertion '_tmp4_ != NULL' failed, line 219 (boxes_wizard_create_co): should not be reached) but I fixed this. You may want to take a look if you can provoke some messages with this patch and if so I would be glad to try to reproduce it. (By pressing Alt + Left or + Right.) If you have any hints about how to get some valuable information from these kind of messages (besides grepping _tmp4_ != NULL in the c files) I would appreciate it if you could tell me.
(In reply to comment #3) > (In reply to comment #2) > > Review of attachment 270581 [details] [review] [details]: > > > > ::: src/app-window.vala > > @@ +204,3 @@ > > + } else if (event.keyval == Gdk.Key.Left && // ALT + Left -> back > > + (event.state & default_modifiers) == > > Gdk.ModifierType.MOD1_MASK) { > > + switch (App.app.ui_state) { > > > > I don't think you need all these switches or set the state even. Just call > > 'clicked' on appropriate back button on this event in CREDS, DISPLAY, > > PROPERTIES OR WIZARD states (i-e where button is shown). > I will take a look at it. > > > @@ +205,3 @@ > > + (event.state & default_modifiers) == > > Gdk.ModifierType.MOD1_MASK) { > > + switch (App.app.ui_state) { > > + case UIState.CREDS: // no back operation on credit window > > > > Indeed it is and thats actually the state about which this bug is. :) > > > > @@ +208,3 @@ > > + case UIState.NONE: > > + case UIState.DISPLAY: > > + case UIState.SETTINGS: // not yet implemented > > > > Did you build this patch? There is no such enum as UIState.SETTINGS so this > > code shouldn't build. Also there is no need to handle all states here. > The idea was to list every state there so that if someone changes something > about the state and greps for it he'll find it and does not forget to change it > here. That would mean a lot of redundant code as you'll want to do this in every place where we do something based on the state. If they change something about a state that is not listed here, that would simply mean that the change doesn't have anything to do with this code and hence there is nothing to forget. > Yes, I build this patch. I have a branch on top of SHA > 7421c2e3136879a14a2450fd01d59720390cb1f5. (Which is a commit from you.) When I > check this out I have > > private enum Boxes.UIState { > NONE, > COLLECTION, > CREDS, > DISPLAY, > SETTINGS, > WIZARD, > PROPERTIES > } > > in ui.vala. I thought this would be a planned feature since I found no > references (grep -R SETTINGS src/*.vala) except in ui.vala. git blame can tell you why and when it was added if you are interested but I think it was supposed to be the same as PROPERTIES but author forgot about it when adding the actual state so introduced a new enum instead. Feel free to provide a separate patch that removes this redundant enum.
Review of attachment 270603 [details] [review]: Looking promising otherwise. As in the other bug, you gotta follow these guidelines for commit logs: https://wiki.gnome.org/Git/CommitMessages . Just a friendly advice: to keep the summary line short, consider writing "&" instead of "and". ::: src/app-window.vala @@ +204,3 @@ + } else if (event.keyval == Gdk.Key.Left && // ALT + Left -> back + (event.state & default_modifiers) == Gdk.ModifierType.MOD1_MASK) { + // find appropriate back button find -> click @@ +226,3 @@ + } + return true; + } else if (event.keyval == Gdk.Key.Right && // ALT + Right -> forward This is not needed. We already have kbd shortcut for those using mnemonics. ::: src/topbar.vala @@ +27,3 @@ + public Gtk.Button cancel_btn; + [GtkChild] + public Gtk.Button props_back_btn; Perhaps we should a 'click_back_button' method here so we don't need to expose the private variables.
(In reply to comment #6) > Review of attachment 270603 [details] [review]: > > Looking promising otherwise. > > As in the other bug, you gotta follow these guidelines for commit logs: > https://wiki.gnome.org/Git/CommitMessages . Just a friendly advice: to keep the > summary line short, consider writing "&" instead of "and". No problem. I'll try to stick to that in the future. > ::: src/app-window.vala > @@ +204,3 @@ > + } else if (event.keyval == Gdk.Key.Left && // ALT + Left -> back > + (event.state & default_modifiers) == > Gdk.ModifierType.MOD1_MASK) { > + // find appropriate back button > > find -> click Ok. > @@ +226,3 @@ > + } > + return true; > + } else if (event.keyval == Gdk.Key.Right && // ALT + Right -> forward > > This is not needed. We already have kbd shortcut for those using mnemonics. I think it would be more consistent if the semantic forward and backward (wich also has a mnemonic in the wizard) would be reachable with the same shortcut everywhere in the application. I personally as a user would appreciate if Alt + Left / Right is available for all semantic back and forward possibilities. I also thought about rethinking the position of the cancel and back buttons. Selection mode: - cancel button on the right Wizard: - cancel on the left, back on the right Properties, Display: - back on the left I can only speak for me, but I find this to be rather inconsistent and confusing from user perspective. (I agree that for the individual modes this makes totally sense.) We could look at other gnome apps for ideas about unifying the back button. If you had this discussion before, just say it. > ::: src/topbar.vala > @@ +27,3 @@ > + public Gtk.Button cancel_btn; > + [GtkChild] > + public Gtk.Button props_back_btn; > > Perhaps we should a 'click_back_button' method here so we don't need to expose > the private variables. I think this is a good idea. I also thought about generalizing the on_key_pressed function yesterday so that we have a hashtable which has the different Key/Modifier combinations as key and the function to be invoked as value.
(In reply to comment #7) > (In reply to comment #6) > > Review of attachment 270603 [details] [review] [details]: > > > > Looking promising otherwise. > > > > As in the other bug, you gotta follow these guidelines for commit logs: > > https://wiki.gnome.org/Git/CommitMessages . Just a friendly advice: to keep the > > summary line short, consider writing "&" instead of "and". > No problem. I'll try to stick to that in the future. > > > ::: src/app-window.vala > > @@ +204,3 @@ > > + } else if (event.keyval == Gdk.Key.Left && // ALT + Left -> back > > + (event.state & default_modifiers) == > > Gdk.ModifierType.MOD1_MASK) { > > + // find appropriate back button > > > > find -> click > Ok. > > > @@ +226,3 @@ > > + } > > + return true; > > + } else if (event.keyval == Gdk.Key.Right && // ALT + Right -> forward > > > > This is not needed. We already have kbd shortcut for those using mnemonics. > I think it would be more consistent if the semantic forward and backward (wich > also has a mnemonic in the wizard) would be reachable with the same shortcut > everywhere in the application. I personally as a user would appreciate if Alt + > Left / Right is available for all semantic back and forward possibilities. Hmm.. You make a good case. OK, lets add this too then. :) Having said that, I think this is a separate logical change so it should go in a separate patch. > I also thought about rethinking the position of the cancel and back buttons. > Selection mode: > - cancel button on the right > Wizard: > - cancel on the left, back on the right > Properties, Display: > - back on the left 'Back' in the properties is the same as 'Cancel' so there is no inconsistency there. You might have some point about other views though. You might want to talk to jimmac and/or mccann on IRC channel about that. Do comment here about results of that discussion though. > > ::: src/topbar.vala > > @@ +27,3 @@ > > + public Gtk.Button cancel_btn; > > + [GtkChild] > > + public Gtk.Button props_back_btn; > > > > Perhaps we should a 'click_back_button' method here so we don't need to expose > > the private variables. > I think this is a good idea. I also thought about generalizing the > on_key_pressed function yesterday so that we have a hashtable which has the > different Key/Modifier combinations as key and the function to be invoked as > value. Why would you need this hashtable? Also if you need something like this, there is GAction and GActionMap API available.
Created attachment 271089 [details] [review] * I encapsulated the back button click to a function in topbar. * I also removed the backward and forward possibility in the wizard. I'll post an extra patch for that. Concerning the hashtable: I thought of a dictionary-like structure where the key is the combination of a keyval and the modifier state. The value would be a delegate that will be executed if the key matches the real state. on_key_pressed would just iterate through this dictionary and invoke the functions and everyone could register a shortcut with AppWindow.register_shortcut (...) or so. It was just an idea for more encapsulation. What do you think about this?
Created attachment 271091 [details] [review] added wizard forward & backward shortcut Added support for Alt + Left and Alt + Right shortcut in the wizard.
Review of attachment 271089 [details] [review]: Almost there, just some comments and a nitpick on commit log: Convention is to use present form of verb but I can "fix" that when merging the change. ::: src/topbar.vala @@ +58,3 @@ + /** + * Clicks the appropriate back button dependent on the ui state. * dependent -> depending * Unless you intend to use this comment for generated docs, you don't need to waste space on C-style comments for a single line of text. Just use C++ style // comments. @@ +70,3 @@ + break; + default: + if (App.window.topbar.back_btn.visible) That is wrong. Button being 'visible' just means that its visible when its container (and its container and so on..) is visible. You should just check for appropriate states here instead.
Review of attachment 271091 [details] [review]: this one is almost ready too but same nitpick about commit log. ::: src/topbar.vala @@ +70,3 @@ break; + case UIState.WIZARD: + if (App.window.wizard.page > 0) { * You want to use the enum for the page number here * {} are redundant for single lines of blocks @@ +88,3 @@ + public void click_forward_button () { + if (App.app.ui_state == UIState.WIZARD) { + if (wizard_continue_btn.visible) i think check of visibility makes sense in this case.
Created attachment 271250 [details] [review] app-window: add support for Alt+Left shortcut As for the visible I just refer to our discussion.
Created attachment 271251 [details] [review] Included review comments.
Created attachment 271252 [details] [review] I forgot transforming a comment.
Review of attachment 271250 [details] [review]: As usual, nitpicks on commit log: it says which possibility is not covered but it doesn't say which ones are covered by the patch. Also 'support for' part is redundant. Oh and first letter (unless its identifier) is supposed to be Capitalized: app-window: Add Alt-Left shortcut. ::: src/topbar.vala @@ +66,3 @@ + break; + default: + if (App.window.topbar.back_btn.visible) I still think you should just check the UI state (i-e more case statements here) rather than checking for visibility. Also visible -> is_visible ?
Review of attachment 271252 [details] [review]: Similar nitpicks about commit log: * 'support for' part is redundant. * shortcut -> shortcuts * add -> Add ::: src/app-window.vala @@ -188,2 @@ } - why are you removing this line? ::: src/topbar.vala @@ +85,3 @@ + public void click_forward_button () { + if (App.app.ui_state == UIState.WIZARD) { + if (wizard_continue_btn.visible) I know I previous said that check of 'visible' is fine here and it still is but I think to be safe we should check for 'is_visible' instead (e.g if we later put the button in box, this logic will fail). @@ +87,3 @@ + if (wizard_continue_btn.visible) + wizard_continue_btn.clicked (); + else if (wizard_create_btn.visible) same here.
Created attachment 271313 [details] [review] app-window: Add Alt+Left shortcut Alt+Left shortcut works now for * selection mode * CREDS * PROPERTIES
Created attachment 271315 [details] [review] Add wizard forward & backward shortcut Add Alt + Left and Alt + Right shortcuts in the wizard.
Review of attachment 271313 [details] [review]: Looks good. I hope you tested this version in all these 3 UI states/modes?
Review of attachment 271315 [details] [review]: Same comment about testing. I hope you tested this (version of) patch against all wizard pages?
Review of attachment 271315 [details] [review]: and tested in both directions?
Before each upload I did these tests. Is something gone wrong? And I just retested everything for you ;)
(In reply to comment #23) > Before each upload I did these tests. Is something gone wrong? No, I was just making sure. Sorry, wrong language. I meant to say "I assume" rather than "I hope". :) > And I just retested everything for you ;) Cool. Your patches don't apply on current git master though. You gotta rebase and test if there is any conflicts during rebase.
Created attachment 271539 [details] [review] app-window: Add Alt+Left shortcut Alt+Left shortcut works now for * selection mode * CREDS * PROPERTIES
Created attachment 271540 [details] [review] Add wizard forward & backward shortcut Add Alt + Left and Alt + Right shortcuts in the wizard.
Created attachment 271544 [details] [review] app-window: Add Alt+Left shortcut This version doesn't click 'Cancel' buttons as per discussion with kittykat, amigadave and joanie. Made some minor changes to the patch as well.
Created attachment 271545 [details] [review] app-window: Add forward & back shortcuts in wizard This version doesn't click 'Cancel' buttons as per discussion with kittykat, amigadave and joanie. Made some minor changes to the patch as well.
Created attachment 271546 [details] [review] app-window: Add Alt+Left shortcut Attached wrong verson previously. :)
Created attachment 271547 [details] [review] app-window: Add forward & back shortcuts in wizard Wrong version for this patch too.
Created attachment 271550 [details] [review] app-window: Add forward & back shortcuts in wizard Don't press 'Create' button either on review page for the same logic as not clicking the cancel buttons.
Attachment 271546 [details] pushed as 7508511 - app-window: Add Alt+Left shortcut Attachment 271550 [details] pushed as 469ff7e - app-window: Add forward & back shortcuts in wizard
Alt + Left does not work for properties anymore. This is a partial regression of this bug due to commit 8c8ea34. Patch will follow shortly.
Created attachment 279104 [details] [review] topbar: Add Alt+Left shortcut for properties This fixes partial regression of bug#725062 due to commit 8c8ea34.
(In reply to comment #34) > Created an attachment (id=279104) [details] [review] > topbar: Add Alt+Left shortcut for properties > > This fixes partial regression of bug#725062 due to commit 8c8ea34. Would be nice to have some indication of what regression you are talking about in the log.