Syntect highlighting experiment #73

CptPotato · 2023-09-22T11:51:33Z

Adds optional syntax highlighting based on syntect to hunk diffs.

screenshots

with background color:

Remaining things to fix/consider (not all changes are pushed to the branch, yet):

Piturnah · 2023-09-22T17:34:18Z

Looking amazing so far, thanks for opening the PR!

Can't wait for this to be ready for review, if you stick with it.

- refactor highlighting.rs - add parsed Hunk struct - remove existing highlighting

CptPotato · 2023-09-24T14:50:34Z

The code is starting to look a little better, though there's still a few things to do.

I made the syntax highlighting optional (still need to fix the config, though). When disabled it uses the current look.

Also, I tried to see how it looks when only the selected hunk is highlighted:

screenshot

I personally like it, what do you think? I could also see if I can make this optional.

Piturnah

Left a few notes/questions! Don't mean to step on your toes too much since I know this is a draft PR. Really appreciate the work you're doing here.

Also, I tried to see how it looks when only the selected hunk is highlighted:

I'm a big fan of the look and feel of this. I do think this functionality should be configurable, but also I think it should be the default behaviour for now (time will tell).

I wonder if the whole file diff should be highlighted when the file is selected (rather than a hunk under the file) as well.

Piturnah · 2023-09-25T10:34:26Z

src/config.rs

