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

Blame: Fix "blame previous revision" feature #6841

Merged
merged 4 commits into from Jul 6, 2019

Conversation

@pmiossec
Copy link
Member

commented Jun 22, 2019

Partly fixes #6605 (other part will be fixed by #6807 or #6132)

Proposed changes

  • Fix "Blame to previous revision" behavior to select the first commit before the one introducing the changes (so the previous, no!?!) as explained in #6605 (comment)

  • While I was at it, I have added a context menu item to blame selected revision to make explicit what could be done by double-clicking (because I realised with #6832 (comment) that it was probably not evident for new users)

Screenshots

For the 2nd point (the 1st change nothing to the ui)

Before

Focus only on the menu (and not the colors of the lines)

image

After

image

Test methodology

  • Manual

Test environment(s)

  • Git Extensions 3.2.0
  • Build 8e4b5fc (Dirty)
  • Git 2.20.1.windows.1
  • Microsoft Windows NT 10.0.17134.0
  • .NET Framework 4.7.3416.0
  • DPI 96dpi (no scaling)
@codecov

This comment has been minimized.

Copy link

commented Jun 22, 2019

Codecov Report

Merging #6841 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #6841      +/-   ##
=========================================
+ Coverage   47.68%   47.7%   +0.01%     
=========================================
  Files         731     731              
  Lines       53901   53912      +11     
  Branches     7073    7075       +2     
=========================================
+ Hits        25703   25719      +16     
- Misses      26803   26805       +2     
+ Partials     1395    1388       -7
Flag Coverage Δ
#production 36.8% <0%> (+0.02%) ⬆️
#tests 97.68% <ø> (+0.02%) ⬆️
@pmiossec pmiossec force-pushed the pmiossec:blame_previous_commit branch from c61f9c5 to abdbccf Jun 22, 2019
Copy link
Member

left a comment

The lines of BlameAuthor and BlameFile are not aligned correctly.
Could you add the correct height calculation to this PR, please?

GitBlame blame = Module.Blame(_fileName, objectId + "^", _encoding, originalLine + ",+1");
if (blame.Lines.Count > 0)

selectedRevision = _revGrid.GetRevision(objectId);

This comment has been minimized.

Copy link
@mstv

mstv Jun 23, 2019

Member

_revGrid.GetRevision returns null quite often. Hence the context menu items do not have an effect.

Disable the context menu items in case they will not select an other revision, on opening the context menu, please, because they do not apply at all if already at this revision or if there is no parent revision, respectively.

Best solution: All revisions are available in the RevisionGrid or loaded on demand -- never failing silently without error message.

STR:

  • blame GitUI/UserControls/BlameControl.cs
  • select the revision from master 18e4485
  • try to blame this or the previous revision

This comment has been minimized.

Copy link
@gerhardol

gerhardol Jun 23, 2019

Member

@mstv What is STR? Has Stuttgart Airport anything to do with this?

This comment has been minimized.

Copy link
@mstv

mstv Jun 23, 2019

Member

No, there's no obvious relation ;-)
STR = steps to reproduce.

This comment has been minimized.

Copy link
@pmiossec

pmiossec Jun 23, 2019

Author Member

This PR is not intended to fix that behavior that is caused by the regression about following renames. That's why I wrote :

Partly fixes #6605 (other part will be fixed by #6807 or #6132)

And I also want one of these 2 PRs merged in the next release to have a consistent behavior.

This comment has been minimized.

Copy link
@mstv

mstv Jun 24, 2019

Member

Yes, I agree. I had understood that it will need part 2. Though in order to test the disabling of the menu items, I found it helpful to have an example where it occurs.

GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
@pmiossec pmiossec force-pushed the pmiossec:blame_previous_commit branch 2 times, most recently from a487fe9 to fa7334c Jun 25, 2019
Copy link
Member

left a comment

Still open: The lines of BlameAuthor and of BlameFile are not aligned correctly.
Could you add the correct height calculation to this PR, please?

The disabling of the context menu items can be done in the second PR.
I think it would help testing the coming changes.

else
_revGrid.SetSelectedRevision(revisionId);
}
else

