Browse Diff tab menu alternatives and presentation #4569

gerhardol · 2018-03-04T18:00:54Z

Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits. First commit was PR #4566, partly reviewed. (Depended on merged commits #4562 #4563 #4565 #4567 #4568)

Tests:
Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff for difftool

Screenshots before and after (if PR changes UI):

What did I do to test the code and ensure quality:

Added tests
Manual testing

Has been tested on (remove any that don't apply):

Windows 10

codecov · 2018-03-04T18:05:57Z

Codecov Report

Merging #4569 into master will increase coverage by 0.46%.
The diff coverage is 65.27%.

@@            Coverage Diff             @@
##           master    #4569      +/-   ##
==========================================
+ Coverage   26.68%   27.14%   +0.46%     
==========================================
  Files         501      504       +3     
  Lines       40752    41009     +257     
  Branches     5961     5970       +9     
==========================================
+ Hits        10873    11132     +259     
+ Misses      29379    29367      -12     
- Partials      500      510      +10

Impacted Files	Coverage Δ
GitUI/CommandsDialogs/FormBrowse.cs	`4.99% <ø> (-0.02%)`	⬇️
GitUI/CommandsDialogs/FormCommit.cs	`9.06% <0%> (-0.02%)`	⬇️
GitUI/CommandsDialogs/FormDiff.cs	`11.94% <0%> (-2.34%)`	⬇️
...sts/CommandsDialogs/RevisionDiffControllerTests.cs	`100% <100%> (ø)`
...sts/GitCommandsTests/Git/GitRevisionTesterTests.cs	`100% <100%> (ø)`	⬆️
GitUI/UserControls/RevisionGrid.cs	`8.69% <100%> (-0.01%)`	⬇️
GitUI/UserControls/FileStatusList.cs	`19.27% <58.33%> (-0.18%)`	⬇️
GitUI/CommandsDialogs/RevisionDiff.cs	`4.73% <6.59%> (+0.62%)`	⬆️
...sDialogs/RevisionDiffContextMenuControllerTests.cs	`82.92% <82.92%> (ø)`
...andsDialogs/FileStatusListContextMenuController.cs	`85.1% <85.1%> (ø)`
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9949704...b9c1a7a. Read the comment docs.

gerhardol · 2018-03-08T12:08:47Z

The first two commits are being reviewed in #4565, the third was mostly reviewed in #4566 (but cannot be merged in isolation). The review is primarily for the last commit - can be done after #4565 is merged.

Part of gitextensions#4564

Fixes gitextensions#4564 Fixes gitextensions#4387 Fixes gitextensions#4396 Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368 Submitted to allow review of the outcome to gitextensions#4564 Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.) Browse Diff: Submodules actions available for multi select (gitextensions#4568) Presents the Reset options for the correct parent in multi select situations No longer differ between parent-child and first-second for Reset Difftool: Describes A and B revisions in the menu Difftool arguments depends on parent availability Better detection of parents to A/B for diffs Commit: Limit DiffTool and FileHistory to tracked FormDiff: Limit FileHistory to Tracked Use RevisionDiffController to align to Browse-Diff

…instead of A/Parent and B/Second)

RussKie

I will pull the branch down for closer inspection

RussKie · 2018-03-12T05:58:58Z

GitUI/CommandsDialogs/FormDiff.cs

