danth / stylix

System-wide colorscheming and typography for NixOS
https://stylix.danth.me/
MIT License
904 stars 105 forks source link

zathura: add transparency to highlight colors #394

Closed jghauser closed 4 weeks ago

jghauser commented 1 month ago

This adds transparency back to the zathura module as discussed. I'm new to nix, so let me know if there's something to improve :).

Closes: #392

jghauser commented 1 month ago

Thanks for the feedback!

  1. Reduce the let-in scope by adding it to programs.zathura.options = let ... in {.

Ok!

  1. Since this is an upstream breaking change, Stylix should not enforce the previous deprecated behaviour by default. Add a Stylix option for declaring zathura's highlight-transparency option. Ideally, the example value should be set to the previous default value. Could you maybe find the zathura commit introducing this breaking change and use the exact value? It would also be good for reference, to link the commit in this PR and the description of the new option.

Here's the relevant commit in zathura. I now realise I was wrong with the 0.3, it's actually 0.5 and I'll change that. But it looks like the zathura people didn't intend this to change the default behaviour, so the commit that removes highlight-transparency also adds 0.5 transparency to all the relevant defaults. Should I still add an option in stylix even if zathura's change was more of a refactor than a change in behaviour?

EDIT: I now also realise that there's a 3rd option to which they added transparency, highlight-fg, which "Defines the color that is for text when highlighting parts of the document (e.g.: numbers for links)". I'm not entirely sure what it's for, but by default it's set to rgba(0,0,0,0.5). This isn't currently defined in stylix, should we add it (maybe set to base00?

danth commented 1 month ago

This isn't currently defined in stylix, should we add it

Yes, this looks like something which we should define.

trueNAHO commented 1 month ago

it looks like the zathura people didn't intend this to change the default behaviour, so the commit that removes highlight-transparency also adds 0.5 transparency to all the relevant defaults. Should I still add an option in stylix even if zathura's change was more of a refactor than a change in behaviour?

Upon reconsideration, Stylix should not enforce unnecessary options:

Instead, Stylix should aim to declare only necessary options. For example, https://github.com/danth/stylix/pull/174 removed unnecessary Stylix declarations.

-- https://github.com/danth/stylix/pull/388#issuecomment-2126947630

People expecting zathura's non-transparent default behaviour would suffer undesired Stylix behaviour, as it tries to mimic zathura's deprecated behaviour by default. Maybe Stylix should not explicitly support zathura transparency, and users should explicitly declare it in their configuration, if desired?

jghauser commented 1 month ago

People expecting zathura's non-transparent default behaviour would suffer undesired Stylix behaviour, as it tries to mimic zathura's deprecated behaviour by default. Maybe Stylix should not explicitly support zathura transparency, and users should explicitly declare it in their configuration, if desired?

I'm not sure this is what the situation is. I think zathura's default behaviour (both before and after the deprecation of highlight-transparency) is to have transparency. It's just that before this was (by default) set in highlight-transparency and now (still by default) in the alpha channel. A user without custom config would not have noticed the change.

I think that it would make sense to change stylix so that the behaviour is the same as before the zathura change. I do not think that having transparency disabled is something that anyone wants (as evidenced by the fact that it's zathura's default behaviour) -- it makes text unreadable and is frankly quite annoying. Of course, there is something to be said for stylix to allow users to configure the level of transparency, but I'm not sure that is a necessary option. So the end decision is with you guys of course :).

donovanglover commented 1 month ago

Tested and works great with vimtex :+1:

Should be fine to merge this now since 24.05 is stable and comes with the new zathura

jghauser commented 1 month ago

This last push fixes the scope issue + wrong transparency level. I'm unsure whether you wanted to make transparency configurable or not in the end @danth @trueNAHO. If so, let me know and I can add that. Otherwise this is done I think.

wvffle commented 4 weeks ago

The test here seems to have timed out, is that the only blocker?

danth commented 4 weeks ago

Yeah, everything else is fine. I'll restart the test.

Related- #403