Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RevisionDiff: Stage/Unstage selected lines #7825

Merged
1 commit merged into from Jan 13, 2021

Conversation

gerhardol
Copy link
Member

@gerhardol gerhardol commented Feb 28, 2020

Fixes #7338

Based on #8708

Proposed changes

Move handling of stage/unstage/reset line patching from FormCommit to FileViewer.
Instead of FormCommit adding menu items to FileViewer, handle
the actions directly in FileViewer, including the hotkey handling.
Also hotkey for general stage/reset lines (previously cherry-pick and revet line patches).
This also aligns the stage/reset menu items to other forms.

Apply the line patching also to RevDiff, supporting line patches for
worktree/index properly, including hotkey handling.
When manipulating a diff involving an artificial commit, refresh the diff

Some code cleanup, like using ViewChangesAsync consistently for diffs/patches

This is a step to avoid using FormCommit to prepare commits

Screenshots

For WorkTree, Similar for Index
Normal commits gets hotkeys and changed Copy grouping too

Before

image

image

After

image

image

Test methodology

Manual


✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned gerhardol Feb 28, 2020
@gerhardol gerhardol force-pushed the feature/i7338-revdiff-stage branch from cd90545 to 8f65c7b Compare March 1, 2020 13:10
@gerhardol gerhardol force-pushed the feature/i7338-revdiff-stage branch from 8f65c7b to e08f690 Compare March 1, 2020 23:48
@gerhardol gerhardol changed the title WIP RevisionDiff: Stage/Unstage selected lines RevisionDiff: Stage/Unstage selected lines Mar 1, 2020
@gerhardol
Copy link
Member Author

CodeFactor complains about complexity for a function with case (3 more added by new hotkeys). I do not see a good way to rewrite that.

@RussKie
Copy link
Member

RussKie commented Mar 2, 2020 via email

@gerhardol
Copy link
Member Author

Updated for #7831
(to be squashed on next update)
I considered a nicer solution than enabling the stage/unstage menu item in TextLoaded, but will wait for that for after #4154 (that partly depends on this PR).
Build fails on translation, I expect this to go in after #7877.

@mstv @RussKie Can you please review?

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stage/untage file/selected lines seem to work well. But the hotkeys are not shown for the FileStatusList.
Reset selected lines works with the same limitations as in FormCommit -- OK.
I have finished the review of the -- well separated -- commits except the main one db6109b. I'll have a look at it later.

The next thing I really like to have, is that the hotkeys work from the RevisionGrid, too, because it gets focused when switching the artificial commits.
Though before we duplicate the hotkeys as for F3, we should rework the hotkey handling completely.

GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
@gerhardol
Copy link
Member Author

gerhardol commented Mar 21, 2020

Stage/untage file/selected lines seem to work well. But the hotkeys are not shown for the FileStatusList.

I cannot why they do not appear...
Should have commented on that

The next thing I really like to have, is that the hotkeys work from the RevisionGrid, too, because it gets focused when switching the artificial commits.

and/or hotkeys for switching between worktree/index, or global keys to select worktree/index and view RevDiff
But that is later, I would like to finish other open items first...

I have finished the review of the -- well separated -- commits except the main one db6109b. I'll have a look at it later.

The main commit is e9ae2d4. db6109b is separate as #7831 required changes after this was submitted. All commits should be squashed when merged

@gerhardol gerhardol force-pushed the feature/i7338-revdiff-stage branch 2 times, most recently from e39e71b to bc32d50 Compare March 22, 2020 10:16
@mstv
Copy link
Member

mstv commented Mar 22, 2020

global keys to select worktree/index and view RevDiff

👍 That's the use case. Of course, for later.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several nits and
The hotkey handling for Reset file(s) to | First is too restrictive: It often does not have any effect although the menu item works as expected.

GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R works for file as expected now. I guess I had not built everything last time.
One final nit.

GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
@gerhardol
Copy link
Member Author

R works for file as expected now. I guess I had not built everything last time.

I have seen this too, since around the time SDK style project was merged. (I did report this in a comment, probably my worst issue description ever). It seem to occur more often if I have not saved before Start/F5, but there has been situations where I thought I saved too.

Dont know how to proceed with that.

@gerhardol
Copy link
Member Author