+        private ContextMenuDiffToolInfo GetContextMenuDiffToolInfo()
+        {
+            bool firstIsParent = _revisionDiffController.IsFirstParent(DiffFiles.Revision.ParentGuids, DiffFiles.SelectedItemParents.Select(i => i.Guid));
+            bool localExists = _revisionDiffController.LocalExists(DiffFiles.SelectedItemsWithParent, _fullPathResolver);


this indicates an incorrect use of IFullPathResolver - behaviors must be injected via constructors and not passed as parameters

I see two two solutions for the relative simple LocalExists() function:

Add LocalExists() to IFullPathResolver instead

Duplicate LocalExists() where it is used (two or three modules). A separate interface/class for this seem to be overkill.

Add LocalExists() to IFullPathResolver instead

👍

I duplicated LocalExists() as it deals with GitItemStatusWithParent.
Probably better to move other similar to a separate class (but FullPathResolver.Resolve() will be duplicated instead).

But I can change again.

This reverts commit 036a195.

TODO: review RevisionDiffController IsFirstParent and LocalExists as their logic looks odd

…textMenuController

RussKie · 2018-03-13T10:17:47Z

I've have taken liberty and shuffled things around.

Referencing RevisionDiffController in places other than RevisionDiff is incorrect (I must have missed it when it happened).

I've extracted the common functionality:

what I understood was related to testing/checking of GitRevision - I moved into GitRevisionTester. I'm not 100% settled, but it feels more appropriate than having that logic in RevisionDiffController and then referenced it somewhere else.
shared context menu logic I moved out to RevisionDiffContextMenuController. I couldn't come up with a better name, so feel free to suggest alternatives.

Thought? Opinions?

gerhardol · 2018-03-13T21:00:48Z

I am OK with the changes. Will suggest a few name changes, will readd description etc
Testcases are still quite meaningless, but it looks good with increased coverage.

LocalRevisionExists -> AnyLocalFileExists()

IsFirstParent()
It makes more sense to have FileStatusList as an argument here, that is what is being checked.
But then GitRevisionTester is no longer a fit for the method.

Referencing RevisionDiffController in places other than RevisionDiff is incorrect (I must have missed it when it happened).

That was done here

shared context menu logic I moved out to RevisionDiffContextMenuController.

FileStatusListController or FileStatusListContextMenuController would be more logical.
Remaining parts of RevisionDiffController fits there too even if currently only used in RevisionDiff.cs.

RussKie · 2018-03-13T21:24:32Z

Testcases are still quite meaningless

Disagree. We see a lot of changes coming at a high rate (no one is working here full time).
Tests help to

document design decisions - a good test name spells out requirements (e.g. MyMethod_should_do_something_in_case_of_bla), and
guard against possible regressions.

Speaking of which, to me the logic in both new methods looked very odd.

shared context menu logic I moved out to RevisionDiffContextMenuController.

FileStatusListController or FileStatusListContextMenuController would be more logical.

No probs, let's rename. I like FileStatusListContextMenuController sightly more.

Remaining parts of RevisionDiffController fits there too even if currently only used in RevisionDiff.cs.

The reason I created the controller (and other <UI control>Controllers) is to abstract business logic from the UI layer. I have a dream that one day we may be able to use another UI framework, so the UI should do as little as possible.
So I need to think more about this proposal, but generally unless/until it is used in multiple I see no reason to share it.

…selected

gerhardol · 2018-03-13T22:59:31Z

Renamed and updated. Did not move more around.
Will need to test a little more myself.

Disagree.

We have had this discussion before and try my best to comply to your vision. But in this situation the tests just takes time and decrease my motivation. These tests are just at the "looks" not at the actual contents. The functionality has to broken down considerably different to meaningful, but that is again a lot more effort. I will try to keep quiet.

gerhardol · 2018-03-14T22:14:16Z

@RussKie
I do not plan to make more changes to this (considering some enhancements, but that is for later)

RussKie

Looks good. Great job with tests!

RussKie · 2018-03-15T06:24:28Z

GitCommands/Git/GitRevisionTester.cs

+            }
+
+            return true;
+        }


We need to rename the method to AnyFirstAreParentsToSelected to match the implementation and the doc "True if one of the first selected is parent".

💡 Thinking of it, may be we can change the signature to something like the following?

+ /// <summary> + /// Indicates whether any of the provided <param ref="selectedRevision" />'s parents are selected. + /// </summary> + public bool AnyRevisionParentsSelected(GitRevision selectedRevision, IEnumerable<GitRevision> revisionsSelectedFirst)

I think API becomes slightly more readable:

bool firstIsParent = _revisionDiffController.AnyRevisionParentsSelected(DiffFiles.Revision, DiffFiles.SelectedItemParents);

A rough idea, open to discussion...

The method checks if "True if all of the first selected are parents" so this would be changed behavior

My bad, misread the logic.

I will be happy to take a discussion why I have done like I have done and to change it to something better. How should this work? Your comment is a good indication of that. I could give some more background tomorrow. The alternative is to take this in follow-up discussions.

That's be good. I'm finding the API somewhat confusing.

This PR (including the related PRs) primarily improves the diff and presentation of versions. In this process, the menu logic has been updated, some alternatives were not relevant and other were missing. The logic was also changed so testing could be done (testing is for the display, not for the actions so I do not find the tests useful still but they suck less than they would in my first iterations).

For the alternatives, some kind of rules were used:

With one item selected, the menu items must be relevant

With multiple items selected, menu items are relevant if at least one item is relevant. For instance if submodules are available if at least one submodule is selected. (There are exceptions to this rule)

In general, the behavior should be the same if the parent-child is selected as if just the "child". It was not like this previously, selecting a parent-child was more limiting.
In some situations selecting the parent, then the selected will provide more information, like the actual parents to parents, that enables those menu items (in diff menu).
AllFirstAreParentsToSelected() is a helper for these situations, this is similar to just selecting one revision.

