| Message ID | 20190822220107.4153-1-me@yadavpratyush.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | git-gui: Add ability to revert selected hunks and lines | expand |
Pratyush Yadav <me@yadavpratyush.com> writes: > This series adds the ability to revert selected lines and hunks in > git-gui. Partially based on the patch by Bert Wesarg [0]. > > The commits can be found in the topic branch 'py/revert-hunks-lines' > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines > > Once reviewed, pull the commits from > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and > apply them locally, whichever you prefer. Let's see how we can work together by you playing the role of git-gui maintainer and the others on the list (including me) playing the role of reviewer and contributor. So I may keep an eye on the discussion on this thread, I may even comment on them myself, but you'll be the one waiting for the discussion to settle, adjusting the patches in response to reviews, etc. and making the final decision when/if the topic is done, at which time you'd be telling me to pull from you. > Pratyush Yadav (4): > git-gui: Move revert confirmation dialog creation to separate function > git-gui: Add option to disable the revert confirmation prompt > git-gui: Add the ability to revert selected lines > git-gui: Add the ability to revert selected hunk "Move" and "Add" after "git-gui:" would better be downcased to be more in line with the others in "git shortlog --no-merges"; I also think "allow doing X" is shorter and better way to say "add the ability to do X". If I am reading the first patch correctly, we already ask for confirmation before reverting local changes, and the steps 3 and 4 are about allowing partial reversion in addition to the wholesale reversion, right? An earlier objection from j6t sounded like we require users to respond to an extra dialog after this series, but that does not look like the case. Instead, step 2 adds a new feature to allow those to opt-out of the existing dialog (which may be reused to squelch the dialog to protect features added in steps 3 and 4). Am I reading the series correctly? Thanks. > > git-gui.sh | 40 +++++++++++++++++++++++++++++-- > lib/diff.tcl | 65 ++++++++++++++++++++++++++++++++++++++++++-------- > lib/index.tcl | 31 ++++++++++++++++-------- > lib/option.tcl | 1 + > 4 files changed, 115 insertions(+), 22 deletions(-) > > -- > 2.21.0
On 22/08/19 03:34PM, Junio C Hamano wrote: > Pratyush Yadav <me@yadavpratyush.com> writes: > > > This series adds the ability to revert selected lines and hunks in > > git-gui. Partially based on the patch by Bert Wesarg [0]. > > > > The commits can be found in the topic branch 'py/revert-hunks-lines' > > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines > > > > Once reviewed, pull the commits from > > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till > > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and > > apply them locally, whichever you prefer. > > Let's see how we can work together by you playing the role of > git-gui maintainer and the others on the list (including me) playing > the role of reviewer and contributor. So I may keep an eye on the > discussion on this thread, I may even comment on them myself, but > you'll be the one waiting for the discussion to settle, adjusting > the patches in response to reviews, etc. and making the final > decision when/if the topic is done, at which time you'd be telling > me to pull from you. > > > Pratyush Yadav (4): > > git-gui: Move revert confirmation dialog creation to separate function > > git-gui: Add option to disable the revert confirmation prompt > > git-gui: Add the ability to revert selected lines > > git-gui: Add the ability to revert selected hunk > > "Move" and "Add" after "git-gui:" would better be downcased to be > more in line with the others in "git shortlog --no-merges"; I also > think "allow doing X" is shorter and better way to say "add the > ability to do X". Will fix. Thanks. > If I am reading the first patch correctly, we already ask for > confirmation before reverting local changes, and the steps 3 and 4 > are about allowing partial reversion in addition to the wholesale > reversion, right? Yes. The ability to revert whole files already exists. This revert always asks for confirmation. Steps 3 and 4 allow for partial reverts. Step 2 allows the user to disable the confirmation dialog for both whole-file reverts and for partial reverts. > An earlier objection from j6t sounded like we > require users to respond to an extra dialog after this series, but > that does not look like the case. Instead, step 2 adds a new > feature to allow those to opt-out of the existing dialog (which may > be reused to squelch the dialog to protect features added in steps 3 > and 4). Am I reading the series correctly? Yes. The users always responded to an extra dialog for whole file reverts even before these changes. j6t was running a fork of git-gui which had the ability for partial reverts, and his fork did not ask for confirmation for partial reverts. Always asking for confirmation disrupts his workflow, so I added an option to disable it. It disables the dialog for partial reverts (which j6t cares about), and also for whole file reverts, which I added to maintain consistency. > > Thanks. > > > > > git-gui.sh | 40 +++++++++++++++++++++++++++++-- > > lib/diff.tcl | 65 ++++++++++++++++++++++++++++++++++++++++++-------- > > lib/index.tcl | 31 ++++++++++++++++-------- > > lib/option.tcl | 1 + > > 4 files changed, 115 insertions(+), 22 deletions(-) > > > > -- > > 2.21.0
On Fri, Aug 23, 2019 at 12:51 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > On 22/08/19 03:34PM, Junio C Hamano wrote: > > Pratyush Yadav <me@yadavpratyush.com> writes: > > > > > This series adds the ability to revert selected lines and hunks in > > > git-gui. Partially based on the patch by Bert Wesarg [0]. > > > > > > The commits can be found in the topic branch 'py/revert-hunks-lines' > > > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines > > > > > > Once reviewed, pull the commits from > > > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till > > > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and > > > apply them locally, whichever you prefer. > > > > Let's see how we can work together by you playing the role of > > git-gui maintainer and the others on the list (including me) playing > > the role of reviewer and contributor. So I may keep an eye on the > > discussion on this thread, I may even comment on them myself, but > > you'll be the one waiting for the discussion to settle, adjusting > > the patches in response to reviews, etc. and making the final > > decision when/if the topic is done, at which time you'd be telling > > me to pull from you. > > > > > Pratyush Yadav (4): > > > git-gui: Move revert confirmation dialog creation to separate function > > > git-gui: Add option to disable the revert confirmation prompt > > > git-gui: Add the ability to revert selected lines > > > git-gui: Add the ability to revert selected hunk > > > > "Move" and "Add" after "git-gui:" would better be downcased to be > > more in line with the others in "git shortlog --no-merges"; I also > > think "allow doing X" is shorter and better way to say "add the > > ability to do X". > > Will fix. Thanks. > > > If I am reading the first patch correctly, we already ask for > > confirmation before reverting local changes, and the steps 3 and 4 > > are about allowing partial reversion in addition to the wholesale > > reversion, right? > > Yes. The ability to revert whole files already exists. This revert > always asks for confirmation. Steps 3 and 4 allow for partial reverts. > Step 2 allows the user to disable the confirmation dialog for both > whole-file reverts and for partial reverts. > > > An earlier objection from j6t sounded like we > > require users to respond to an extra dialog after this series, but > > that does not look like the case. Instead, step 2 adds a new > > feature to allow those to opt-out of the existing dialog (which may > > be reused to squelch the dialog to protect features added in steps 3 > > and 4). Am I reading the series correctly? > > Yes. The users always responded to an extra dialog for whole file > reverts even before these changes. j6t was running a fork of git-gui > which had the ability for partial reverts, and his fork did not ask for > confirmation for partial reverts. Always asking for confirmation > disrupts his workflow, so I added an option to disable it. It disables > the dialog for partial reverts (which j6t cares about), and also for > whole file reverts, which I added to maintain consistency. as I'm the one who use this feature for more than 7 years, I can only object to this. I'm happy to have the confirmation dialog for the whole-file revert (the current state) but having the dialog also for partial revert would be too disruptive. And disabling both at the same time would not be an option for me. The thing is, that the partial revert "just don't happen by accident". Here are the minimum user actions needed to get to this dialog: 1. whole-file revert - do a Ctrl+J, more or less anywhere in the GUI 2. hunk revert/revert one unselected line - right click anywhere in the diff pane (thats around 60% of the window area) - move the mouse pointer down 3/4 menu items - click this menu item 3. partially revert selected lines - select some content in the diff pane by starting by pressing and holding a left click - end the selection by releasing the left click - move the mouse pointer down 3/4 menu items - click this menu item Thats always at least 2 user actions more than the whole-file revert. Thus this cannot happen by accident quite easily in comparison to the whole-file revert. And thats the reason why this dialog exists, from my point of view. I can see the need to disable the dialog for the whole-file revert, and IIRC that was also requested a long time ago on this list. But I don't see a reason to have this dialog also for the partial reverts as a safety measure. Best, Bert > > -- > Regards, > Pratyush Yadav
Bert Wesarg <bert.wesarg@googlemail.com> writes: > The thing is, that the partial revert "just don't happen by accident". > Here are the minimum user actions needed to get to this dialog: > > 1. whole-file revert > > - do a Ctrl+J, more or less anywhere in the GUI > > 2. hunk revert/revert one unselected line > > - right click anywhere in the diff pane (thats around 60% of the window area) > - move the mouse pointer down 3/4 menu items > - click this menu item > > 3. partially revert selected lines > > - select some content in the diff pane by starting by pressing and > holding a left click > - end the selection by releasing the left click > - move the mouse pointer down 3/4 menu items > - click this menu item > > Thats always at least 2 user actions more than the whole-file revert. > Thus this cannot happen by accident quite easily in comparison to the > whole-file revert. And thats the reason why this dialog exists, from > my point of view. > > I can see the need to disable the dialog for the whole-file revert, > and IIRC that was also requested a long time ago on this list. But I > don't see a reason to have this dialog also for the partial reverts as > a safety measure. Thanks for walking us readers through your thought process.
On 23/08/19 08:04AM, Bert Wesarg wrote: > On Fri, Aug 23, 2019 at 12:51 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > On 22/08/19 03:34PM, Junio C Hamano wrote: [...] > as I'm the one who use this feature for more than 7 years, I can only > object to this. I'm happy to have the confirmation dialog for the > whole-file revert (the current state) but having the dialog also for > partial revert would be too disruptive. And disabling both at the same > time would not be an option for me. > > The thing is, that the partial revert "just don't happen by accident". > Here are the minimum user actions needed to get to this dialog: > > 1. whole-file revert > > - do a Ctrl+J, more or less anywhere in the GUI Hmm, have to agree with you on this. It is kind of easy to trigger. But read below why I think partial reverts are just as easy to trigger. > 2. hunk revert/revert one unselected line > > - right click anywhere in the diff pane (thats around 60% of the window area) > - move the mouse pointer down 3/4 menu items > - click this menu item But what if you wanted to click "Stage hunk", and instead click "Revert hunk" instead. This is what I mean by "accidentally". This is even more a risk in the current layout of the buttons, which are in the order: Stage Hunk Revert Hunk Stage Lines Revert Lines In this layout, if you wanted to click Stage, you might end up clicking Revert instead, losing part of your work. Even if we switch up the layout a bit, I feel like the potential of "mis-aiming" your mouse is there, but it can certainly be improved. > 3. partially revert selected lines > > - select some content in the diff pane by starting by pressing and > holding a left click > - end the selection by releasing the left click > - move the mouse pointer down 3/4 menu items > - click this menu item > > Thats always at least 2 user actions more than the whole-file revert. > Thus this cannot happen by accident quite easily in comparison to the > whole-file revert. And thats the reason why this dialog exists, from > my point of view. > > I can see the need to disable the dialog for the whole-file revert, > and IIRC that was also requested a long time ago on this list. But I > don't see a reason to have this dialog also for the partial reverts as > a safety measure. I do (not too strongly, but I do), as I explained why above. It shouldn't be too difficult to have separate knobs for whole-file and partial reverts, but they will add two config options. Not necessarily a bad thing, I just thought the people who wanted to disable partial reverts would also want to disable whole-file reverts. But I have another suggestion in mind. I'll reply to one of the other emails in this thread about it. Please read it there, I'd rather not type it twice.
On Fri, Aug 23, 2019 at 03:31:03AM +0530, Pratyush Yadav wrote: > Hi, > > This series adds the ability to revert selected lines and hunks in > git-gui. Partially based on the patch by Bert Wesarg [0]. > > The commits can be found in the topic branch 'py/revert-hunks-lines' > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines > > Once reviewed, pull the commits from > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and > apply them locally, whichever you prefer. > > Changes in v2: > - Add an option to disable the revert confirmation prompt as suggested > by Johannes Sixt. > - Base the patches on Pat's git-gui tree instead of git.git. We've had these features for years in git-cola. Please copy our keyboard shortcuts. IMO we should not re-invent the user interactions. http://git-cola.github.io/share/doc/git-cola/hotkeys.html Ctrl-u is our revert-unstaged-edits hotkeys. "s" is for staging/unstaging (or Ctrl-s if the file list is focused). The same hotkey is used for operating at the line level. If no lines are selected, the hunk surrounding the current cursor position is used. Please make keyboard interaction a first-class design consideration. I have a very strong opinion about the confirmation dialog, so I'll just mention that here since Hannes is on this thread. In cola we do have a confirmation dialog, and I strongly believe this is the correct behavior because it's an operation that drops data that cannot be recovered. In the other thread, it was mentioned that this dialog would be a nuisance. Perhaps that is true -- for the dialog that may have been implemented in this series (I haven't run it to verify). Let's dive into that concern. In git-cola we have a confirmation dialog and it is by no way a detriment to the workflow, and I use that feature all the time. Why? The reason is that we focused on the keyboard interaction. The workflow is as follows: Ctrl-u to initiate the revert action The prompt appears immediately. - Hitting any of "enter", "y", or "spacebar" will confirm the confirmation, and proceed. - Hitting any of "escape" or "n" will cancel the action. So essentially the workflow for the power user becomes "ctrl-u, enter" and that is such a tiny overhead that it really is not a bother at all. On the other hand, if I had to actually move my hand over to a mouse or trackpad and actually "click" on something then I would be super annoyed. That would be simply horrible with RSI in mind. OTOH having to hit "enter" or "spacebar" (which is the largest key on your keyboard, and your thumbs have good hefty muscles) is totally acceptable in my book because it strikes the right balance between safety for a destructive operation and convenience. Now, let's consider the alternative -- adding an option to disable the prompt. I don't like that. Why? It's yet another option. It's yet another thing to document, yet another code path, and yet another pitfall for a user who might run git-gui in a different configuration (and becomes surprised when revert doesn't prompt and suddenly loses their work). Do we really need an option, or do we need better usability instead? My opinion is that the latter is the real need. That's my $.02 from having used this feature in practice since 2013.
On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote: > > On Fri, Aug 23, 2019 at 03:31:03AM +0530, Pratyush Yadav wrote: > > Hi, > > > > This series adds the ability to revert selected lines and hunks in > > git-gui. Partially based on the patch by Bert Wesarg [0]. > > > > The commits can be found in the topic branch 'py/revert-hunks-lines' > > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines > > > > Once reviewed, pull the commits from > > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till > > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and > > apply them locally, whichever you prefer. > > > > Changes in v2: > > - Add an option to disable the revert confirmation prompt as suggested > > by Johannes Sixt. > > - Base the patches on Pat's git-gui tree instead of git.git. > > > We've had these features for years in git-cola. this series does not introduce new hotkeys. > > Please copy our keyboard shortcuts. > IMO we should not re-invent the user interactions. > > http://git-cola.github.io/share/doc/git-cola/hotkeys.html my part for the homework: > > Ctrl-u is our revert-unstaged-edits hotkeys. https://github.com/patthoyts/git-gui/commit/b677c66e299c8754a6093cbd19ce71b0ad2a8a17 > "s" is for > staging/unstaging (or Ctrl-s if the file list is focused). https://github.com/patthoyts/git-gui/commit/cd16a6c9298778265a044e5f9a39b006277b32f2 https://github.com/patthoyts/git-gui/commit/e210e67451f22f97c1476d6b78b6fa7fdd5817f9#diff-ceba4b88c7e634c5401a4487d45d3ab4R774 Bert > > The same hotkey is used for operating at the line level. > If no lines are selected, the hunk surrounding the current cursor > position is used. > > Please make keyboard interaction a first-class design consideration. >
On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote: > > I have a very strong opinion about the confirmation dialog, so I'll just > mention that here since Hannes is on this thread. > > In cola we do have a confirmation dialog, and I strongly believe this is > the correct behavior because it's an operation that drops data that > cannot be recovered. > > In the other thread, it was mentioned that this dialog would be a > nuisance. Perhaps that is true -- for the dialog that may have been > implemented in this series (I haven't run it to verify). > > Let's dive into that concern. > > In git-cola we have a confirmation dialog and it is by no way a > detriment to the workflow, and I use that feature all the time. > Why? The reason is that we focused on the keyboard interaction. > > The workflow is as follows: > > Ctrl-u to initiate the revert action > The prompt appears immediately. > - Hitting any of "enter", "y", or "spacebar" will > confirm the confirmation, and proceed. > - Hitting any of "escape" or "n" will cancel the action. > > So essentially the workflow for the power user becomes "ctrl-u, enter" > and that is such a tiny overhead that it really is not a bother at all. > > On the other hand, if I had to actually move my hand over to a mouse or > trackpad and actually "click" on something then I would be super > annoyed. That would be simply horrible with RSI in mind. > I take this as a point for*not* having a confirmation dialog when doing the action per mouse. Which matches exactly my original implementation. > OTOH having to hit "enter" or "spacebar" (which is the largest key on > your keyboard, and your thumbs have good hefty muscles) is totally > acceptable in my book because it strikes the right balance between > safety for a destructive operation and convenience. > > Now, let's consider the alternative -- adding an option to disable the > prompt. I don't like that. > > Why? It's yet another option. It's yet another thing to document, yet > another code path, and yet another pitfall for a user who might run > git-gui in a different configuration (and becomes surprised when revert > doesn't prompt and suddenly loses their work). > > Do we really need an option, or do we need better usability instead? > My opinion is that the latter is the real need. > > > That's my $.02 from having used this feature in practice since 2013. 2012 Best, Bert > -- > David
On Sat, Aug 24, 2019 at 08:57:22AM +0200, Bert Wesarg wrote: > On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote: > > On the other hand, if I had to actually move my hand over to a mouse or > > trackpad and actually "click" on something then I would be super > > annoyed. That would be simply horrible with RSI in mind. > > > > I take this as a point for *not* having a confirmation dialog when > doing the action per mouse. Which matches exactly my original > implementation. +1 > > That's my $.02 from having used this feature in practice since 2013. > > 2012 Nice! ;-) I agree with everything you've written here. cheers,
Am 24.08.19 um 08:57 schrieb Bert Wesarg: > On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote: >> On the other hand, if I had to actually move my hand over to a mouse or >> trackpad and actually "click" on something then I would be super >> annoyed. That would be simply horrible with RSI in mind. >> > > I take this as a point for*not* having a confirmation dialog when > doing the action per mouse. Which matches exactly my original > implementation. I totally agree. -- Hannes
On 23/08/19 11:12PM, Bert Wesarg wrote: > On Fri, Aug 23, 2019, 18:44 Pratyush Yadav <me@yadavpratyush.com> wrote: > > > On 23/08/19 08:04AM, Bert Wesarg wrote: > > > On Fri, Aug 23, 2019 at 12:51 AM Pratyush Yadav <me@yadavpratyush.com> > > wrote: > > > > > > > > On 22/08/19 03:34PM, Junio C Hamano wrote: > > [...] > > > as I'm the one who use this feature for more than 7 years, I can only > > > object to this. I'm happy to have the confirmation dialog for the > > > whole-file revert (the current state) but having the dialog also for > > > partial revert would be too disruptive. And disabling both at the same > > > time would not be an option for me. > > > > > > The thing is, that the partial revert "just don't happen by accident". > > > Here are the minimum user actions needed to get to this dialog: > > > > > > 1. whole-file revert > > > > > > - do a Ctrl+J, more or less anywhere in the GUI > > > > Hmm, have to agree with you on this. It is kind of easy to trigger. But > > read below why I think partial reverts are just as easy to trigger. > > > > > 2. hunk revert/revert one unselected line > > > > > > - right click anywhere in the diff pane (thats around 60% of the window > > area) > > > - move the mouse pointer down 3/4 menu items > > > - click this menu item > > > > But what if you wanted to click "Stage hunk", and instead click "Revert > > hunk" instead. This is what I mean by "accidentally". > > > > This is even more a risk in the current layout of the buttons, which are > > in the order: > > > > Stage Hunk > > Revert Hunk > > Stage Lines > > Revert Lines > > > > but this is your current layout! my layout in the patch from 2012 was (and > still is in my git-gui): > > Stage Hunk > Stage Line > ---------- > Revert Hunk > Revert Line > > > thats a separator inbetween! Looking at how this discussion has been going, I see that people really don't like the dialog, even with a config option. So I will send a re-roll with this layout of buttons and ability to undo last revert. I hope that will satisfy most people. > bert > > > > > In this layout, if you wanted to click Stage, you might end up clicking > > Revert instead, losing part of your work. Even if we switch up the > > layout a bit, I feel like the potential of "mis-aiming" your mouse is > > there, but it can certainly be improved. > > > > > 3. partially revert selected lines > > > > > > - select some content in the diff pane by starting by pressing and > > > holding a left click > > > - end the selection by releasing the left click > > > - move the mouse pointer down 3/4 menu items > > > - click this menu item > > > > > > Thats always at least 2 user actions more than the whole-file revert. > > > Thus this cannot happen by accident quite easily in comparison to the > > > whole-file revert. And thats the reason why this dialog exists, from > > > my point of view. > > > > > > I can see the need to disable the dialog for the whole-file revert, > > > and IIRC that was also requested a long time ago on this list. But I > > > don't see a reason to have this dialog also for the partial reverts as > > > a safety measure. > > > > I do (not too strongly, but I do), as I explained why above. > > > > It shouldn't be too difficult to have separate knobs for whole-file and > > partial reverts, but they will add two config options. Not necessarily a > > bad thing, I just thought the people who wanted to disable partial > > reverts would also want to disable whole-file reverts. > > > > But I have another suggestion in mind. I'll reply to one of the other > > emails in this thread about it. Please read it there, I'd rather not > > type it twice. > > > > -- > > Regards, > > Pratyush Yadav > >