This comment has been minimized.

Copy link
@mstv

mstv Jun 26, 2019

Member

nit: Our new coding style prefers early return over else in order to reduce nesting.

@pmiossec

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

@mstv I think I have fixed most of the things related to this PR and that I won't add much.

The 2 others improvements you ask we to do are not really related to the original goal of this PR just intended to fix the 'blame previous commit' feature.

Still open: The lines of BlameAuthor and of BlameFile are not aligned correctly.
Could you add the correct height calculation to this PR, please?

Unfortunately, no 😕 for some reason :

  • I don't have the problem, so I can't work on it and test the fix
  • the fix in my other branch is on a component that don't exist in this branch, so the fix should be completely different and I don't know where to do it. That's why I'm not very confident in including it.
  • not related (like just said)

The disabling of the context menu items can be done in the second PR.
I think it would help testing the coming changes.

Yes, I would like to see it fixed in another PR but I want to wait for the rename regression before working on it.

So, except the last review comment, I think this PR is in good state to be merged...

@mstv

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

OK

@pmiossec pmiossec force-pushed the pmiossec:blame_previous_commit branch 3 times, most recently from f715aa8 to c616642 Jul 1, 2019
@mstv
mstv approved these changes Jul 1, 2019
Copy link
Member

left a comment

LGTM. In part 2, the context menu items should be disabled if they are not applicable. This will improve the UX, especially if the file history is not complete. And it will facilitate testing the file history fix.

@mstv mstv added the status: ready label Jul 1, 2019
pmiossec added 3 commits May 29, 2019
to select the first commit before the revision selected (so the previous, no!?!)

Partly fix #6605
to make explicit what could be done by double-clicking
@pmiossec pmiossec force-pushed the pmiossec:blame_previous_commit branch from c616642 to 8618125 Jul 2, 2019
@pmiossec

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

In part 2, the context menu items should be disabled if they are not applicable

@mstv I finally did it.

@mstv
mstv approved these changes Jul 2, 2019
Copy link
Member

left a comment

Thank you. LGTM.
@gitextensions/git-extensions-source, I am voting to include this in master now.

Copy link
Member

left a comment

Blame this revision is something I often missed
Agree this should go in master now and be included in 3.2

@@ -414,7 +414,18 @@ private int GetBlameLine()

private void contextMenu_Opened(object sender, EventArgs e)
{
blameRevisionToolStripMenuItem.Enabled = false;

This comment has been minimized.

Copy link
@gerhardol

gerhardol Jul 2, 2019

Member

I was confused enough to comment and would like to have a comment like
//Disable the menu entries by default
Disabling could be done in the early exit too

This comment has been minimized.

Copy link
@pmiossec

pmiossec Jul 3, 2019

Author Member

I put it back in the early exit as it is equivalent and was what I did first...

@RussKie

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Is there a way we can test it?

@pmiossec

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Is there a way we can test it?

Looking at the code, I don't see how to achieve it with unit tests because under the hood, it relies on RevisionGridControl which is very difficult to handle.

With integration tests, it should be achievable (with a lot of work).
We have to improve a lot GitModuleTestHelper to be able to create git commits and complex histories and doing file renames.

@pmiossec pmiossec force-pushed the pmiossec:blame_previous_commit branch from 8618125 to 7be58af Jul 3, 2019
Copy link
Member

left a comment

Suggest merging, adding to 3.2 milestone

@gerhardol gerhardol added this to the 3.2.0 milestone Jul 5, 2019
@RussKie RussKie merged commit b33a12e into gitextensions:master Jul 6, 2019
3 checks passed
3 checks passed
CodeFactor No issues found.
Details
WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@RussKie RussKie removed the status: ready label Jul 6, 2019
@pmiossec pmiossec deleted the pmiossec:blame_previous_commit branch Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.