There are some special handling for merge commits, to show the combined diff like in e51741d. This selects all parents and adds the artificial DiffFiles.CombinedDiff to compare to the Selected revision. The combined diff is shown if only one of the parents to e517 is shown but would be confusing if a non parent was selected.
(So combined diff is shown if at least one parent shown, but no other revision than parents).

For staged/unstaged, the availability is available is based on the selected revision but the parent must be staged/Head to be sure a file can be staged/unstaged. This commands breaks the rule to "show if at least one is possible" as it is not possible to determine the availability in a good way for the command.

FirstIsParent is also used to confirm that a parent to parent exists for difftool exists and to prune when parent-to-selected(B) is is the same as first(A).

Pushing a few clarifications related to this.

Not sure if it got any clearer...

RussKie · 2018-03-15T06:32:05Z

GitCommands/Git/GitRevisionTester.cs

+
+            foreach (var item in items)
+            {
+                string filePath = _fullPathResolver?.Resolve(item.Name);


_fullPathResolver can't be null

RussKie · 2018-03-15T06:33:51Z

GitUI/CommandsDialogs/FormDiff.cs

@@ -65,6 +71,12 @@ public partial class FormDiff : GitModuleForm
            DiffText.ExtraDiffArgumentsChanged += DiffTextOnExtraDiffArgumentsChanged;
        }

+        protected override void OnRuntimeLoad(EventArgs e)
+        {
+            _revisionDiffContextMenuController = new FileStatusListContextMenuController();


❓ Can this be moved into constructor? If so, this method an go....

Yes, I just followed the pattern you had use in other modules.

I would have used OnRuntimeLoad instead of instantiating objects in constructor where would be a dependency on fully set GitModule, which isn't available until the control is loaded.
In this case there is no dependency on GitModule and I'm pretty sure it is safe to move the instantiation in to the constructor.

RussKie · 2018-03-15T06:36:51Z

GitUI/CommandsDialogs/RevisionDiff.cs

+            return DescribeRevision(sha1, 0);
+        }
+
+        private string DescribeRevision(string sha1, int maxLength)


we can combine these two methods into one - private string DescribeRevision(string sha1, int maxLength = 0)

changing
Edit: Delegate, need the signature

RussKie · 2018-03-15T06:48:49Z

GitUI/CommandsDialogs/RevisionDiff.cs

