RevFileTree: Blame instead of View #9424

gerhardol · 2021-07-28T21:26:44Z

Part of #7598
Closes #9435
Closes #9437

Proposed changes

RevFileTree: Blame instead of View
Show blame for object type blobs (files), as is available in FileHistory.
Note that object type commit (submodules) are shown as text, as before.
It is possible to toggle the view to show file contents, as before.

In FileHistory the commit info is available in a header. This is excluded in RevFileTree. The info for the selected commit can be seen in Commit, Diff tabs, overview is available in tooltip for otyher commits.
This could be configurable.

RevDiff: HotKey to view FileTree
A menu command existed to view the file, this as a hotkey.

Note that T was selected as key, F is to be used to add the filepath to the Advanced filter in the grid.

RevDiff: B to view Blame instead of Diff
Note that this is just for the current file, selecting another (or the same) file will show Diff by default.
For artificial, HEAD is blamed, worktree/index changes cannot be shown
When blaming in RevDiff or FileTree, select the current line in fileviewer in Blame (especially useful in RevDiff)
Cancellation support in LoadBlame() (in between operations, not in methods)

Screenshots

Before

For reference - FileHistory Blame tab, unchanged by this PR

(RevDiff B opened FileHistory in a new instance, no screenshot.)

After

Only the hotkey added

Context menu and hotkey B toggle Diff/Blame.

Test methodology

Manual

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

mstv

👍 Feels quite natural.

Wishes for this PR:

Improve FileTree / BlameControl updates #9437 Update / paint FileTree independent from BlameControl in order to reduce flicker
Improve FileTree / BlameControl updates #9437 Make switching commits, e.g. Blame previous revision, not loose the file selection

Wishes for later:

Improve FileTree / BlameControl updates #9437 Scroll FileTree selection to be visible when switching commits, e.g. on Blame this / previous
BlameControl: Make Commit Info gutter collapsible #9436 Make Commit Info gutter of BlameControl collapsible (simulating "View"; improves scrolling UX)
Store / restore splitter position of BlameControl (there is a pending issue for master anyway: 13caf92)
should be done in Restore splitter persistence #9106
BlameControl: Hide unrelated context menu item #9435 Hide unrelated context menu item
BlameControl: Correctly align line numbers with text #9434 BlameControl: Correctly align line numbers with text

GitUI/CommandsDialogs/FormBrowse.cs

GitUI/CommandsDialogs/RevisionFileTreeControl.Designer.cs

GitUI/CommandsDialogs/RevisionFileTreeControl.cs

gerhardol · 2021-08-01T22:25:41Z

Wishes for this PR:

