Stage Diff Hunk inconsistency #330

Closed
living180 opened this Issue Jul 22, 2014 · 5 comments

Projects

None yet

3 participants

@living180
Contributor

I've discovered what seems to me to be inconsistent behavior with the Stage Diff Hunk and related context menu actions. If there is no text selected, the hunk under the cursor is staged, which makes sense. However, if there is text selected, the hunk(s) contained within the selection will be staged instead. I found this behavior surprising.

I would instead expect either one of two things:
a) for the Stage Diff Hunk action to always act upon the hunk under the cursor
or
b) for the name of the action to change to something like Stage Selected Diff Hunks if there is a selection.

If you think either of these changes would be an improvement, I'm happy to put together a PR if that would be helpful.

[Edited to change "Diff Region" to "Diff Hunk" per #328]

@living180 living180 changed the title from Stage Diff Region inconsistency to Stage Diff Hunk inconsistency Jul 23, 2014
@davvid
Member
davvid commented Jul 24, 2014

Part of the reason may be to make the keyboard-shortcut workflow smoother. The workflow is "hit s to stage stuff". If you have a selection it uses the selection. If you don't, it uses the entire hunk. It's a nice convenient thing that we should keep. That's just a note -- this issue is actually talking about "Stage Diff Hunk" and its menu actions, but they probably share the same code paths.

I think for the menu actions it might make sense to do something like option B since it'll make the behavior when multiple hunks are selected clearer to the user. What do you think?

I guess one downside to option B is that it introduces a new string that has to be translated. That downside exists for just about any UI change so it's not a show-stopper by any means, just a note.

@ugtar do you think option B is good, or would changing the behavior to only operate on a single hunk be better in your opinion?

@ugtar
Member
ugtar commented Jul 24, 2014

Personally I would prefer option C (heh). That is simplify. Just like the
shortcut for 's' stages the selection if there is one, or stages the hunk
under the cursor if there is no selection, the context menu should have
only one option, and that option should be sensitive to the context, just
like the shortcut key.

So, if you have a selection, the context menu item would be "stage selected
lines". If there is nothing selected the the only context menu item would
be "stage diff hunk".

The only functionality that would be lost by making this change is you
would lose the ability to stage multiple hunks. However I see this as a
small loss since you need to make a selection to do that anyway. You can
easily select the entirety of the multiple hunks and just use the "stage
selected lines" function.

This reduces the options to one at all times. A simple "stage stuff" (as
you hinted at) that does the Right Thing based on the context (with
appropriate wording for the two cases). Incidentally this also obsoletes
the mostly extraneous 'h' shortcut. Ah dwimery....

Let me know if I've missed anything.

Uri

Please consider the environment before printing this message.
http://wwf.panda.org/savepaper/
On Jul 23, 2014 8:29 PM, "David Aguilar" notifications@github.com wrote:

Part of the reason may be to make the keyboard-shortcut workflow smoother.
The workflow is "hit s to stage stuff". If you have a selection it uses the
selection. If you don't, it uses the entire hunk. It's a nice convenient
thing that we should keep. That's just a note -- this issue is actually
talking about "Stage Diff Hunk" and its menu actions, but they probably
share the same code paths.

I think for the menu actions it might make sense to do something like
option B since it'll make the behavior when multiple hunks are selected
clearer to the user. What do you think?

I guess one downside to option B is that it introduces a new string that
has to be translated. That downside exists for just about any UI change so
it's not a show-stopper by any means, just a note.

@ugtar https://github.com/ugtar do you think option B is good, or would
changing the behavior to only operate on a single hunk be better in your
opinion?


Reply to this email directly or view it on GitHub
#330 (comment).

@davvid
Member
davvid commented Jul 24, 2014

Yes! That's even better. \O//

@living180
Contributor

@ugtar I like that a lot too. That's a lot more straightforward than either option I had come up with. @davvid would you like me to put something together to implement this?

@davvid
Member
davvid commented Jul 24, 2014

Definitely, that'd be awesome.

@living180 living180 added a commit to living180/git-cola that referenced this issue Jul 28, 2014
@living180 living180 widgets.diff: simplify stage/unstage/revert
Previously the Stage, Unstage, and Revert commands each had two
variants, e.g. "Stage Diff Hunk" and "Stage Selected Lines".  The "Diff
Hunk" variant would act upon the hunk underneath the cursor if there was
no selection, or on all hunks falling within the selection if there was
a selection.  The "Selected Lines" variant was only enabled if there was
a selection and would operate on the lines falling within the selection.

It was decided to simplify things and combine the two variants into a
single command that adapts to the context.  Returning to the "Stage"
case, the command stages the hunk under the cursor if there is no
selection, and stages the selected lines if there is a selection.

This was actually implemented by making a single context-sensitive
action for the Stage/Unstage case, and another action for the Revert
case.  The action text (and icon, for the Stage/Unstage action) is set
dynamically when creating the context menu.

Closes #330

Suggested-by: Uri Okrent <uokrent@gmail.com>
Helped-by: David Aguilar <davvid@gmail.com>
Signed-off-by: Daniel Harding <dharding@living180.net>
e112471
@living180 living180 added a commit that closed this issue Jul 29, 2014
@living180 living180 widgets.diff: simplify stage/unstage/revert
Previously the Stage, Unstage, and Revert commands each had two
variants, e.g. "Stage Diff Hunk" and "Stage Selected Lines".  The "Diff
Hunk" variant would act upon the hunk underneath the cursor if there was
no selection, or on all hunks falling within the selection if there was
a selection.  The "Selected Lines" variant was only enabled if there was
a selection and would operate on the lines falling within the selection.

It was decided to simplify things and combine the two variants into a
single command that adapts to the context.  Returning to the "Stage"
case, the command stages the hunk under the cursor if there is no
selection, and stages the selected lines if there is a selection.

This was actually implemented by making a single context-sensitive
action for the Stage/Unstage case, and another action for the Revert
case.  The action text (and icon, for the Stage/Unstage action) is set
dynamically when creating the context menu.

Closes #330

Suggested-by: Uri Okrent <uokrent@gmail.com>
Helped-by: David Aguilar <davvid@gmail.com>
Signed-off-by: Daniel Harding <dharding@living180.net>
cde61a7
@living180 living180 closed this in cde61a7 Jul 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment