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

zathura: add transparency to highlight colors #394

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

jghauser
Copy link
Contributor

@jghauser jghauser commented May 26, 2024

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

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested this yet, but I have two suggestions:

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

  2. Zathura recently-ish deprecated their highlight-transparency option, which by default was set to something like 0.3

    -- zathura: missing transparency #392 (comment)

    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.

@jghauser
Copy link
Contributor Author

jghauser commented May 26, 2024

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
Copy link
Owner

danth commented May 26, 2024

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

Yes, this looks like something which we should define.

@trueNAHO
Copy link
Collaborator

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, #174 removed unnecessary Stylix declarations.

-- #388 (comment)

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
Copy link
Contributor Author

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
Copy link
Contributor

Tested and works great with vimtex 👍

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

@jghauser jghauser force-pushed the fix_zathura_highlighting branch from fd59863 to 9d00661 Compare June 2, 2024 08:47
@jghauser
Copy link
Contributor Author

jghauser commented Jun 2, 2024

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.

Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, making it configurable is not necessary for now since it sounds like this is just replicating the previous default value.

@danth danth requested a review from trueNAHO June 2, 2024 12:53
@danth danth enabled auto-merge (squash) June 2, 2024 12:53
@wvffle
Copy link

wvffle commented Jun 3, 2024

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

@danth
Copy link
Owner

danth commented Jun 4, 2024

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

Related- #403

@danth danth dismissed trueNAHO’s stale review June 4, 2024 15:24

All actions have been addressed.

@danth danth merged commit 85a0a92 into danth:master Jun 4, 2024
9 checks passed
@jghauser jghauser deleted the fix_zathura_highlighting branch June 4, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zathura: missing transparency
5 participants