Skip to content

Add toggle "Show entire file" for built-in Diff viewer #615

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.

Sign up for GitHub

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

Jump to bottom
Closed
goran-w opened this issue Oct 28, 2024 · 7 comments · Fixed by #652
Closed

Add toggle "Show entire file" for built-in Diff viewer #615

goran-w opened this issue Oct 28, 2024 · 7 comments · Fixed by #652
Assignees
@love-linger
Labels
suggestion We are still considering whether it is necessary to add this feature

Comments

@goran-w
Copy link
Contributor

goran-w commented Oct 28, 2024

The built-in Diff viewer has options to "Increase/Decrease Number of Visible Lines", with icon-buttons in its top (mini-)bar. If we want to show the entire file contents, we would have to press "Increase ..." a large number of times, which becomes very tedious.

Therefore, I'd suggest adding another icon-toggle for "Show entire file".

For reference, the "Fork" Git client has exactly this feature, and a similar feature can be found in GitSmart as a "Compact" toggle. GitKraken instead allows switching between 3 diff views: "Hunk" (unified, compact) / "Inline" (unified, whole file) / "Split" (side-by-side, whole file).
@goran-w goran-w mentioned this issue Oct 28, 2024
Closed
@love-linger
Copy link
Collaborator

duplicated #611

@love-linger love-linger self-assigned this Oct 29, 2024
@gadfly3173
Copy link
Contributor

we would have to press "Increase ..." a large number of times

Maybe change that 2 buttons to RepeatButton would be better? @love-linger

@love-linger
Copy link
Collaborator

we would have to press "Increase ..." a large number of times

Maybe change that 2 buttons to RepeatButton would be better? @love-linger

I don't think so. Because when the value of --unified parameter changed, SourceGit will re-generate the diff result by start a new git diff process, which costs too much to me.

I think it's a good idea to leave all these advanced operations to the external diff/merge tool.

@love-linger love-linger added duplicate This issue or pull request already exists suggestion We are still considering whether it is necessary to add this feature labels Oct 29, 2024
@goran-w
Copy link
Contributor Author

goran-w commented Oct 29, 2024 •
edited
Loading

Sorry about the duplicate. And I don't think the Increase/Decrease buttons need to be changed - they are fine as they are, and were never meant to be used for "show entire file".

However, I find it unfortunate to not have instead a separate option for that, and to have to resort to external tools for this when the built-in Diff is much more convenient for quick (re)viewing of changes. Having to popup an external tool takes extra time and disrupts the workflow - I would prefer to only use that for resolving merge conflicts. Viewing the whole file and being able to navigate quickly between changes (#616) would improve this tool a lot, IMHO.

I understand the need to leave more advanced stuff to external diff tools, but this seems like a rather "basic" addition - wouldn't it be even simpler to implement than the current partial/compacted view? The feature has already been requested by more than one person (hence the duplicate) and it's available in several other similar Git clients. (Would you be willing to accept a PR for this, eventually?)

@love-linger
Copy link
Collaborator

Of course you can submit a PR, but I'm afraid it is not a simple job:

  • We can not just use large number for --unified=N parameter. For example, I'm a game developer and using UnrealEngine. There are more than 170K+ lines in https://github.com/EpicGames/UnrealEngine/blob/release/Engine/Build/Commit.gitdeps.xml file.
  • There are many other source code files those were generated by tools, such as language bindings or library definations. Which may also have many lines in a single file.
  • The new implementation must be compatible with Hunk based selection and Patch generation which used to stage/unstage/discard partial changes in a single file.
  • Most fully-featured diff/merge tool will has a minimap in scrollbar to help people find the changes quickly with scrollbar.
  • I am concerned about the performance of the new implementation, both loading/building data and rendering data.

@goran-w
Copy link
Contributor Author

goran-w commented Oct 29, 2024

Thank you for this excellent feedback! The difficult part would be compatibility with Hunk/Patch code, as you say. Minimap is not necessary, whole file + navigation to prev/next change would still be a big improvement. As for very large files, I think we could accept slower performance for these as it would only affect the build and render of Diff when the new view-style is enabled.

@love-linger love-linger added enhancement New feature or request help-wanted Extra attention is needed and removed duplicate This issue or pull request already exists suggestion We are still considering whether it is necessary to add this feature labels Oct 29, 2024
@gadfly3173
Copy link
Contributor

gadfly3173 commented Oct 29, 2024 •
edited
Loading

I tried to view the file history and blame of https://github.com/infinilabs/analysis-ik/blob/master/config/extra_main.dic (398716 lines) in sourcegit, and it took about 2 seconds to open (i3-7350k), which seems to be not so unacceptable.

For files that are too large, we can treat them as binary files and not process them. Gitkraken will directly display Binary File instead of rendering text content for files larger than about 10MB.

@love-linger love-linger added suggestion We are still considering whether it is necessary to add this feature and removed enhancement New feature or request help-wanted Extra attention is needed labels Nov 4, 2024
@goran-w goran-w mentioned this issue Nov 4, 2024
Merged
@love-linger love-linger linked a pull request Nov 4, 2024 that will close this issue
feature: diff - toggle show all lines (#615) #652
Merged
love-linger pushed a commit that referenced this issue Nov 4, 2024
@goran-w
feature: diff - toggle show all lines (#615) (#652)
1a8acbf 
* Renamed 1 of 2 SyncScrollOffset props, for clarity

The property "SyncScrollOffset" in TextDiff is distinct from the one with the same name in TwoSideTextDiff. These two properties are used in separate (though slightly related) ways and are not really connected.

The one in TwoSideTextDiff is mainly used to keep the scroll-pos of the two SingleSideTextDiffPresenter views in sync (aligned), while the one in TextDiff is used only to preserve/reset the scroll-pos in the single CombinedTextDiffPresenter view when (re)loading Diff Content (so not really syncing anything).

To clarify this and to make the two properties more distinguishable, I renamed the one in TextDiff to simply "ScrollOffset".

* Added icon and string for "Show All Lines"

New StreamGeometry "Icons.Lines.All" using SVG path from "text_line_spacing_regular" at https://avaloniaui.github.io/icons.html.
New String "Text.Diff.VisualLines.All" for en_US locale (no translations yet).

* Implemented new TextDiff feature "Show All Lines" (toggle)

* Added new ToggleButton in DiffView toolbar, visible when IsTextDiff, disabling the buttons "Increase/Decrease Number of Visible Lines" when on.

* Added new Preference property "UseFullTextDiff".

* Added StyledProperty "UseFullTextDiffProperty" in TextDiffView, with a DataTemplate binding to the corresponding preference property.

* When changed, UseFullTextDiffProperty is handled identically as UseSideBySideDiffProperty (via new helper method RefreshContent(), for unification with OnDataContextChanged()).

* Added new method DiffContext.ToggleFullTextDiff() for changing the preference property and reloading the diff content.

* Implemented the new feature by overriding the "unified" (number of context lines) for Commands.Diff() with a very high number.

NOTE: The number used (~1 billion) is supposed to be the highest one working on Mac, according to this forum comment: https://stackoverflow.com/questions/28727424/for-git-diff-is-there-a-uinfinity-option-to-show-the-whole-file#comment135202820_28846576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@love-linger love-linger

Labels
suggestion We are still considering whether it is necessary to add this feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feature: diff - toggle show all lines (#615) goran-w/sourcegit
3 participants
@love-linger @gadfly3173 @goran-w