Enhance configuration of pager colors #5524
Conversation
|
Thanks for improving this. |
|
I somehow missed this earlier, thanks for working on this. If order is important, the entries should be sorted alphabetically so that keeping the order in the future is as simple as inserting new entries in the right location, or for those of us using saner editors, as simple as selecting the lines in question and running I'm not clear on why the fallbacks array is needed, however. Do you mind clarifying? Thanks! |
|
I think the fallbacks are needed to make sure it looks right with themes that haven't specified those colors. |
|
On the fallbacks array: This is so that the secondary/selected pager specs can default to the normal pager specs (i.e. you don't /need/ to specify the secondary/selected configs if you don't want to). Without this fallbacks table the pager color scheme would require secondary/selected configured, and thus anyone with a previous color scheme, or creating a new color scheme would require to edit their configuration in order for it not to look ugly. To put it another way: without this fallback table there would be a regression in pager color schemes and a regression in the amount of specs you need to configure in order to have a decent looking color scheme. |
|
The semantics of the order are from left to right when writing an entry in the pager; that is, you draw the background, the prefix, the completion and finally the description. This was the ordering previous to my PR so maintaining it here seems fair (and possibly a follow up PR to change if important). The ordering that’s important is that groups of pager categories are kept contiguous to each other (and thus inherently cannot be alphabetic). See the code that calculates an offset to see why this is necessary (uses this trick reduces code size/complexity so I’m in favor of it) |
|
Thanks for doing this, this looks awesome! I didn't know array designators Please add to I think the fallback array has some wrong values, I'll comment on the commit. |
src/highlight.cpp
Outdated
| [highlight_spec_pager_secondary_prefix] = highlight_spec_pager_prefix, | ||
| [highlight_spec_pager_secondary_completion] = highlight_spec_pager_completion, | ||
| [highlight_spec_pager_secondary_description] = highlight_spec_pager_description, | ||
| [highlight_spec_pager_selected_background] = highlight_spec_pager_background, |
There was a problem hiding this comment.
These highlight_spec_pager_selected_* are fallbacks for the selected row, I think these should fall back to highlight_spec_selection otherwise the selected row will not be visible.
There was a problem hiding this comment.
In order to avoid regressing old color schemes I think we should make highlight_spec_pager_selected_background default to highlight_spec_search_match and the prefix/completion/description default to the unselected pager defaults (with these defaults anybody- i.e. everybody- who doesn't specify these variables at first will get the same behavior as before; fish_color_search_match controls the bg color of a selected row and everything else is configured using the pager_{prefix,completion,description} variables). What do you think?
|
Also there's something funny going on with the parens in the description:
|
Oops yeah, looks like I messed up the colors on those guys. Will fix to use the same colors/logic as before |
|
Nice. This stuff is all ancient C-ported code, we should have a real struct for highlight_specs. |
|
Maybe that'll be my next project ¯_(ツ)_/¯ |
I was hacking on this part of the codebase and found this comment mentioning to keep two things in sync, and felt like we could do better.
Originally I sought out to configure the foreground color of the selected text in the pager. After reading a thread on a github issue I was inpired to do more: now you can conifgure any part of the pager when selected, and when a row is secondary. More specifically this commit adds the ability to specify a pager row's: - Prefix - Completion text - Description - Background when said row is selected or secondary.
|
@ridiculousfish, are you happy with the changes? |
|
Merged as 50448e4, thanks!! |
This PR contains two commits, each of them attempts to have their own nice commit messages, but I'll summarize here too:
Fixes issue #2442. Inspiration was taken from Chris Mansley (unsure why I can't tag him right now), so thanks man!
Example configuration can be seen below:
Should I update some docs? Where if so?