{"thread":{"id":"51876","subject":"git-gui: duplicated key binds?","startedAt":"2019-09-18T11:27:27Z","lastAt":"2019-09-18T20:46:59Z","messageCount":3,"participants":["Birger Skogeng Pedersen","Pratyush Yadav"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"382547","messageId":"CAGr--=KC=OPMJZB883MWys=J068cdZNHL=eOYK81fNBEv9MhvA@mail.gmail.com","threadId":"51876","inReplyTo":null,"subject":"git-gui: duplicated key binds?","fromName":"Birger Skogeng Pedersen","fromEmail":"birger.sp@gmail.com","sentAt":"2019-09-18T11:24:40Z","receivedAt":"2019-09-18T11:27:27Z","isPatch":false,"sender":{"key":"birger.sp@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5260237?v=4"},"body":"Hi,\n\n\nIt looks to me like there are a lot of key binds duplicated in the\ngit-gui source.\n\nFor instance, Ctrl/Cmd+Enter are bound in two lines:\nbind $ui_comm <$M1B-Key-Return> {do_commit;break}\nand\nbind .   <$M1B-Key-Return> do_commit\n\nI guess the first one is specified to work in the commit message\nwidget. The second one is for all widgets(?).\n\nAlso, I see that some binds are with the \"all\" tag, while most are\nwith just a dot as tag.\n\nIs this a mistake (aka something I could write a patch for)? Or am I\njust missing something?\n\n[0] https://github.com/prati0100/git-gui/blob/master/git-gui.sh#L3835-L3928\n\n\nI propose:\n- replace \"bind .   \" with \"bind all \"\n- remove duplicated bind entries, if a key is bound to \"all\" then it\nshouldn't be bound with another tag\n\n\nBirger\n"},{"id":"382557","messageId":"CAGr--=L8iRCW2zKLEN73rpqfK+xB_0RsbhUGuCUVA56Qo2BtQQ@mail.gmail.com","threadId":"51876","inReplyTo":"CAGr--=KC=OPMJZB883MWys=J068cdZNHL=eOYK81fNBEv9MhvA@mail.gmail.com","subject":"Re: git-gui: duplicated key binds?","fromName":"Birger Skogeng Pedersen","fromEmail":"birger.sp@gmail.com","sentAt":"2019-09-18T16:17:02Z","receivedAt":"2019-09-18T16:17:19Z","isPatch":false,"sender":{"key":"birger.sp@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5260237?v=4"},"body":"Doing some more experimenting with these, I realize I was completely\nwrong about them. Please disregard my previous email :-)\n\nBirger\n"},{"id":"382573","messageId":"20190918204653.jxzirmmmauf52wn7@yadavpratyush.com","threadId":"51876","inReplyTo":"CAGr--=KC=OPMJZB883MWys=J068cdZNHL=eOYK81fNBEv9MhvA@mail.gmail.com","subject":"Re: git-gui: duplicated key binds?","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-09-18T20:46:53Z","receivedAt":"2019-09-18T20:46:59Z","isPatch":false,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 18/09/19 01:24PM, Birger Skogeng Pedersen wrote:\n> Hi,\n> \n> \n> It looks to me like there are a lot of key binds duplicated in the\n> git-gui source.\n> \n> For instance, Ctrl/Cmd+Enter are bound in two lines:\n> bind $ui_comm <$M1B-Key-Return> {do_commit;break}\n> and\n> bind .   <$M1B-Key-Return> do_commit\n> \n> I guess the first one is specified to work in the commit message\n> widget. The second one is for all widgets(?).\n\nYes, but in this case they are doing the same thing.\n\nActually, this is a bit of a mess that shouldn't exist in the first \nplace.\n\nAccording to the bind manual page [0], \n\n  It is possible for several bindings to match a given X event. If the \n  bindings are associated with different tag's, then each of the \n  bindings will be executed, in order. By default, a binding for the \n  widget will be executed first, followed by a class binding, a binding \n  for its toplevel, and an all binding.\n\n  The continue and break commands may be used inside a binding script to \n  control the processing of matching scripts. If continue is invoked, \n  then the current binding script is terminated but Tk will continue \n  processing binding scripts associated with other tag's. If the break \n  command is invoked within a binding script, then that script \n  terminates and no other scripts will be invoked for the event.\n\nWhat this essentially means is that first the binding for $ui_comm is \nexecuted, and then the binding for \".\". But since the binding for \n$ui_comm has a break, only the first one is executed, and not the \nsecond. This is what happens when the focus is in $ui_comm.\n\nIf the focus is elsewhere, the second binding (the one for \".\") is \nexecuted.\n\nMy point is, a break for something so simple should not be there in the \nfirst place. If you do want Ctrl+Enter to make a commit from anywhere, \ndon't specify the binding for it in $ui_comm. So in this case IMO the \nbreak is a simple hack to prevent do_commit from being executed twice.\n\nNow on the more subjective side, is it really a good idea to allow \nCtrl+Enter to commit from anywhere? IMHO it should only do that from the \ncommit message buffer.\n \n> Also, I see that some binds are with the \"all\" tag, while most are\n> with just a dot as tag.\n\nAh, more of this bind mess. Luckily for us only places where \"all\" is \nused is for binds that quit the application. So it shouldn't be too much  \na problem.\n\nNow for the dirty details:\n\nExcerpt from the bind manual page:\n\n  - If the tag is the name of a toplevel window the binding applies to \n    the toplevel window and all its internal windows.\n\n  - If tag has the value all, the binding applies to all windows in the \n    application.\n\nAs far as I understand, binds for \".\" are executed for every child \nwindow of the toplevel window \".\" (aka the main widow). Examples for \nchildren of \".\" include the diff viewer, the commit message buffer, etc. \n\nNow this is where my understanding of this gets shaky. If you bind \nCtrl-W to quit the window, then in that case every child window of \".\" \nshould have the same behaviour in that binding. But that is not the case \nfor, say, the \"Options\" dialog.\n\nThe Options dialog is a toplevel window itself (all dialogs are), but it \nis called \".options\", so one would assume it should be a subwindow for \n\".\". That does not appear to be the case.\n\nBut we do want Ctrl-W to work on the options dialog too (and the tools \ndialog, and all other dialogs). So, the binds for Ctrl-W should stay \nbound to \"all\". Same argument can be applied to Ctrl-Q.\n \n> Is this a mistake (aka something I could write a patch for)? Or am I\n> just missing something?\n\nI haven't got the time right now to look at all the duplicate bindings, \nbut at least for the Ctrl-Enter one, it is debatable whether it is a \nbug.\n\nIf people think having this binding active for only the commit message \nbuffer, then it is a bug, and the binding for \".\" should be removed. I \nam one of those people.\n\nBut if people think that Ctrl-Enter should trigger a commit _anywhere_ \nin the UI, then it is fine as it is.\n\nI will try to look at other duplicates tomorrow.\n \n> I propose:\n> - replace \"bind .   \" with \"bind all \"\n\nLike I mentioned above, \"bind .\" and \"bind all\" are not the same thing. \nIn our case, the main problem is with dialogs. They have a different \ntoplevel window than the main program. So if we do replace \"bind .\" with \n\"bind all\", then the bindings for things like commit, rescan will also \nmove over to those dialogs. I don't think it is a good idea to do so.\n\n> - remove duplicated bind entries, if a key is bound to \"all\" then it\n> shouldn't be bound with another tag\n\nFrom my quick scan of the search results for \"bind\", the only keys bound \nto \"all\" are \"Q\" and \"W\". \"Q\" quits the entire application, and \"W\" \ncloses the current toplevel window. Both should stay the way they are.\n\nAs for duplicated bindings for \".\", and other widgets, that is something \nI haven't looked into too well. I will get to it by tomorrow.\n\n[0] https://www.tcl.tk/man/tcl8.4/TkCmd/bind.htm\n\n-- \nRegards,\nPratyush Yadav\n"}]}