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

fix(highlights): avoid NONE backgrounds for custom groups #50

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

ramojus
Copy link
Owner

@ramojus ramojus commented Jul 31, 2024

My solution for #47, alternative to #48
I think this way is more future proof, when new plugins use custom groups, we won't have to worry about this issue anymore. Also nicer that the fix is just in one module.

Fixes #47

@antoineco
Copy link
Contributor

This does look more future proof indeed 👍
I just have one comment: defaulting to bg in cases where plugins were designed to use bg3 may lead to less esthetically pleasing visuals.

@ramojus
Copy link
Owner Author

ramojus commented Jul 31, 2024

These custom groups replicate shade of e.g. bg3 on top of bg (which will no longer be NONE), so I think it should be fine aesthetically, or am I missing something?
One thing though, I should remove this change for FileTreeWinSeparator group, because it's better to leave it NONE

@antoineco
Copy link
Contributor

antoineco commented Jul 31, 2024

am I missing something?

Those buttons are now barely visible:

Screenshot 2024-07-31 at 10 25 48

Compare with #48

Screenshot 2024-07-30 at 14 20 15

@ramojus
Copy link
Owner Author

ramojus commented Jul 31, 2024

Problem is, we lose control of the bg when it's transparent, so this will depend a lot on the strength of transparency and how light the wallpaper is.
I don't think it makes much sense to apply the shade of bg4 on top of bg3 (FloatNormal), when FloatNormal is not visible. I mean, Normal is also not visible, but it just seems more reasonable to default to it, when no bg is visible. This is also a limitation of my solution, but I can't know from where MenuButtonSelected will be used (might not be used from floating window)

For people who use transparency with light wallpapers I would just suggest lightening the background color, or just the shades, which is/will be possible on v1.

@antoineco
Copy link
Contributor

antoineco commented Jul 31, 2024

Nothing is preventing from having a default in custom_groups.lua and keeping the plugins nicely optimized though.

Even if it's not possible to control what the background will look like, it should be safe to assume that a majority of people use a high opacity (92% in my screenshot), because whoever has ever tried lowering the opacity further knows how unreadable the text becomes on mixed backgrounds. Also, I am using one of macOS' default wallpapers, which is fairly bright already.
That means, even with transparency enabled, the background is likely to remain fairly close to the non-transparent bg.

@ramojus
Copy link
Owner Author

ramojus commented Jul 31, 2024

I see, so it's probably safe to assume that when transparent background is enabled, the background will be brighter (because most wallpapers are brighter than the background of Normal), but not by much, because lot's of transparency doesn't make much sense. I think with this assumption we can work something out.
Why not lighten all of the shades by default when transparent background is enabled? I've actually had issues with this, e.g. with transparent background when I hover over an identifier and I don't really see it get highlighted, because the background is too bright.
But we should probably make a separate issue/PR for this, if you want to you're welcome to do so, I don't really have much time these days.

As to modification of specific plugins to avoid this problem, I still think my earlier statement still stands:

I don't think it makes much sense to apply the shade of bg4 on top of bg3 (FloatNormal), when FloatNormal is not visible. I mean, Normal is also not visible, but it just seems more reasonable to default to it, when no bg is visible.

@antoineco
Copy link
Contributor

antoineco commented Jul 31, 2024

I'm not in a good position to provide meaningful guidance here since I have rarely used transparent terminals, especially not for any serious work. You are the best person to decide what amount of engineering makes sense based on how many UI-oriented plugin customizations may be added in the future.

I just wanted to share some observations about what can be expected about the background color in general, and the fact that making UI elements too dark in that mode feels like a degraded experience (to me at least).

@dartey25
Copy link

@antoineco @ramojus Thank you guys for the fixes!

@ramojus ramojus merged commit 7bb96f8 into main Jul 31, 2024
1 check passed
@ramojus
Copy link
Owner Author

ramojus commented Jul 31, 2024

@antoineco thanks for the observations!
I'll probably add this functionality at some point: lighten all of the shades by default when transparent background is enabled.

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.

Transparent background not working
3 participants