There are some things I am not so happy about with this PR.
Some changes will be simpler with changes that depends on the following:
#4154 (comment)

Replace ViewCurrentChanges with ViewChangesAsync - to unify

Another is the handling of SelectedDiff.CherryPickContextMenuEntry_Visible(); in TextLoaded. If the file to view is stored in FileViewer as GitItemStatusWithParent then that handling can be built in, as well as the patch handling that currently is slightly duplicated in FileViewer, FormCommit and RevisionDiffControl.

@gerhardol
Copy link
Member Author

Squashed review comments and changes due to #7831, rebased on #7868 (to be merged tomorrow, will conflict). If no further review I plan to initiate a merge in two days.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead!

@gerhardol
Copy link
Member Author

👍 works well again (shortcuts), (waiting for the refactoring).

The merge problems was in the private branch only that I had not pushed in a while.
I am not so inspired to rewrite this, it takes some time...

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Dec 31, 2020
The "Add selection to commit message" command is inserted to the
FileViewer menu, which is confusing.
Other inserted entries are removed in gitextensions#7825.
A proper redesign could be done with enabling and eventhandlers,
but usage is considered low and workaround is to copy&paste.
gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Dec 31, 2020
The "Add selection to commit message" command is inserted to the
FileViewer menu, which is confusing.
Other inserted entries are removed in gitextensions#7825.
A proper redesign could be done with enabling and eventhandlers,
but usage is considered low and workaround is to copy&paste.
@gerhardol gerhardol force-pushed the feature/i7338-revdiff-stage branch 2 times, most recently from ba2ebaf to 52de36e Compare January 1, 2021 21:43
@gerhardol
Copy link
Member Author

Rewritten the existing functionality (and thereby this PR) after @RussKie requests.
Still WIP as #8708 is not merged yet - I actually would like a comment on this PR before merging that PR.
The code is ready for a review though.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only nits. Will run it later.

GitUI/CommandsDialogs/FormCommit.cs Outdated Show resolved Hide resolved
GitUI/Editor/FileViewer.cs Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
@gerhardol
Copy link
Member Author

Rebased on latst #8708 , review comments addressed and a fixup of my own

@gerhardol gerhardol marked this pull request as ready for review January 5, 2021 20:32
@gerhardol gerhardol changed the title WIP RevisionDiff: Stage/Unstage selected lines RevisionDiff: Stage/Unstage selected lines Jan 5, 2021
@gerhardol
Copy link
Member Author

Squashed review comments (but not my unreviewed changes) and rebased on master, removed WIP now when #8708 is merged

@RussKie
Copy link
Member

RussKie commented Jan 10, 2021

Since @mstv is happy, let's merge it :)

@gerhardol
Copy link
Member Author

Will merge after #8734

Now I only miss a way to change the commit message for Index commit and a commit button to skip the modal commit dialog in most situations

Move handling of stage/unstage/reset line patching from FormCommit to FileViewer.
Instead of FormCommit adding menu items to FileViewer, handle
the actions directly in FileViewer, including the hotkey handling.
Also hotkey for cherry-pick and reset line patches.
This also aligns the stage/reset menu items to other forms.

Apply the line patching also to RevDiff, supporting line patches for
worktree/index properly, including hotkey handling.
When manipulating a diff with an artificial commit, refresh the diff
after patching.

Some code cleanup, like using ViewChangesAsync consistently for diffs/patches

FormCommit; Use common method for head revisions
Set parents for artificial revisions
@gerhardol
Copy link
Member Author

Squashed and rebased after #8734 with a minor removal ofunused code in FormCommit:ShowChanges()

@gerhardol
Copy link
Member Author

@msftbot merge in 3 days

@ghost ghost added the status: auto merge label Jan 10, 2021
@ghost
Copy link

ghost commented Jan 10, 2021

Hello @gerhardol!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Wed, 13 Jan 2021 19:58:24 GMT, which is in 3 days

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 7a2a9d5 into gitextensions:master Jan 13, 2021
@ghost ghost added this to the 4.0 milestone Jan 13, 2021
@gerhardol gerhardol deleted the feature/i7338-revdiff-stage branch January 13, 2021 20:21
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stage/unstage select lines in Browse
3 participants