Skip to content
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

regression: no more diff coloration #211

Open
Valentin271 opened this issue Jan 9, 2024 · 14 comments · May be fixed by #223
Open

regression: no more diff coloration #211

Valentin271 opened this issue Jan 9, 2024 · 14 comments · May be fixed by #223
Labels
A-theme Area: Themes, system theme detection, and syntax highlighting C-bug Category: Something isn't working

Comments

@Valentin271
Copy link
Contributor

Valentin271 commented Jan 9, 2024

In inlyne 0.3.3, ```diff were rendered as follow (the whole line colored).
image

With inlyne main, ```diff renders as follow (only + colored).

image


After more research, it looks like commit 04ef6e1 introduced this behavior.

I can probably take a look at that but not in the immediate future.

@CosmicHorrorDev
Copy link
Collaborator

Just guessing from where things regressed, but it's probably just that the new default light theme doesn't support highlighting diffs correctly (which isn't surprising because the themes embedded with syntect are of mixed maintenance status)

We have access to a lot more themes through two-face now (thanks to bat), so it would probably be good to evaluate and switch over to (potentially a subset of) those themes

@CosmicHorrorDev CosmicHorrorDev added C-bug Category: Something isn't working A-theme Area: Themes, system theme detection, and syntax highlighting labels Jan 9, 2024
@Valentin271
Copy link
Contributor Author

Good to know, i'll take a look at it when possible.

I thought of the theme support too but the colored + threw me off. I'll check and see.

@Valentin271
Copy link
Contributor Author

I did a little bit of research on that today, I can confirm this simply comes from InspiredGithub theme not supporting diff correctly.

I think we should switch to using two-faces theme so people have more choice.

Do you remember why you switched to InspiredGithub? I think default dark and light color_highlighter should be variants of the same theme (e.g. Base16Ocean{Dark,Light}, OneHalf{Dark,Light}).

@CosmicHorrorDev
Copy link
Collaborator

We originally switched away from a light/dark variant theme due to the light theme having very low-contrast colors (Base16Mocha IIRC)

I picked inspiredgithub since it seemed to have much better contrast, but that was before we added two-face. I've got some free time today, so I can work on integrating two-face's themes

@Valentin271
Copy link
Contributor Author

I thought Base16OceanLight had low contrast too.
I tried some themes today, I think monokai and OneHalf (but diff colors are too muted on this one imo) are the best options (that have variants, support diff and have decent colors overall).

Here is a quick comparison:
image

@CosmicHorrorDev
Copy link
Collaborator

#219 exposes a lot more themes if you want to take a look at those too

Also worth noting that it looks like a lot of themes mess up that diff highlighting, but we can always patch individual themes to fix little issues like that (depending on how hard that ends up being)

@Valentin271
Copy link
Contributor Author

Valentin271 commented Jan 22, 2024

I already tried those by changing the code directly (see my comparing screenshot), tried again and monokai is definitely the best imo.

we can always patch individual themes

We could also fix this upstream, currently it's an issue in bat, two-face and inlyne at the very least.
I can probably take a look at this.

@CosmicHorrorDev
Copy link
Collaborator

I already tried those by changing the code directly (see my comparing screenshot), tried again and monokai is definitely the best imo.

Ah apologies, that does make more sense now. I agree that monokai is likely the best (although I do really like Coldark-Cold/Coldark-Dark too)

We could also fix this upstream, currently it's an issue in bat, two-face and inlyne at the very least.
I can probably take a look at this.

I agree, patching upstream is the path of least resistance in the long run. bat mostly just aggregates different themes from different repos (themes within https://github.com/sharkdp/bat/tree/master/assets) along with having the ability to patch individual files (the patches dir in that same link). I'm not sure what the criteria is on when patching is okay vs when to fix upstream and then update the submodule in bat

Also worth noting that I'd like to start adding tests for individual themes+syntax highlighting within two-face itself so that we can prevent regressions in the future without having to test everything downstream here

@Valentin271
Copy link
Contributor Author

although I do really like Coldark-Cold/Coldark-Dark too

True, they do look good too. The darker background on light theme helps a lot. Let's go for that.

Also worth noting that I'd like to start adding tests for individual themes+syntax highlighting within two-face itself so that we can prevent regressions in the future without having to test everything downstream here

Yeah that would be a good idea. Just looking at the example you provided in the readme, looks like this could be easily tested with insta.

@Valentin271
Copy link
Contributor Author

So I've played around with the values of the github theme, there is actually nothing to patch as the style is present (e.g. markup.deleted) and even works perfectly.

Long story short I ended up using a color pick on the letters of a diff and ... yeah it's the right color, it's just so close to black it's hard to see.

The actual problem is that the github theme uses background color for diffs, and with them it would be way easier to see the colors.
I don't know how hard it would be to support background colors for code block, probably somewhat internal to syntect.

@CosmicHorrorDev
Copy link
Collaborator

(The good news is that means a lot of the foundational work is already supported. I can elaborate more after I get off of work)

@CosmicHorrorDev
Copy link
Collaborator

Alright, so the good news is that the background color for the individual lines is already included in the HTML that comrak gives us which means that both synctect and comrak are doing their individual things correctly

```diff
+ Ins
- Del
```

gives us

<pre style="background-color:#ffffff;"><code class="language-diff"><span style="background-color:#ddffdd;color:#003300;">+ Ins
</span><span style="background-color:#ffdddd;color:#770000;">- Del
</span></code></pre>

and we already support parsing that, so we just need to add an arm for Style::BackgroundColor here

https://github.com/trimental/inlyne/blob/5bca55d504d57d7f1eba8cfecbc096ca3bfc0a80/src/interpreter/mod.rs#L489-L497

That gets all of the easy stuff out of the way which finally leaves the hard parts of

  1. Storing that value with the individual crate::text::Text snippets and finally
  2. Actually rendering the background color of crate::text::Text's individually since it's currently for entire crate::text::TextBoxes instead

@CosmicHorrorDev
Copy link
Collaborator

I've gone ahead and merged #219, so we can handle switching the default dark/light syntax highlighting theme if you think there's still enough reason to do so (beyond the bug with inlyne not supporting background color on spans)

@Valentin271
Copy link
Contributor Author

Valentin271 commented Jan 24, 2024

If we can fix the background color in code blocks and the diff/colors looks ok, then there should be no reason to change the default theme.

I've quickly looked at supporting that, I think I can do it I just couldn't really find where the background color was actually drawn.

@Valentin271 Valentin271 linked a pull request Jan 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Themes, system theme detection, and syntax highlighting C-bug Category: Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants