Add diff options as icon buttons

[stdout-se]

Overview

Adds toolbar controls to the right side of the diff file header for common diff display options, with matching entries in Preferences -> Code.

Toolbar

• Word wrap (toggle) — existing preference; new shortcut in the header (still available from the diff context menu and Preferences).
• Formatting marks (toggle) — show tab and space characters in the diff text.
• Comparison method (menu) — choose how Git compares lines when showing a patch in the diff view.

Preferences

New settings under Code:

• Show formatting marks (tabs and spaces)
• Comparison method (radio list, same four modes as the header menu)

(Note: Word wrap was already in Preferences, no change to this)

Comparison method

Controls how git diff is run when loading a patch for the diff view. Options map to Git’s built-in flags:

Option	Git flags
Recognize line endings and white space differences (this is the default)	None
Ignore line ending differences	--ignore-cr-at-eol
Ignore line ending and white space length differences	--ignore-cr-at-eol, --ignore-space-change
Ignore line ending differences and all white space differences	--ignore-cr-at-eol, --ignore-all-space

Note: Comparison method affects how patches are displayed on-screen only. This does not affect exporting patches, reverting files, etc. This still uses the precise methods.

Changing the comparison method reloads the current patch (re-runs git diff). Word wrap and formatting marks apply immediately without reloading.

Icons

Notes about icons I have used: All icons are MIT Licensed according to these pages:
Update: See comments below.

https://www.svgrepo.com/svg/361077/clear-all
https://www.svgrepo.com/svg/361075/circle-slash
https://www.svgrepo.com/svg/361072/circle-large-filled
https://www.svgrepo.com/svg/361073/circle-large-outline
https://www.svgrepo.com/svg/361403/whitespace
https://www.svgrepo.com/svg/361405/word-wrap

[stdout-se]

Reference link: https://git-scm.com/docs/diff-options#Documentation/diff-options.txt-ignore-space-at-eol

[stdout-se]

License update: I found the proper reference repo for the icons:
Reference links:
https://github.com/microsoft/vscode-codicons
https://microsoft.github.io/vscode-codicons/dist/codicon.html

License:
https://github.com/microsoft/vscode-codicons/blob/main/LICENSE

While the license for the code in the repo is MIT, the icons are under The license is Creative Commons Attribution 4.0 International. Not great. Not terrible, I guess.

[jorio]

Super neat!! I've wanted to do this for a long time, this'll be sure to make people happy. It looks well polished from what I can see. Awesome!

I'll cook up some SVG icons by hand so we don't have to worry about licensing.

Please, can you convert your identifiers to camelCase for consistency with the rest of the code? (The only place with snake_case is porcelain.py because it's an extension to pygit2; the rest of the code more or less follows Qt's naming conventions.)

I will review the rest of your code in detail soon. Thank you for your hard work!

[stdout-se]

Fixed name case, also added a simple test for the formatting marks button

[jorio]

I expanded on your work a bit:

• Specific "no changes" message if a whitespace-only change was detected
• Moved the buttons into their own container class, which will make it easier to add new buttons in the future (e.g. controls for the amount of context lines)
• Lower contrast whitespace marks
• Fixed issue where syntax highlighting tokenization got out of sync with the actual text, in the case of a re-indented line with --ignore-all-space
• Replaced third-party icons with my own so there are no licensing issues to worry about

Thoughts on the UI:

1. What do you think about simplifying the whitespace options to a single on/off toggle for --ignore-all-space? I feel that the other options aren't as useful and weigh down the UI a bit.

   In particular, I find --ignore-space-change very tricky to get across intuitively in the UI, so I suggest getting rid of it (unless you have a use for it yourself?)

2. I'm pondering whether the CR option should really be joined at the hip with the whitespace options. I wouldn't want to encourage novices to check in a mess of mixed LF/CRLF line endings instead of configuring core.autocrlf properly. Do you have a use for --ignore-cr-at-eol?

3. Now that there's a dedicated button to toggle word wrap, is "word wrap" in DiffView's context menu redundant?

---

Notes on your implementation:

1. The QTextOption.Flag <-> int conversion boilerplate is unnecessary. Bitwise ops like flags &= ~QTextOption.Flag.ShowTabsAndSpaces work just fine in all 3 supported Qt bindings.