+        {
+            var parents = DiffFiles.SelectedItemParents
+                .Where(i => showUnstagedAndCombined ||
+                    !(i.Guid.IsNullOrWhiteSpace() || i.Guid == GitRevision.UnstagedGuid || i.Guid == DiffFiles.CombinedDiff.Text))


what does i.Guid == DiffFiles.CombinedDiff.Text mean?
CombinedDiff.Text is a localised string "Combined Diff", why are we comparing it to a commit's SHA1?

DiffFiles.CombinedDiff.Text is used both for display and as an ID for a variant of an "artificial commit".

Something doesn't feel right about it.... can't articulate what exactly, but it feels bad

It is not nice, but I preferred to not spread the knowledge about the revision. The proper way would be to add it in GitRevision.cs like:
public const string IndexGuid = "1111111111111111111111111111111111111111";

we already have it

gitextensions/GitCommands/Git/GitRevision.cs

Line 18 in 663036a

public const string IndexGuid = "1111111111111111111111111111111111111111";

In a similar way:
public const string CombinedDiffGuid = "2222...";
DiffFiles.CombinedDiff.Text will be used in some situations still.

Added a CombinedDiffGuid for the artificial diff after all. For instance #4154 seem to enhance the comparison more, so it is better to set the pattern. Not sure if the TranslationApp handles this correctly.

RussKie · 2018-03-15T06:51:09Z

UnitTests/GitCommandsTests/Git/GitRevisionTesterTests.cs

+        [Test]
+        public void LocalExists_should_return_false_if_none_of_locally_tracked_items_have_files()
+        {
+            Assert.Inconclusive();


you've done a great job with tests 👍

these tests need finishing too 😉

Will finish later, some done

gerhardol · 2018-03-17T22:21:44Z

I have not planned to change anything more

gerhardol mentioned this pull request Mar 4, 2018

BrowseDiff should show differences to all selected revisions #4564

Closed

4 tasks

RussKie added the 👓 status: needs review label Mar 5, 2018

gerhardol force-pushed the feature/n4564-n4387-browsediff-multiple-revisions branch 2 times, most recently from acfced4 to f45512a Compare March 6, 2018 00:27

This was referenced Mar 6, 2018

Wrong diff for initial commit #4580

Closed

wip Browse Diff: Show differences for several selections #4566

Closed

gerhardol force-pushed the feature/n4564-n4387-browsediff-multiple-revisions branch from f45512a to 48d37c0 Compare March 7, 2018 20:37

gerhardol mentioned this pull request Mar 9, 2018

Browse Diff: Incorrect comparision for multiple parents #4565

Merged

gerhardol force-pushed the feature/n4564-n4387-browsediff-multiple-revisions branch from 48d37c0 to f2d2424 Compare March 10, 2018 12:59

gerhardol changed the title ~~wip Browse Diff tab menu alternatives and presentation~~ Browse Diff tab menu alternatives and presentation Mar 10, 2018

gerhardol force-pushed the feature/n4564-n4387-browsediff-multiple-revisions branch 2 times, most recently from fbc734d to c5e00e4 Compare March 10, 2018 23:48

gerhardol added 3 commits March 11, 2018 19:31

Browse Diff: Show differences for several selections

0d3c196

Part of gitextensions#4564

Cleanup: Use Selected and First consistently for diff/reset options (…

bf40505

…instead of A/Parent and B/Second)

gerhardol force-pushed the feature/n4564-n4387-browsediff-multiple-revisions branch from c5e00e4 to bf40505 Compare March 11, 2018 18:37

Browse diff block reset to Combined diff

a5a97f9

RussKie requested changes Mar 12, 2018

View reviewed changes

Review comment: Split LocalExists() to local, FullPathResolver

036a195

gerhardol force-pushed the feature/n4564-n4387-browsediff-multiple-revisions branch from b5281f4 to 036a195 Compare March 12, 2018 21:33

RussKie added 5 commits March 13, 2018 17:36

Revert "Review comment: Split LocalExists() to local, FullPathResolver"

b081d87

This reverts commit 036a195.

Tweaks and test stubs

31014d9

TODO: review RevisionDiffController IsFirstParent and LocalExists as their logic looks odd

Remove RevisionDiffController from FormDiff and FileStatusList

0f430e2

Remove RevisionDiffController from FormDiff and FileStatusList (cont.)

56b077b

Extract common revision diff context menu logic into IRevisionDiffCon…

578339d

…textMenuController

gerhardol added 2 commits March 13, 2018 23:54

Review comment renames and documentation

c993523

Reset revision desc always set to <multiple> when more than one item …

dd11338

…selected

RussKie requested changes Mar 15, 2018

View reviewed changes

RussKie removed the 👓 status: needs review label Mar 15, 2018

Review comments

7a265cd

gerhardol mentioned this pull request Mar 17, 2018

Merge diff silently switches to diff to 2nd parent when 1st is the same #2535

Closed

Review comments

7de2cf2

Review comments: Implement remaining tests

3e97549

gerhardol force-pushed the feature/n4564-n4387-browsediff-multiple-revisions branch from ab9f736 to 3e97549 Compare March 17, 2018 22:35

RussKie approved these changes Mar 18, 2018

View reviewed changes

gerhardol added 2 commits March 18, 2018 12:30

Review comment: Clarify CombinedDiff Guid

85f0f7f

Review comment: Remove TODO

b9c1a7a

gerhardol merged commit 1dc1f81 into gitextensions:master Mar 18, 2018

gerhardol mentioned this pull request Mar 18, 2018

Show more info in files list of two commit diff #4154

Closed

gerhardol deleted the feature/n4564-n4387-browsediff-multiple-revisions branch November 14, 2021 13:02

Browse Diff tab menu alternatives and presentation #4569

Browse Diff tab menu alternatives and presentation #4569

Conversation

gerhardol commented Mar 4, 2018 • edited Loading

Screenshots before and after (if PR changes UI):

codecov bot commented Mar 4, 2018 • edited Loading

Codecov Report

gerhardol commented Mar 8, 2018

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

RussKie commented Mar 13, 2018

gerhardol commented Mar 13, 2018

RussKie commented Mar 13, 2018 • edited Loading

gerhardol commented Mar 13, 2018

gerhardol commented Mar 14, 2018

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Mar 17, 2018

gerhardol commented Mar 4, 2018 •

edited

Loading

codecov bot commented Mar 4, 2018 •

edited

Loading

gerhardol Mar 12, 2018 •

edited

Loading

RussKie commented Mar 13, 2018 •

edited

Loading

RussKie Mar 16, 2018 •

edited

Loading

gerhardol Mar 15, 2018 •

edited

Loading