feat: Avoid coloring moved lines #11947

gerhardol · 2024-09-27T22:58:23Z

Fixes #11843
Depends on #11862

Proposed changes

feat: Avoid coloring moved lines

Use heuristics to select which lines that are candidates to
mark inlines for, avoid lines that Git has detected
as moved lines.
(the change is not complete, a few parts required from the second commit, to be squashed befor merge)

feat: Only parse diff files once

Only parse the document for added/removed lines once, by using
the DiffLineInfo info parsed for the linenumbering.
Previously, the document was parsed many times every time the
highlighting (including searching and marking) was changed.
This simplifies the code.

Test methodology

Some kind of tests will be added for the gap change, the other visible change has updated tests.

Merge strategy

Rebase

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

mstv

review of first commit:

test CanGetAddedMovedDiffInfo needs to be adapted

src/app/GitUI/Editor/Diff/DiffLineNumAnalyzer.cs

src/app/GitUI/Editor/Diff/DiffLineInfo.cs

src/app/GitUI/Editor/Diff/DiffLineNumAnalyzer.cs

src/app/GitUI/Editor/Diff/DiffLinesInfo.cs

mstv · 2024-09-28T12:25:45Z

src/app/GitUI/Editor/Diff/GrepHighlightService.cs

+    public override void AddTextHighlighting(IDocument document)
+        => document.MarkerStrategy.AddMarkers(_textMarkers);


❔ Move to a base class?

That would be TextHighlightService, but there are no textMerkers there.
So overridden in Diff (base), Grep and Difftastic.

RussKie · 2024-09-28T14:29:06Z

src/app/GitUI/Editor/Diff/Segment.cs

+
+namespace GitUI.Editor.Diff;
+
+public struct Segment : ISegment


Why do we need the interface?

It is a TextEditor interface

There is one implementation in externals\ICSharpCode.TextEditor\Project\Src\Document\AbstractSegment.cs
Dis not use it due to the name.
The class is only used in this file.

mstv · 2024-09-28T21:06:38Z

The first commit does not detect / skip all moved lines. This is fixed up by the second commit. But the first commit should correctly handle all cases.

You have provided another example commit with commit 1e5d99a, file src/app/GitUI/Editor/Diff/DiffHighlightService.cs, lines 210 / 180 ff.

first commit 4aed061:

second commit 30da54f:

gerhardol · 2024-09-28T21:45:14Z

* test `CanGetAddedMovedDiffInfo` needs to be adapted

fixed (diff file was added as a displaycase for #119330 and I updated the file after your comments and missed to add this test to my playlist...)

The first commit does not detect / skip all moved lines. This is fixed up by the second commit. But the first commit should correctly handle all cases.

I would rather squash the commits, there are too many changes that will not be used.

mstv

I would prefer to split the PR after the second commit.

The second commit should remove ref found from GetBlockOfLines already.
maxLine rather is endLine.

mstv · 2024-09-29T18:36:51Z

src/app/GitUI/Editor/Diff/RangeDiffHighlightService.cs

+    public override bool IsSearchMatch(DiffViewerLineNumberControl lineNumbersControl, int indexInText)
+        => lineNumbersControl.GetLineInfo(indexInText)?.LineType is DiffLineType.Header;
+
+    public override string[] GetFullDiffPrefixes() => _diffFullPrefixes;


❔ Why move GetXxx behind IsYyy?

Do you mean the order of functions (unchanged, other code in between was changed) or the IsSearch call (FileViewer uses that)?

Do you mean the order of functions

Yes, (GetGitCommandConfiguration and) IsSearchMatch should have been / be moved behind GetFullPrefixes.

unchanged

Only visible in BC after manually matching the lines.

no change now

src/app/GitUI/Editor/Diff/DiffHighlightService.cs

gerhardol · 2024-09-29T20:13:23Z

I would prefer to split the PR after the second commit.

Can do that after #11862 is merged.

gerhardol · 2024-10-07T20:02:13Z

Rebased on #11862 dropped the "gap" part (except the refactoring) and the cache part.

src/app/GitUI/Editor/Diff/DiffHighlightService.cs

mstv

👍

gerhardol · 2024-10-17T21:31:53Z

Plan to squash and merge this in two days.
May separate the delimiters change.
As the first commit is not enough, I will squash the commits (do not bother to switch the order, that would fix this too)

Only parse the document for added/removed lines once, by using the DiffLineInfo info parsed for the linenumbering. Previously, the document was parsed many times every time the highlighting (including searching and marking) was changed. This simplifies the code.

Use heuristics to select which lines that are candidates to mark inlines for, avoid lines that Git has detected as moved lines.

gerhardol marked this pull request as draft September 27, 2024 22:58

mstv reviewed Sep 28, 2024

View reviewed changes

RussKie reviewed Sep 28, 2024

View reviewed changes

gerhardol mentioned this pull request Sep 28, 2024

Improve in-line diff highlight and add anchor markers #11862

Merged

mstv reviewed Sep 29, 2024

View reviewed changes

gerhardol force-pushed the feature/difflineinfo-coloring branch from 755675e to d8e6ef8 Compare October 7, 2024 19:53

gerhardol force-pushed the feature/difflineinfo-coloring branch from dc68fcc to a03ef88 Compare October 14, 2024 19:33

gerhardol marked this pull request as ready for review October 14, 2024 19:34

This was referenced Oct 14, 2024

feat: Cache the diff highlighting #11981

Merged

feat(difftastic): use VRulerPos for right hand diff #11982

Closed

mstv reviewed Oct 14, 2024

View reviewed changes

gerhardol force-pushed the feature/difflineinfo-coloring branch from 55f1758 to b49e180 Compare October 15, 2024 20:47

mstv approved these changes Oct 17, 2024

View reviewed changes

gerhardol added 3 commits October 18, 2024 22:56

chore: Sort Delimiters

b9a2004

feat: Avoid coloring moved lines (gitextensions#11947)

58af311

Use heuristics to select which lines that are candidates to mark inlines for, avoid lines that Git has detected as moved lines.

gerhardol force-pushed the feature/difflineinfo-coloring branch from 0a9358a to 58af311 Compare October 18, 2024 20:56

gerhardol merged commit 38d2fe4 into gitextensions:master Oct 19, 2024
4 checks passed

gerhardol deleted the feature/difflineinfo-coloring branch October 19, 2024 19:34

RussKie added this to the 5.1 milestone Oct 29, 2024

		public override void AddTextHighlighting(IDocument document)
		=> document.MarkerStrategy.AddMarkers(_textMarkers);


		namespace GitUI.Editor.Diff;

		public struct Segment : ISegment

Uh oh!

feat: Avoid coloring moved lines #11947

feat: Avoid coloring moved lines #11947

Uh oh!

Conversation

gerhardol commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Test methodology

Merge strategy

Uh oh!

mstv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstv commented Sep 28, 2024

Uh oh!

gerhardol commented Sep 28, 2024

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gerhardol commented Sep 29, 2024

Uh oh!

gerhardol commented Oct 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

gerhardol commented Oct 17, 2024

Uh oh!

Uh oh!

Uh oh!

gerhardol commented Sep 27, 2024 •

edited

Loading

mstv left a comment •

edited

Loading