2. I moved _applyFormattingMarksTextOption to CodeView (DiffView's base class). This way, both diff and blame views get support for formatting marks.

3. In your tests, the waits are superfluous. (Waiting is only needed when you need Qt to process the event queue, or when the test explicitly enables async tasks with the taskThread fixture. Tasks are async in the real world, but most unit tests enforce synchronous execution by default for determinism.)

4. When you create your own widgets, instead of looking them up by name in tests, just use the attribute wherever possible.

5. I removed the _restoreComparisonMethodAfterTest fixture because the mainWindow fixture ensures we start with fresh settings.

6. I removed RepoWidget.reloadCurrentPatchFromPrefs and reverted to the previous version of MainWindow.onAcceptPrefsDialog. I'll admit that calling onAcceptPrefsDialog is not the most elegant way to trigger just a patch refresh, but it does the job. It is overdue for a cleanup, but that's outside the scope of the PR.

[cornelius-dol]

Just wanted to chime in on this.

In particular, I find --ignore-space-change very tricky to get across intuitively in the UI, so I suggest getting rid of it (unless you have a use for it yourself?)

I agree with difficulty in communicating what this actually does, since I am not sure from the way it's presented. But I want to note that it is immensely valuable to be able to ignore whitespace changes that merely change indentation and/or alignment, vs. those that are material, say inside a string or regex.

I'm pondering whether the CR option should really be joined at the hip with the whitespace options. I wouldn't want to encourage novices to check in a mess of mixed LF/CRLF line endings instead of configuring core.autocrlf properly. Do you have a use for --ignore-cr-at-eol?

IMHO, this should be separate; tools should accommodate their users, not the other way around. If some dev has checked in different line endings from what I am using and my OS/Compiler/etc doesn't care about line endings, then neither do I. I would want to permanently have line-ending differences ignored, no matter what whitespace option I have enabled.

[stdout-se]

Still getting used to QT, lots to learn 😄

@jorio
What do you think about simplifying the whitespace options to a single on/off toggle for --ignore-all-space? I feel that the other options aren't as useful and weigh down the UI a bit.

In particular, I find --ignore-space-change very tricky to get across intuitively in the UI, so I suggest getting rid of it (unless you have a use for it yourself?)

I use all these options in different repos, so for me they're useful. 😄 The options are just an interpretation of what I've seen in different diff tools, I don't particularly like how the experience works but I haven't been able to figure out a better UI/UX.

There might be a better layout where the options are presented as an increasing level of ignore? "Ignore nothing, ignore some things, ignore more things, ignore the most things." If you get what I mean.

The documentation probably needs to show some good examples to the users. Need to think about that. But can be its own task maybe.

@jorio
Do you have a use for --ignore-cr-at-eol?

There are some messy repos where it's quite useful. There are some cases I activate it depending on what I'm reviewing in different stages, and sometimes I just come in to review architecture or safety and don't care about code style. For reviewing codes style, you need all whitespace and newlines to be visible, but in other cases you're reviewing on a higher level and it's just noise. I think there are a lot of developers that use this with the motivation "Don't break my workflow". 😄

I've also worked as a teacher where I review student code, and it's very useful to focus on the high level stuff. I don't expect beginner programmers in a classroom to worry about CR/LF/CRFL.

@cornelius-dol
I would want to permanently have line-ending differences ignored, no matter what whitespace option I have enabled.

Could be a good structure, yes.

@jorio
Now that there's a dedicated button to toggle word wrap, is "word wrap" in DiffView's context menu redundant?

I think either show all options, or remove word wrap from that menu.

@jorio
The QTextOption.Flag <-> int conversion boilerplate is unnecessary. Bitwise ops like flags &= ~QTextOption.Flag.ShowTabsAndSpaces work just fine in all 3 supported Qt bindings.

Not sure why I had trouble with this, it was a hack to get around something. Good that it could be removed.

[jorio]

You both made good points, so I kept all options, with one caveat:

After experimenting a bit, I found out that --ignore-cr-at-eol seems to be "implied" by -b and -w.
git diff treats an LF→CRLF change as a modification in trailing whitespace length, so, both -b and -w already ignore LF→CRLF at EOL.
So: I kept --ignore-cr-at-eol, but made it mutually exclusive with -b and -w.
(Let me know if I'm interpreting git's behavior incorrectly here!)

I also added additional controls to the button strip: a spinbox to tune the amount of context lines and an SVG preview toggle. I've had these controls in the back of my mind for years, thank you Andreas for initiating this.

I merged all this into master, but feel free to chime in if this still needs tweaking before I flesh out the documentation and cut a new release.