Add navigation and highlighting of change-blocks in Text Diff (#616, #717)

[goran-w]

Related issues:

• Initial suggestion: Add navigation to "Prev/Next Change" for built-in Diff viewer #616
• Follow-up suggestion: Allow (optional) prev/next change navigation within visible page in Text Diff #717
• Converted to discussion: Allow (optional) prev/next change navigation within visible page in Text Diff #750

Status:

• This PR is now ready for Merge, after discussion and re-implementation.
• I made a "more detailed" implementation, based on how most external Diff/Merge tools handle prev/next change navigation, while @love-linger commited a "simpler" implementation (from the initial suggestion) on the develop branch (134c710, 882878d), released in v8.39.
• I've added a way to choose between these two implementations, via a ToggleButton "Highlighted Diff Navigation" in DiffView toolbar. The choice is stored as a Preference, which defaults to the simpler implementation by @love-linger.
• Keyboard shortcuts could be added as a future enhancement.

Quick feature overview of the new feature:

• Implemented a feature for navigating to and highlighting change-blocks (i.e groups of consecutive added/deleted lines), by toolbar buttons (arrow up/down for prev/next change-block).
• Repeated navigation to first/last change-block will simply scroll there (and move the caret) again.
• Numbers for the "current change-block" and "total number of change-blocks" are displayed in the toolbar.
• This feature has minimal impact on existing Diff functionality, as the "current change-block" is initially unset (for refreshed Diff) and will not be activated until clicking on the prev/next buttons.
• Performance-wise, this functionality adds a call to ProcessChangeBlocks() upon creating a TextDiff / TwoSideTextDiff object. That method does a single loop over the Diff Lines and collects start/end line numbers of each change-block, so does not add much performance cost compared to the work already being done.

Detailed summary of changes:

• Added Preference.EnableChangeBlocks and a ToggleButton "Highlighted Diff Navigation" in DiffView toolbar, for switching between the default "simpler" change-navigation style and this new feature.
• Modified behavior of the "Previous/Next Difference" buttons in DiffView toolbar, when enabling the new feature.
• Added display of "current/total" change-block numbers in DiffView toolbar, visible only when enabling the new feature.
• Added method ToggleTwoSideDiff() in ViewModels.DiffContext, so the change-block indicator can be refreshed there.
• Made Models.TextDiff and ObservableObject, for propagating property changes.
• Added new class TextDiffChangeBlock, to keep track of each change-block from the Diff.
• Added property ChangeBlocks and method ProcessChangeBlocks() in Models.TextDiff and ViewModels.TwoSideTextDiff.
• Added properties CurrentChangeBlockIdx (and associated Styled Properties where needed) in Models.TextDiff, TextDiffView and ThemedTextDiffPresenter.
• Added virtual method GetChangeBlock() in TextDiffPresenter and derived classes.
• Added method ThemedTextDiffPresenter.GetCurrentChangeBlock().
• Added method ThemedTextDiffPresenter.JumpToChangeBlock() which moves TextArea.Caret to the first line of the current change-block and scrolls it into center of view.
• Added Class Handler for CurrentChangeBlockIdxProperty.Changed in TextDiffView, to call JumpToChangeBlock() on descendant ThemedTextDiffPresenter views.
• Added code in LineBackgroundRenderer.Draw() to highlight the current change-block area (if any) with gray-tinted background rectangle(s) and a gray border line at top & bottom.
• (Corrected a misspelled local variable: nextHigh(t)light.)

[love-linger]

I'm very sorry. After checked your implementation, I think we should use another implementation because:

• Chunk/Hunk is only necessary to calculate if changes in uncommitted local changes when user hover it.
• Jumping to a different Chunk/Hunk within the current visible lines is not necessary in my opinion.

I'd like to implement this feature by just modify DiffView.axaml. Taking Goto next change as an example:

// Step 1. Find the first text diff view editor (also works with side-by-side mode since two sides changes are aligned and we sync the scroll offset between them)
var textDiffView = this.FindDescendantOfType<ThemedTextDiffPresenter>();
if (!textDiffView)
{
    // Do nothing since it is not a text diff result.
    return;
}

// Step 2. Find the last line of current editor.
// Step 3. Get the index of that line in Models.TextDiff
// Step 4. Find the next change after that line.
// Step 5. Scroll to that line.

I'll try to implement this by myself later.

[goran-w]

@love-linger I see. And yes, my change-blocks could be chunks/hunks instead, as long as they are well-defined and can be counted.

However:

• We should still highlight the current change-block (or chunk/hunk).
• We should then also detect when the Caret enters another change-block.
• Prev/next should NOT jump to the prev/next change-block off-screen (as you seem to suggest), but instead to the prev/next change-block "globally" (which may still be on-screen)!

This is how the major Diff/Merge tools out there implement prev/next. My implementation was a slight simplification / first effort since it does not (yet) track when Caret moves to another change-block (instead it only moves the Caret when navigating, so change-block <-> Caret interaction is essentially one-way).

Also, I did not add keyboard shortcuts yet, but that should definitely be added in a later PR to make this VERY useful. (Using the mouse is not the fastest possible workflow)

Ideally, in future we should also display "current / total" number of change-blocks and have "first / last" navigation as well. , similar to this screenshot from the Plastic SCM Mergetool (which is what I come from and am used to) :

[love-linger]

I still insist that when we click the previous or next button, we should find the target change directly from the first or last line of the current page, rather than by the current Caret position.

• The code editor in the text diff view only provides Read-Only mode now. The cursor can only be used for selection.
• I must say it again that it is unnecessary to jump changes within current page.
• Hunk detection in SourceGit is only a suggestion, or a target that the program automatically selects for the user. I do not intend to promote or emphasize the definition of Hunk in this software. Users can choose parts of a Hunk or operate across multiple Hunks.

[love-linger]

I've pushed my implementation of this feature.

[goran-w]

@love-linger Thank you, but your pushed implementation does not help me much. It's basically an enhanced page-up/down that knows about changes. I could easily lose track and miss changes by using that implementation, since there is no highlighting.

What I want is a much more exact tracking of the change-blocks (ideally with the "current / total" numbers visible) and with the kind of highlighting I implemented, so that I can really step through each one and review them. (Thus, in my opinion, navigating change-blocks within visual lines is still crucial.)

Also, my change-blocks were not based on your Hunk detection. Instead it uses the kind of change-blocks used in most external Diff/Merge tools, where "unchanged lines break the sequence" - that definition is very simple. (The two implementations should be able to coexist without interfering, since the Hunk interactions are based on mouse hover and can be overlayed on top of the change-block highlight.)

I do not view Caret tracking as crucial for now, and I have no issue with the Diff view being read-only. (Although future edit functionality - as in Plastic SCM's Pending Changes view - would be awesome!)

[goran-w]

NOTE: I rebased and updated the branch in my fork to adjust to your changes on develop, but my implementation is currently inactive since the up/down buttons now use your implementation.

@love-linger Just curious - did you actually run my implementation to try it out, or did you just review the code?

[love-linger]

Just curious - did you actually run my implementation to try it out, or did you just review the code?

I just read your code.

[goran-w]

@love-linger It would be nice if you at least tried it out, to make sure you understand the kind of highlighting and navigation I'm looking for. 🥺 (I could re-enable my implementation on my rebased branch, so you can try it.)

[love-linger]

I've tried your implementation.

As I said, when we click the previous or next button, we should find the target change directly from the first or last line of the current page.

If you have scrolled the highlighted block out of current page manually. The next time you click the prev or next button, it will always starts from the last highlighted block, just like back-scroll. And user can not change the hightlighted block in any other ways.

[goran-w]

@love-linger I'm not sure I understand your point.

In my implementation, the thing missing right now is the display of counters "current / total" which would make it more clear even if you've scrolled the highlighted block out of view. I could add that too.

I find it super useful to be able to see at a glance how many changes there are in a file, and to step through them in a very controlled manner like this (while highlighting the current one).

Could we at least make this an optional feature?

[love-linger]

for example: there are 5 blocks and each block taking a single page and you're currently highlighting the 1st block (at the first page). then you scroll (or use page down) to the 4th page where the 4th change block exists. when you click the next button, the expected block is 5th.

[goran-w]

@love-linger I see what you mean - but scroll and (Pg)Up/Dn etc are actually different, since scroll does not move Caret while (Pg)Up/Dn does (same for clicking to place Caret etc).

The way Plastic SCM Mergetool handles this quite elegantly is to track Caret but not scroll. I.e. moving the Caret will modify the highlight and "current / total" display, while scrolling won't. So navigating to prev/next change will always be based on the "current" number.

If we add the "current / total" numbers display it would IMHO become clearer, even without tracking Caret which can be added later as an enhancement.

[love-linger]

I will think about it carefully tomorrow. At present, the available space in the toolbar is very tight. It is not recommended to add too many functions. Including this feature, it has been implemented in the early version, and was deleted after adding support for external diff tool

[goran-w]

@love-linger Thank you. Meanwhile, I added a "current / total" numbers display in the toolbar, to illustrate my reasoning.

[goran-w]

BTW: if needed, horizontal space in the Diff toolbar could be regained by moving "toggle" options into a dropdown. Also, I usually go for a two-sided diff - then the side-by-side diff panes themselves need more horiz space than the toolbar buttons do...

[love-linger]

I still don't quite like this function.

1. I have mentioned many times about how to determine the previous or next change. I insist that the first line of the currently displayed content should be used to find the previous one and the last line to find the next one. In this way, no matter whether I use buttons, mouse scrolling or paging functions, when I click the "previous" or "next" button again, its behavior is always consistent.
2. Jumping within the currently displayed content is completely unnecessary. For example, in the following view, I need to click multiple times.

3. If you think that the comparison view function provided by the software is missing or different from the operation habits of the software you usually use. SourceGit retains the ability to use external tools.

[goran-w]

@love-linger I fully understand your preference when it comes to navigation, it's just not the way I prefer to work. If you look at P4Merge, WinMerge and others they do navigation within the displayed content, but I should not have to resort to external tools for that - it would mean extra clicks and loading times for EACH file, which slows down workflow greatly.

In my PR I've begun providing the means for making this a preference option. Your implementation could be the default, and the current/total numbers indicator can be hidden in that case.

[goran-w]

Besides, being able to see at a glance a number (current/total) of how many change-blocks there are in a Diff is IMHO a great feature in itself (slightly related to #647).

[goran-w]

To summarize: I would use the external diff tool only for merge-conflict resolution (4-way merge) and for especially complex diffs, but for daily routine Diff I would use the built-in Diff since it's much faster to work that way. Therefore, the built-in Diff has to cover the basic needs of my workflow, which revolves around being able to step through and highlight each change-block for review before commit (usually in a side-by-side Diff view). If I'm not able to step through multiple blocks on a single screen/page, I might lose track and miss a change. I'm using the current/total numbers to see how many blocks there are and where I'm at while reviewing the changes. I'm fine with having this as an optional workflow feature, that can be turned on via Preferences.

[goran-w]

@love-linger I reworked the feature implementation in this PR. It's now turned off by default, but can be toggled on by a button in the DiffView toolbar. This toggle is saved as a Preference. Ready for review/merge.

[goran-w]

Rebased PR to develop branch, after Release 8.41.

[goran-w]

@love-linger :

2. Jumping within the currently displayed content is completely unnecessary. For example, in the following view, I need to click multiple times.

Actually, when jumping to a "change-block" even within the currently displayed content, if the change-block is close to the top/bottom of the displayed content it starts scrolling it closer to the vertical center in order to provide more context (the ScrollToLine() method has this nice feature). This makes for a nice and smooth navigation through the set of changes, regardless of the "Show All Lines" toggle. I don't mind clicking multiple times - it helps me keep track of each individual block of changes (along with the counter that I added). This is how I'm used to working, from a whole range of other tools - Plastic SCM, TortoiseMerge, P4Merge, Meld etc... They all do this kind of stepping and highlighting inside the currently displayed content.

The highlighting helps me keep visual track of which change I'm focusing on. In two-side-diff it also helps me see clearer the coupling between empty/added/removed lines on left/right side.

The current/total numbers help me keep track of how many changes there are in a diff, and helps me keep track of which once I've looked at.