@@ -64,6 +65,8 @@ impl Default for Options {
            lookahead_lines: 5,
            truncate_lines: true,
            ws_error_highlight: WsErrorHighlight::default(),
+            // TODO: decide on default


Ideally, I think the default syntax highlighting theme should come from the terminal's theme (just as the default gex colour theme does). This should make life a bit easier for users as some colour coherency is ensured out of the box.

I am not sure of the feasibility of this since Syntect only accepts colors as RGBA. This StackOverflow answer suggests that in at least xterm you can query the current terminal colors, but I haven't investigated how cross-platform this approach is, and I don't think it's supported in Crossterm.

This will be a decent amount of work to look into and is definitely not blocking for this PR - but my gut feeling is that as long as this isn't possible, then syntax highlighting should be disabled by default. Let me know your thoughts.

I'm not sure if this is a viable solution since, as you mentioned there doesn't seem to be a reliable way to get the terminal's colors.

I think a "better" way could be creating a custom syntect theme and then map the Styles back to the 16 color ANSI sequenses and display them using crossterm. But I don't think I'll explore that option in this PR for now.

Either way, I agree that gex should be usable out-of-the-box even on terminals that don't support true-color / 24 bit colors. So yeah, the highlighting should be disabled by default.

Piturnah · 2023-09-25T10:38:20Z

src/highlight.rs

+
+    // workaround: remove trailing line break
+    buf.pop();
+    buf.push_str("\x1b[0m");


Haven't investigated but my suspicion is that resetting attributes this way will break things if the user has a custom background color set. The render::terminal module provides a ResetAttributes that should do what you want (if it doesn't then that's probably a bug with ResetAttributes).

Piturnah · 2023-09-25T10:38:56Z

src/highlight.rs

+    buf.pop();
+
+    // according to docs of `as_24_bit_terminal_escaped`
+    buf.push_str("\x1b[0m");


See prior comment

Piturnah · 2023-09-25T11:00:09Z

src/highlight.rs

+    syntax: &SyntaxReference,
+) -> String {
+    // TODO: move somewhere else?
+    let marker_added = as_24_bit_terminal_escaped(&[(MARKER_ADDED_STYLE, MARKER_ADDED)], true);


It is unclear to me why we use as_24_bit_terminal_escaped instead of the crossterm Command API

Piturnah · 2023-09-25T11:07:56Z

src/hunk.rs

+
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum DiffLineType {
+    Unchanged,


Bikeshed (not important): how do we feel about changing this variant to Context to bring it inline with git's documentation?

Sounds good, I'm not familiar with the exact naming in git so I just chose something. I'll also use "deleted" instead of "removed" since I think I saw these used in other parts of gex.

I didn't pay it much thought before but come to think of it, git actually uses "old" and "new" where in gex we use "deletion" and "addition", so it's already inconsistent. To be honest old and new don't sound as obvious to me personally, could just be my bias.

Anyway, this is far from an important point about the code so I will stop fixating on it 😅

Piturnah · 2023-09-25T11:10:19Z

src/hunk.rs

+            ' ' => Ok(Self::Unchanged),
+            '+' => Ok(Self::Added),
+            '-' => Ok(Self::Removed),
+            c => Err(anyhow!("'{c}' is not a valid diff type character.")),


In the case of an added trailing newline this will cause a bug (see #62)

CptPotato · 2023-10-02T17:09:57Z

Thanks for the first review, appreciate it! I'll probably take my time with this PR.

One problem I didn't notice until now is that highlighting each hunk in isolation is wrong, since it doesn't always produce valid syntax.
So I'll have to perform highlighting to the whole file first and then take the highlighted lines for each hunk from there. I'll probably cut some corners for now and only highlight the "new" part of the diff, which should be fairly easy. Looking at delta's output it actually seems to do the same thing. To highlight the deleted lines I'd have to reproduce the old version of the file first, which can be implemented at a later point if it's needed.

Piturnah · 2023-10-02T17:14:49Z

Thanks for the first review, appreciate it! I'll probably take my time with this PR.

Absolutely take your time. Thanks again for taking a look at this!

One problem I didn't notice until now is that highlighting each hunk in isolation is wrong, since it doesn't always produce valid syntax.
So I'll have to perform highlighting to the whole file first and then take the highlighted lines for each hunk from there. I'll probably cut some corners for now and only highlight the "new" part of the diff, which should be fairly easy. Looking at delta's output it actually seems to do the same thing. To highlight the deleted lines I'd have to reproduce the old version of the file first, which can be implemented at a later point if it's needed.

Seems good to me!

CptPotato added 2 commits September 22, 2023 13:47

syntect highlighting experiment

537364c

fix typo

820688e

CptPotato force-pushed the syntect-highlighting branch from dc8231a to 54cbb44 Compare September 22, 2023 14:33

move highlighting to separate module, change marker

9e63cbe

CptPotato force-pushed the syntect-highlighting branch from 54cbb44 to 9e63cbe Compare September 22, 2023 14:52

CptPotato force-pushed the syntect-highlighting branch from 8fbeb4f to e02e0e8 Compare September 23, 2023 17:48

refactor

7395f56

- refactor highlighting.rs - add parsed Hunk struct - remove existing highlighting

CptPotato force-pushed the syntect-highlighting branch from e02e0e8 to 7395f56 Compare September 23, 2023 17:50

highlight only the selected hunk

6fb54e1

CptPotato force-pushed the syntect-highlighting branch from c53520d to a7350fe Compare September 24, 2023 14:40

Refactor hunk display impl. Add highlight config.

17068a7

CptPotato force-pushed the syntect-highlighting branch from a7350fe to 17068a7 Compare September 24, 2023 14:49

Piturnah reviewed Sep 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syntect highlighting experiment #73

Syntect highlighting experiment #73

CptPotato commented Sep 22, 2023 •

edited

Loading

Piturnah commented Sep 22, 2023

CptPotato commented Sep 24, 2023

Piturnah left a comment •

edited

Loading

Piturnah Sep 25, 2023

CptPotato Oct 2, 2023

Piturnah Sep 25, 2023

Piturnah Sep 25, 2023

Piturnah Sep 25, 2023

Piturnah Sep 25, 2023

CptPotato Oct 2, 2023

Piturnah Oct 2, 2023

Piturnah Sep 25, 2023

CptPotato commented Oct 2, 2023

Piturnah commented Oct 2, 2023

Syntect highlighting experiment #73

Are you sure you want to change the base?

Syntect highlighting experiment #73

Conversation

CptPotato commented Sep 22, 2023 • edited Loading

Piturnah commented Sep 22, 2023

CptPotato commented Sep 24, 2023

Piturnah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CptPotato commented Oct 2, 2023

Piturnah commented Oct 2, 2023

CptPotato commented Sep 22, 2023 •

edited

Loading

Piturnah left a comment •

edited

Loading