Most of these are for BlameControl should be a separate issue (#7598 is quite confusing as is...)
Improvements are necessary.

Also if Blame is improved, there may be need for a configurable way to show Blame/View (or even Blame/Diff).
Some discussions in #7598, this is a first implementation to fond what is really needed.

Edit: The blame/view context menu control could be added to this or if requested, if that is considered a longer term solution.

gerhardol · 2021-08-05T23:49:09Z

Converted this to a draft and based it on #9425 (approved, to be merged soon) and #9426 as there are some functionality required.
The commits from 7037db2 RevFileTree: Blame instead of View to 1c921d7 fixup! RevFileTree: Blame instead of View are unchanged.

Some new additions:

flicker selecting is removed in c7f0165 fixup! avoid flicker switching files
The existing issue is removed 58e63c0 Blame; Hide EnableAutomaticContinuousScroll
To avoid that slow blame is a blocker, it is possible to View files as before.
The shortcut key B is used to toggle View/Blame instead of opening a new instance. (the value is stored in settings.
Note: a similar change should be added to RevDiff, B to Blame files.

gerhardol · 2021-08-08T23:00:07Z

Cleaned up commits a little, adding RevDiff Blame
Ready for review again
Further changes are presented in #9445

GintasS

Wrote a few comments regarding style basically.

GitUI/CommandsDialogs/RevisionFileTreeControl.cs

gerhardol · 2021-08-09T19:39:02Z

Rebased on master to resolve MC (and master eeb8f67 is unusable, fixed in adbb410).
Also squashed the NRT in #9424 (comment)

mstv

LGTM

mstv · 2021-08-09T22:00:01Z

GitUI/CommandsDialogs/RevisionFileTreeControl.Designer.cs

+            this.BlameControl.Margin = new System.Windows.Forms.Padding(0);
+            this.BlameControl.Name = "blameControl";
+            this.BlameControl.Size = new System.Drawing.Size(487, 303);
+            this.BlameControl.TabIndex = 0;


Conflict with FileText.TabIndex (although they are not displayed at the same time).

That was intentional, really a need to separate them?

As a general rule: no item should have identical indices. Though this is the keyboard tab order, i.e. used when tab between controls. Accessibility tools don't use it (AFAIK), and use the order in which controls are added into respective parents instead.

Do you request to change this?

gerhardol · 2021-08-17T19:31:58Z

Rebased without changes, to simplify reviews and get the build successful

RussKie · 2021-08-18T13:39:52Z

GitUI/CommandsDialogs/RevisionFileTreeControl.cs

@@ -51,26 +51,29 @@ public partial class RevisionFileTreeControl : GitModuleControl
        private readonly Stack<string> _lastSelectedNodes = new();
        private readonly IRevisionFileTreeController _revisionFileTreeController;
        private readonly IFullPathResolver _fullPathResolver;
-        private readonly IFindFilePredicateProvider _findFilePredicateProvider;
+        private readonly IFindFilePredicateProvider _findFilePredicateProvider = new FindFilePredicateProvider();


Why is this necessary?

It is not necessary for this PR, it was a change I had done (and believe should be changed) that was refactored to a separate commit

GitUI/CommandsDialogs/RevisionDiffControl.cs

GitUI/CommandsDialogs/RevisionFileTreeControl.cs

gerhardol · 2021-08-18T22:35:27Z

Added two more features:

When blaming in RevDiff or FileTree, select the current line in fileviewer in Blame (especially useful in RevDiff)
Cancellation support in LoadBlame() (in between operations, not in methods)

RussKie · 2021-08-22T05:45:08Z

GitUI/CommandsDialogs/RevisionFileTreeControl.cs

        {
+            _revisionGrid = revisionGrid;


I very much dislike this - this creates further coupling between various controls, and making it harder to reason about each individual control's functionality, and further complicating testing stories.

This is required for BlameControl so the same as in FormFileHistory.
You can see this as a step to remove a lot of code in GE: The follow up #9445 obsoletes FormFileHistory, so this is a step to remove a complete form.

gerhardol · 2021-09-03T22:39:46Z

Rebased to resolve a merge conflict

RussKie · 2021-09-04T14:44:56Z

I ran it locally briefly, seems to work. Need time to form a better opinion, but overall I think it is a good change.
Two things I've noticed:

"Blame" isn't sticky, i.e. when change file selection the view changes to the "diff". Feels weird.
Attempt to open history "feels" like it getting ignored. At least there is no immediate feedback, the new window doesn't open, or opens after a significant delay.

gerhardol · 2021-09-04T16:43:52Z

1. "Blame" isn't sticky, i.e. when change file selection the view changes to the "diff". Feels weird.

Blame/View is sticky in FileTree tab.
Intentional in diff tab, I want to go to diff quickly in diff tab. This is partly as BlameControl is slower, annoying to show first then to blame.
I have used this for a month or so now, this is something I appreciate so I prefer to keep this.

2. Attempt to open history "feels" like it getting ignored. At least there is no immediate feedback, the new window doesn't open, or opens after a significant delay.

Do you mean H? That is not changed by this PR.
Or open FileTab? Open FileTree with Blame requires a little longer time to open Blame to View, That is why Blame/View is sticky, stored in settings.

Blame can start presenting on a line. This is is available in RevDiff and RevFileTree since gitextensions#9424. (Even if the code enabling this has existed for long time.) The "goto line" was only used if the control had to be init, not if e.g. toggling to Diff and selecting an new diff line and then Blame again.

Blame can start presenting on a line. This is is available in RevDiff and RevFileTree since gitextensions#9424. (Even if the code enabling this has existed for long time, it was just used for a special case.) The "goto line" was only used if the control had to be init, not if e.g. toggling to Diff and selecting an new diff line and then Blame again.

Blame can start presenting on a specific line. This is is available in RevDiff and RevFileTree since #9424. (Even if the code enabling this has existed for long time, it was just used for a special case.) The "goto line" was only used if the control had to be initialised, not if e.g. toggling to Diff and selecting an new diff line and then Blame again.

"RevisionFileTreeShowBlame" settings was introduced in gitextensions#9424. The issue with this settings is that whenever a file history is opened, blame is turned on for all open instances of the app, and affects the File Tree tab experience.

Resolves #10511 "RevisionFileTreeShowBlame" settings was introduced in #9424. The issue with this settings is that whenever a file history is opened, blame is turned on for all open instances of the app, and affects the File Tree tab experience. Co-authored-by: Gerhard Olsson <6248932+gerhardol@users.noreply.github.com>

Resolves #10511 "RevisionFileTreeShowBlame" settings was introduced in #9424. The issue with this settings is that whenever a file history is opened, blame is turned on for all open instances of the app, and affects the File Tree tab experience. Co-authored-by: Gerhard Olsson <6248932+gerhardol@users.noreply.github.com> (cherry picked from commit 53d608e)

ghost assigned gerhardol Jul 28, 2021

This was referenced Jul 28, 2021

Show artificial commits also for filtered grid #9393

Merged

Redesign of FormFileHistory #7598

Closed

mstv reviewed Aug 1, 2021

View reviewed changes

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/RevisionFileTreeControl.Designer.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/RevisionFileTreeControl.cs Outdated Show resolved Hide resolved

gerhardol force-pushed the feature/i7598-filetree-blame branch from f4c28ee to 3976a07 Compare August 2, 2021 22:00

This was referenced Aug 3, 2021

Restore splitter persistence #9106

Merged

BlameControl: Make Commit Info gutter collapsible #9436

Open

Improve FileTree / BlameControl updates #9437

Closed

gerhardol marked this pull request as draft August 5, 2021 23:29

gerhardol force-pushed the feature/i7598-filetree-blame branch from 3976a07 to d392820 Compare August 5, 2021 23:35

This was referenced Aug 6, 2021

Blame/filter in Browse, replace formfilehistory #9445

Merged

Diff to actual parent in filtered grids #9426

Merged

gerhardol force-pushed the feature/i7598-filetree-blame branch 2 times, most recently from 390b8fa to 34691dc Compare August 8, 2021 22:44

gerhardol marked this pull request as ready for review August 8, 2021 22:57

RussKie self-requested a review August 9, 2021 01:07

GintasS reviewed Aug 9, 2021

View reviewed changes

gerhardol force-pushed the feature/i7598-filetree-blame branch from 00f6da8 to 16b7ec4 Compare August 9, 2021 19:36

mstv reviewed Aug 9, 2021

View reviewed changes

gerhardol force-pushed the feature/i7598-filetree-blame branch from 16b7ec4 to 20fee19 Compare August 17, 2021 19:31

RussKie reviewed Aug 18, 2021

View reviewed changes

gerhardol mentioned this pull request Aug 20, 2021

BlameControl: Hide unrelated context menu item #9435

Closed

RussKie reviewed Aug 22, 2021

View reviewed changes

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Aug 31, 2021

LoadBlame: CancellationToken (gitextensions#9424)

ebb38ec

gerhardol added 5 commits September 3, 2021 23:26

Hide CommitInfo in FileTree

3ed8202

RevDiff: HotKey to view FileTree

b12ad3b

RevDiff: Possible to Blame files instead of view/diff

af4913c

RevDiff/FileTree: Start blaming on current file line

1c71c1c

LoadBlame: CancellationToken (gitextensions#9424)

12ce1eb

gerhardol force-pushed the feature/i7598-filetree-blame branch from ebb38ec to 12ce1eb Compare September 3, 2021 22:38

RussKie approved these changes Sep 4, 2021

View reviewed changes

RussKie merged commit 1fd88ac into gitextensions:master Sep 5, 2021

RussKie added this to the 3.6 milestone Sep 5, 2021

gerhardol deleted the feature/i7598-filetree-blame branch September 6, 2021 05:22

gerhardol mentioned this pull request Jan 7, 2022

RevDiff: Persist Blame until another file is selected #9804

Merged

gerhardol mentioned this pull request Jan 7, 2022

Blame: Start on line also when control already exists #9805

Merged

This was referenced Jan 22, 2022

RevDiff: Setting to show Blame in FileTree #9831

Merged

Blame: Start on line also when control already exists #9832

Merged

RussKie mentioned this pull request Nov 27, 2022

Make "blame" setting non-sticky #10453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RevFileTree: Blame instead of View #9424

RevFileTree: Blame instead of View #9424

gerhardol commented Jul 28, 2021 •

edited

mstv left a comment •

edited

gerhardol commented Aug 1, 2021 •

edited

gerhardol commented Aug 5, 2021

gerhardol commented Aug 8, 2021

GintasS left a comment

gerhardol commented Aug 9, 2021

mstv left a comment

mstv Aug 9, 2021

gerhardol Aug 9, 2021

RussKie Aug 18, 2021

gerhardol Aug 18, 2021

gerhardol commented Aug 17, 2021

RussKie Aug 18, 2021

gerhardol Aug 18, 2021

gerhardol commented Aug 18, 2021

RussKie Aug 22, 2021

gerhardol Aug 22, 2021

gerhardol commented Sep 3, 2021

RussKie commented Sep 4, 2021

gerhardol commented Sep 4, 2021

RevFileTree: Blame instead of View #9424

RevFileTree: Blame instead of View #9424

Conversation

gerhardol commented Jul 28, 2021 • edited

Proposed changes

Screenshots

Before

After

Test methodology

mstv left a comment • edited

Choose a reason for hiding this comment

gerhardol commented Aug 1, 2021 • edited

gerhardol commented Aug 5, 2021

gerhardol commented Aug 8, 2021

GintasS left a comment

Choose a reason for hiding this comment

gerhardol commented Aug 9, 2021

mstv 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

gerhardol commented Aug 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Sep 3, 2021

RussKie commented Sep 4, 2021

gerhardol commented Sep 4, 2021

gerhardol commented Jul 28, 2021 •

edited

mstv left a comment •

edited

gerhardol commented Aug 1, 2021 •

edited