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 default-theme to correctly highlight focused attachments #1687

Merged
merged 2 commits into from
Dec 25, 2024

Conversation

paumr
Copy link
Contributor

@paumr paumr commented Dec 22, 2024

e90da9b broke the highlighting of focused_attachments.

This PR resets the faulty value in question to its prior color.

@lucc
Copy link
Collaborator

lucc commented Dec 22, 2024

In what sense is it broken? The commit you mention is from #1640, @meeuw what was the motivation for the change from green to gray at this point specifically?

I checked on xterm's default theme and it might be a little hard to read there:
2024-12-22-224526_243x42_scrot

@paumr can you please also check your change with the extra/theme_test.py script, preferably on different terminals (light and dark).

@lucc lucc added the theming label Dec 22, 2024
@paumr
Copy link
Contributor Author

paumr commented Dec 23, 2024

@lucc thanks for looking into this. Your terminal probably only supports 16 colors, in which case everything works as expected.

As you can see here: https://alot.readthedocs.io/en/latest/configuration/theming.html#colour-attributes
The configuration format is:

name = mono fg, mono bg, 16c fg, 16c bg, 256 fg, 256 bg

means #1640 kept the 16c highlighted-background at light green, but set the 256c background to "dark gray" (the same color as the non-highlighted setting). I assume this was a copy and paste error.

If you want to reproduce this issue open alot in a 256color terminal. Then you won't see any attachment highlighting.

@lucc
Copy link
Collaborator

lucc commented Dec 23, 2024

Ok now I understand what you say about #1640 braking the theme (having a typo) at this point.

My point about it being hard to read after your proposed change is the contrast between the fg and bg of the selected attachement:

2024-12-23-175001_248x40_scrot

I consider that light gray to be hard to read on the light green.

Do you mind changing the fg or bg a bit so that it is easier to read the text?

@lucc
Copy link
Collaborator

lucc commented Dec 23, 2024

PS for others to understand the bug in the current master: the current theme does not distinguish between selected and not selected attachement:

2024-12-23-180003_602x18_scrot
2024-12-23-180026_469x26_scrot

That is bad UI.

@paumr
Copy link
Contributor Author

paumr commented Dec 23, 2024

Ok, as requested I changed the fg color to dark grey to increase the contrast a bit.
I hope this change doesn't break too many themes/styles.

@meeuw
Copy link
Contributor

meeuw commented Dec 23, 2024

I don't remember changing green to gray, my main rationale was to change the colors to make them both usable for black on white as well as white on black terminals.

I actually think it's a typo (from me)

@lucc
Copy link
Collaborator

lucc commented Dec 24, 2024

@meeuw can I ask you to review this? Otherwise I would merge it.

Copy link
Contributor

@meeuw meeuw left a comment

Choose a reason for hiding this comment

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

Sure! I did some quick testing and it's readable on both black-on-white as white-on-black terminals.

Thanks for fixing!

@lucc lucc merged commit 16b50e5 into pazz:master Dec 25, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants