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

RFC: ui: refactor style handling #1154

Merged
merged 1 commit into from
Mar 27, 2024
Merged

RFC: ui: refactor style handling #1154

merged 1 commit into from
Mar 27, 2024

Conversation

rnpnr
Copy link
Collaborator

@rnpnr rnpnr commented Nov 10, 2023

Hi everyone, this is a pretty major change to the way styles are handled inside vis. I would really appreciate testing to make sure this doesn't cause major breakage with existing themes. Most likely this will cause settings that were silently ignored before to start working.

Also I would appreciate some tests with the default style. I tested it in the Linux virtual console and it seems to work great there. I have not tested with a transparent terminal emulator so I would appreciate if someone could let me know if it respects that or not.

Commit message follows:

The old style handling had a lot edge cases where one of the colours or the attribute wouldn't get applied correctly. This commit adds a new style_set() method to the Ui which should be called instead of manually touching a cell's style. This also means that the Cell struct can be made opaque since all the handling is now done inside the ui-terminal files.

With this it is now viable to combine the light and dark 16 colour themes into a single base-16 theme. This theme works very well with the Linux virtual console and will now be the default theme regardless of if the terminal supports 256 colours or not. This should address the common complaints about vis not respecting the users default terminal colours.

fixes #1151: Theming is sometimes partially applied or ignored
see #1103: terminal no longer has transparency/opacity
see #1040: Transparent background and setting options by default

@lobre
Copy link

lobre commented Nov 13, 2023

Nice work! Just tested it rapidly and it seems way better than before!

I would like to try creating a full 16-color simple theme with your branch, to see if I manage to have something meaningful. It could help identify problems here.

For now, the default theme in my terminal is not that great, especially the contrast in selections. If I manage to have something simple but efficient, maybe I could then submit it as default to have something great out of the box, that would generally work great in major terminal emulators (with default palettes). Let's see.

@lobre
Copy link

lobre commented Nov 13, 2023

Does it make sense to call it a 16-base while it only supports 8 colors? Isn't it the opportunity to bring the missing 8 bright variants?

@mcepl
Copy link
Contributor

mcepl commented Nov 13, 2023

It didn’t apply completely cleanly. After some work with merge tools I got
0001-ui-refactor-style-handling.patch

@rnpnr
Copy link
Collaborator Author

rnpnr commented Nov 13, 2023

Does it make sense to call it a 16-base while it only supports 8 colors? Isn't it the opportunity to bring the missing 8 bright variants?

This is kind of my bad. In the linux virtual console the bold attribute is applied by adding 8 to the underlying colour giving the bright variant. So from that perspective it is a 16 colour theme.

I'm not particularly tied to what is here. If you come with something better I would be fine with making that the default instead. I would prefer if it keeps working with the virtual console though.

It didn’t apply completely cleanly.

That is to be expected. This applies on top of current master not on top of the updated lexers.

@mcepl
Copy link
Contributor

mcepl commented Nov 14, 2023 via email

@rnpnr
Copy link
Collaborator Author

rnpnr commented Nov 14, 2023

Hmm, with my current setup and default theme I get dark yellow cursor on the light yellow background, so the cursor is more or less invisible.

Which theme is that? Some settings related to the cursor and selection used to be silently ignored so it could be an issue with the theme.

@mcepl
Copy link
Contributor

mcepl commented Nov 14, 2023

Hmm, with my current setup and default theme I get dark yellow cursor on the light yellow background, so the cursor is more or less invisible.

Which theme is that? Some settings related to the cursor and selection used to be silently ignored so it could be an issue with the theme.

Light theme in my foot terminal and no theme in ~/.config/vis/visrc.lua (which I guess means default.lua theme, right?).

@rnpnr
Copy link
Collaborator Author

rnpnr commented Nov 14, 2023

Light theme in my foot terminal and no theme in ~/.config/vis/visrc.lua (which I guess means default.lua theme, right?).

It looks like foot has a handful of different light themes, some with poor contrast between regular and bright. I'm not sure we can get it right for all possible combinations but I do think the theme can be modified. Can you send a screenshot of your terminal without vis loaded (but with an active cursor) and with vis loaded? Maybe I can at least infer what should be changed.

@mcepl
Copy link
Contributor

mcepl commented Nov 15, 2023

It looks like foot has a handful of different light themes, some with poor contrast between regular and bright. I'm not sure we can get it right for all possible combinations but I do think the theme can be modified. Can you send a screenshot of your terminal without vis loaded (but with an active cursor) and with vis loaded? Maybe I can at least infer what should be changed.

Taking this back, I had some really weird (most likely my own work) theme, which is most likely buggy. Unfortunately, I cannot find anything I would like (I am a friend of Plan9/ACME-like very plain styles), but it is not fair to blame vis for it.

@mcepl
Copy link
Contributor

mcepl commented Nov 23, 2023

Light theme in my foot terminal and no theme in ~/.config/vis/visrc.lua (which I guess means default.lua theme, right?).

It looks like foot has a handful of different light themes, some with poor contrast between regular and bright. I'm not sure we can get it right for all possible combinations but I do think the theme can be modified. Can you send a screenshot of your terminal without vis loaded (but with an active cursor) and with vis loaded? Maybe I can at least infer what should be changed.

screenshot-2023-11-23_23-11-1700779388

(left is vis with no theme set, right is just plain foot with bash running; both with acme foot style; cursor in vis is actually on line with swayidle, the letter s is slightly darker)

Actually, when using vis-minimal-theme/minimal-clear, it is much better:

screenshot-2023-11-23_23-11-1700779891

I am not sure why the default theme fails so miserably, but at least I can work now.

@mcepl
Copy link
Contributor

mcepl commented Dec 2, 2023

To conclude my rambling:

Tested-by: Matěj Cepl mcepl@cepl.eu

@rnpnr
Copy link
Collaborator Author

rnpnr commented Dec 2, 2023

To conclude my rambling:

Tested-by: Matěj Cepl mcepl@cepl.eu

Thanks, you have a valid point though. I don't want to make the default theme worse. I don't want to do what minimal-clear does though because the point is to only use the terminals colors.

Note for others looking at that theme: local clear = '#00000000' is invalid from vis' perspective and is treated as local clear = ''. Currently the parsing code only accounts for exactly 6 hex digits.

@sansfontieres
Copy link
Contributor

sansfontieres commented Dec 8, 2023

vis.lexers.STYLE_CURSOR_LINE seems to steal the colours of the current line (but not the font style)
image
image
(see the "-v" on the last picture)

@rnpnr rnpnr mentioned this pull request Dec 29, 2023
8 tasks
@rnpnr
Copy link
Collaborator Author

rnpnr commented Dec 29, 2023

One other issue I noticed, also related to dimming, is that when you have the cursor somewhere within a dimmed section the line to the right of the cursor gets reset to the normal attribute. See attached:
image

@mcepl
Copy link
Contributor

mcepl commented Mar 10, 2024

Couldn't we just merge this and #1133? I was running this for the last couple of weeks, and it seems to be working. I would hope that with merging these, we could get more testing and bigger chance to fix any remaining issues.

@rnpnr
Copy link
Collaborator Author

rnpnr commented Mar 10, 2024

I would be ok with that. I've been using both of them for as long has they have been open and while I know of a couple of bugs in both of them they don't get in the way of editing in any way. I've been too busy with other things to track down them down. Most of them (like the screenshots above) are probably quite trivial.

I'll hold off for a couple days for any other opinions but I think its time.

The old style handling had a lot edge cases where one of the
colours or the attribute wouldn't get applied correctly. This
commit adds a new style_set() method to the Ui which should be
called instead of manually touching a cell's style. This also
means that the Cell struct can be made opaque since all the
handling is now done inside the ui-terminal files.

With this it is now viable to combine the light and dark 16 colour
themes into a single base-16 theme. This theme works very well
with the Linux virtual console and will now be the default theme
regardless of if the terminal supports 256 colours or not.  This
should address the common complaints about vis not respecting the
users default terminal colours.

fixes martanne#1151: Theming is sometimes partially applied or ignored
see martanne#1103: terminal no longer has transparency/opacity
see martanne#1040: Transparent background and setting options by default
@mcepl
Copy link
Contributor

mcepl commented Mar 26, 2024

How long is “a couple of days” in your part of the world?

@rnpnr rnpnr merged commit 95bf9f5 into martanne:master Mar 27, 2024
24 checks passed
@rnpnr rnpnr deleted the selstyle branch March 27, 2024 12:07
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.

Theming is sometimes partially applied or ignored
4 participants