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

Warn that text_fg_override_threshold can cause confusing issues with block characters #6767

Closed
joveian opened this issue Oct 29, 2023 · 3 comments

Comments

@joveian
Copy link
Contributor

joveian commented Oct 29, 2023

Is your feature request related to a problem? Please describe.
I noticed that pastel uses unicode block characters (particularly U+2580 ▀ Upper Half Block) to display a checkerboard pattern around the displayed color (which works even with text_fg_override_threshold set to 40) but then also for the displayed color with foreground and background set to the same color (which ends up with white or black lines in the color patch). I have a patch that I'll submit to pastel unless you want to make an exception to this override for non-text characters.

Describe the solution you'd like
I love the text_fg_override_threshold feature since lots of programs print unreadable combinations, although I worry that if this kind of graphics issue is common it might cause confusing bug reports here and elsewhere (it took me a bit to figure out the issue). The best option I can think of is adding something like "WARNING: if you use this option make sure to try turning it off if you notice ANY kind of graphics issues." to the end of the comment below the option (and maybe something in the FAQ as well).

Describe alternatives you've considered
Possibly it might make sense to not override for non-text characters, although I'm not sure how easy that would be to determine and I suspect that sometimes it would be better to make the non-text characters more visible as currently happens.

Additional context
While I didn't try different themes, I found text_fg_override_threshold set to 35 was slightly less than easily readable with my color settings (testing with ansi --color-table) while 40 was good. I suspect 40 is high enough that it is likely to work well in terms of readability with most if not all themes and might be worth mentioning as a possible starting point.

@kovidgoyal
Copy link
Owner

Not worth the effort to implement per character overrides not to mention
that there is no way to know if a particular character should be
overridden always or not. Besides, nowadays there is the kitty graphics
protocol, unicode blocks are so last century :)

Feel free to send a PR with an appropriate warning added to the text of
the option in options/definition.py

@joveian
Copy link
Contributor Author

joveian commented Oct 30, 2023

I was also thinking that it might make sense to have a few keymaps for things that might help with figuring out various issues, or maybe one key that switches through a few different settings after a short delay. Currently this seems only possible if multiple settings files are used, in which case keeping the settings you don't want to change in sync between multiple files is more of a challenge (as is updating them as features are added and changed).

I suggest three new mappable actions:
set name value
toggle_setting name separator value1 separator value2
unset name

The first could be used to map changing any setting (except those that don't work with config file reloads) while the second would set to value1 unless the current setting is equal to value1 in which case value2 would be used (where value1 and value2 are multiple arguments if needed, I guess they should all need to be the current setting for value2 to be used). unset would set name to the default value.

To make longer settings changes possible maybe combine could allow newline as a separator and then end on an empty line so that you could do something like:
map key combine
set text_fg_override_theshold 0
set disable_ligatures always
set font_features none
unset modify_font
text_composition_strategy 1.0 0
sleep 1
load_config_file

I guess action_alias could be used to make everything fit on one line but it seems easier to be able to put everything in one place.

I don't know python but might be able to learn enough to make a patch.

Another idea I had along those lines is config file variants, where load_config_file could be given a --variant argument and allow settings to be given multiple times for different variants (but for each setting the no variant specified version is used unless the current variant has a different value for that setting). So you could do something like:
text_fg_override_threshold 40
A, B: text_fg_override_threshold 0
and then load_config_file --variant A (or B) would use 0 and no variant specified or any other variant specified would use 40. There could potentially be a set of default variants defined with settings less likely to cause trouble that could similarly be cycled through with combine and sleep in case of various graphics or font issues (to get a starting point for what could be going wrong). However, this would have a larger long term effect on the config file syntax while set, toggle_setting, and unset are similar to existing commands (and multi-line combine would have a smaller long term impact).

@kovidgoyal
Copy link
Owner

You have include, use it in your config files to establish common
settings. Your variants can then each be a different conf file that
includes the common settings and overrides whatever it wants after the
include.

And yes, you can make a PR to allow load_config_file to accept an
--override option just as kitty accepts an --override option. Not
something I am particularly interested in, but I will accept a PR for
it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants