-
Notifications
You must be signed in to change notification settings - Fork 267
Add navigation and highlighting of change-blocks in Text Diff (#616, #717) #703
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
Add navigation and highlighting of change-blocks in Text Diff (#616, #717) #703
Conversation
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.
@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) :
image
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 ofHunk
in this software. Users can choose parts of aHunk
or operate across multiple Hunks.
I've pushed my implementation of this feature.
bd9896d
to
57e147e
Compare
@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!)
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?
Just curious - did you actually run my implementation to try it out, or did you just review the code?
I just read your code.
@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.)
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.
@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 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 Thank you. Meanwhile, I added a "current / total" numbers display in the toolbar, to illustrate my reasoning.
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...
I still don't quite like this function.
- 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.
- Jumping within the currently displayed content is completely unnecessary. For example, in the following view, I need to click multiple times.
@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.
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).
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.
b62c484
to
c5dc25b
Compare
807cec8
to
400aaac
Compare
c5dc25b
to
a3ab326
Compare
ec84369
to
e65ac18
Compare
29dd853
to
b0a7593
Compare
@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.
b0a7593
to
908b982
Compare
Rebased PR to develop
branch, after Release 8.41.
* Modified behavior of the Prev/Next Change buttons in DiffView toolbar. * Well-defined change-blocks are pre-calculated and can be navigated between. * Current change-block is highlighted in the Diff panel(s). * Prev/next at start/end of range (re-)scrolls to first/last change-block (I.e when unset, or already at first/last change-block, or at the only one.) * Current change-block is unset in RefreshContent().
908b982
to
904ec16
Compare
- 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.
655d71b
into
sourcegit-scm:develop
* change the name of this feature to `Enable Block-Navigation` * change the icon of the toggle button used to enable this feature * use a new class `BlockNavigation` to hold all the data about this feature * create `BlockNavigation` data only when it is enabled Signed-off-by: leo <longshuang@msn.cn>
BranchCM.MergeMultiBranches, CommitCM.MergeMultiple, MergeMultiple sourcegit-scm#793 CommitCM.Merge 2053ce0 CommitDetail.Files.Search 894f3e9 Diff.UseBlockNavigation sourcegit-scm#703 FileCM.ResolveUsing 3b5d873 Hotkeys.Global.Clone bea2a39 InProgress.CherryPick.Head e1df5c5 InProgress.Merge.Operating ef40e4b InProgress.Rebase.StoppedAt, Repository.Skip sourcegit-scm#790 InProgress.Revert.Head 93d9a04 Merge.Source 2504a52 WorkingCopy.CommitToEdit c136821
* localization: add missing de_DE keys BranchCM.MergeMultiBranches, CommitCM.MergeMultiple, MergeMultiple #793 CommitCM.Merge 2053ce0 CommitDetail.Files.Search 894f3e9 Diff.UseBlockNavigation #703 FileCM.ResolveUsing 3b5d873 Hotkeys.Global.Clone bea2a39 InProgress.CherryPick.Head e1df5c5 InProgress.Merge.Operating ef40e4b InProgress.Rebase.StoppedAt, Repository.Skip #790 InProgress.Revert.Head 93d9a04 Merge.Source 2504a52 WorkingCopy.CommitToEdit c136821 * localization: consistently use clone with 'k' for most other keys a more "germanized" version with a k is used rather than a c
Uh oh!
There was an error while loading. Please reload this page.
Related issues:
Status:
develop
branch (134c710, 882878d), released in v8.39.Quick feature overview of the new feature:
Detailed summary of changes: