Fixing CSS for Updater VM name Labels

[alimirjamali]

Setting the default VM Name Label colors for custom labels

Reference to issue: Fixes QubesOS/qubes-issues#9236

[marmarta]

I don't like enforcing black/white text for all labels in all widgets - I'm afraid this will cause problems with users' custom themes or some other things we have not foreseen (as in, this looks like it works right now, but I'm somewhat worried about weird interactions in the future...)

In the color definitions (qubes-colors-light/dark files) we have a text-color color defined that is the color you need (black for light themes, dark for light themes) - I think it would be a bit safer to add a 'qube-box-all-colors' or something css class to the qube-widgets-base file, set the label color there, and then add this class to qube labels in updaters:

in qui/updater/utils.py:

def label_color_theme(color: str) -> str:
    widget = Gtk.Label()
    widget.get_style_context().add_class(f'qube-box-{color}')
    widget.get_style_context().add_class('qube-box-all-colors')  # add this
    gtk_color = widget.get_style_context().get_color(Gtk.StateFlags.NORMAL)
    color_rgb = ast.literal_eval(gtk_color.to_string()[3:])
    color_hex = '#{:02x}{:02x}{:02x}'.format(*color_rgb)
    return color_hex

[alimirjamali]

In the color definitions (qubes-colors-light/dark files) we have a text-color color defined that is the color you need (black for light themes, dark for light themes) - I think it would be a bit safer to add a 'qube-box-all-colors' or something css class to the qube-widgets-base file, set the label color there, and then add this class to qube labels in updaters:

That is an excellent suggestion. I will try this in next few days and see if nothing breaks.

[alimirjamali]

widget.get_style_context().add_class('qube-box-all-colors') # add this

I just realized that this is effectively overwriting the previous 'qube-box-{color} CSS class. It does not properly cascade. All labels will be in qube-box-all-colors this way. It would be possible to do something like:

def label_color_theme(color: str) -> str:
    widget = Gtk.Label()
    if color in ['red', 'orange', 'yellow', 'green', 'gray', 'blue', 'purple', 'black']:
        widget.get_style_context().add_class(f'qube-box-{color}')
    else:
        widget.get_style_context().add_class('qube-box-all-colors')  # add this
    gtk_color = widget.get_style_context().get_color(Gtk.StateFlags.NORMAL)
    color_rgb = ast.literal_eval(gtk_color.to_string()[3:])
    color_hex = '#{:02x}{:02x}{:02x}'.format(*color_rgb)
    return color_hex

We do not have the color label index here. But we could do something like:

    if qubesadmin.Qubes().get_label(color).index <= qubes.config.max_default_label:
        widget.get_style_context().add_class(f'qube-box-{color}')
    else:
        widget.get_style_context().add_class('qube-box-all-colors')  # add this        

[marmarta]

The latter sounds like a good idea, I like it. And good catch on the cascading... ugh, css is the worst sometimes :D

[alimirjamali]

The latter sounds like a good idea, I like it.

OK. I made the final changes and tested them. I used qube-box-custom-label class for the custom labels as it is a little bit more definitive than qube-box-all-colors

Hope this is good.

[marmarta]

PipelineRetryFailed

[marmarta]

looks great! The tests failed due to some timeouts on server side, I bumped them to run again

[alimirjamali]

looks great! The tests failed due to some timeouts on server side, I bumped them to run again

I am still not very proficient with builder2 and OpenQA testing workflow. Hopefully better in future.

[marmarta]

Oh, this was not your fault at all - sometimes the qa server has a bad day, and clearly, it was one of those.

[marmarta]

Ok, I see a lot of errors due to pylint-dev/astroid#2190 (so nice to be caught in upstream issues :D) ; I'm going to restart and pray, but other than that, I see that you are importing qubes, which is qubes-core-admin package, and not qubes-core-admin-client, which should be the one used to talk with outside world.

@marmarek, this is a question for you - here what is used is the max_default_label value (currently hardcoded as 8 in core-admin); is it ok to import qubes for it?

[marmarek]

@marmarek, this is a question for you - here what is used is the max_default_label value (currently hardcoded as 8 in core-admin); is it ok to import qubes for it?

Definitely not, qubes module is not guaranteed to be available where this tool is running (especially, it isn't in sys-gui etc). And also, where "default" labels end is rather implementation detail, not part of any kind of API, it's allowed to change etc. The property you want to test here is really "does the label has corresponding css class defined in this package", so... maybe test for this property? Like, check if it's one of those for which you define CSS class?

[alimirjamali]

The property you want to test here is really "does the label has corresponding css class defined in this package",

@marmarta
Maybe it is better to avoid over complicating it. Defining a constant such as DEFINED_LABEL_CSS = 8 should do it for the time? In utils.py?

[marmarta]

Or list them manually, with a comment in CSS and code saying "if you are adding new colors, you need to add it here and here and here"? It's ugly, but in practice what we want is not "is the label defined", but "is the relevant css defined in this package"...

[alimirjamali]

Ok. Lets do something like if f'qube-box-{color}' is in qubes-color-dark.css & qubes-color-light.css & qubes-widgets-base.css, then widget.get_style_context().add_class(f'qube-box-{color}') else widget.get_style_context().add_class('qube-box-custom-label')

Is this good?

[marmarta]

Ok. Lets do something like if f'qube-box-{color}' is in qubes-color-dark.css & qubes-color-light.css & qubes-widgets-base.css, then widget.get_style_context().add_class(f'qube-box-{color}') else widget.get_style_context().add_class('qube-box-custom-label')

Is this good?

I think so? I'm mildly worried that it might be sorta slow in the long run if there is a lot of labels to label? Thus the idea of just manually listing them. But this is a bit more elegant, true.

[alimirjamali]

I think so? I'm mildly worried that it might be sorta slow in the long run if there is a lot of labels to label? Thus the idea of just manually listing them. But this is a bit more elegant, true.

You are right. Not only it makes it slow. But also adds complexity. I still believe on adding DEFINED_LABEL_CSS = 8 constant and using it instead of qubes.config.max_default_label in the 'if' condition. It is the best compromise in my opinion.

It will be only necessary to remember to bump this number if CSS for any new standard label is added in the feature.

[marmarta]

I'm ok with that

[marmarta]

I like this, just one issue - you seem to have accidentally pushed together with this the font change, which is problematic due to "this font is not installed, oops" problem... I'm good with either changing that commit to use font-family: monospace or dropping it from here.

[marmarek]

I'd rather prefer to rely on what CSS classes you have defined, not label indexes (and hardcoding it's 8 of them). As I said before, there is no guarantee those won't change.

[alimirjamali]

I'd rather prefer to rely on what CSS classes you have defined, not label indexes (and hardcoding it's 8 of them). As I said before, there is no guarantee those won't change.

OK. I will work on it.

I like this, just one issue - you seem to have accidentally pushed together with this the font change, which is problematic due to "this font is not installed, oops" problem... I'm good with either changing that commit to use font-family: monospace or dropping it from here.

Exactly. I was actually working on it. It is related to another issue which we discussed before. I have to git reset --hard and work from there.

[alimirjamali]

I think so? I'm mildly worried that it might be sorta slow in the long run if there is a lot of labels to label?

OK. I have came up with this so far. It loads the CSS data from file only once. It would be very nice if can somehow obtain Gtk.CssProvider data which was loaded via qubes_config.widgets.gtk_utils.load_theme function directly from memory without modifying gtk_utils library. I am searching for possible workarounds.

try:                                                                                                                                
    provider = Gtk.CssProvider()                                                                                                    
    provider.load_from_path(path.dirname(path.realpath(__file__))+                                                                  
        '/../styles/qubes-widgets-base.css')                                                                                        
    CSS_THEME = provider.to_string()                                                                                                
except:                                                                                                                             
    CSS_THEME = ''                                                                                                                  
                                                                                                                                    
def label_color_theme(color: str) -> str:                                                                                           
    widget = Gtk.Label()                                                                                                            
    if f'.qube-box-{color}' in CSS_THEME:                                                                                           
        widget.get_style_context().add_class(f'qube-box-{color}')                                                                   
    else:                                                                                                                           
        widget.get_style_context().add_class('qube-box-custom-label')                                                               
    gtk_color = widget.get_style_context().get_color(Gtk.StateFlags.NORMAL)                                                         
    color_rgb = ast.literal_eval(gtk_color.to_string()[3:])                                                                         
    color_hex = '#{:02x}{:02x}{:02x}'.format(*color_rgb)                                                                            
    return color_hex

[marmarta]

I can't figure out a way to get the CSS provider from a widget, yeah. If this works fast enough, it's an option that we can use here.

[alimirjamali]

I have been testing the above fix for almost a week. I believe it to be a proper fix. It uses the already in-memory Css data without reading the CSS file twice.

[alimirjamali]

Regarding Pylink warning on EffectiveCssProvider Global mutable variable within the module, I had added underscores after and before the variable. I can not use a class variable here.

[marmarta]

Suggested change

	Return Value is the Effective Css Provider which could be discarded
	Returns used CSS provider; it can be safely discarded if unused.

[alimirjamali]

Applied the suggestion

[alimirjamali]

I am reading the Pylink errors. Most of it is because of the existing code. Here is what is related to this commit:

https://gitlab.com/QubesOS/qubes-desktop-linux-manager/-/jobs/7257225118#L128

p.s. I can work on the rest if this is a priority

[marmarek]

Most of those are because pylint is confusing GTK3 and GTK4... There is a fix pending (on pylint side) but it wasn't merged yet.

[alimirjamali]

There is a minor bug in the logic. If user creates labels such as yellowstone, greenland, bluefish, blackpanter yell, blu, re or similar, the condition assumes the the CSS class exists. I did not touch the existing commit as I did not want to create confusions. If the current one is OK, I can use regex or something similar in another commit.

[marmarta]

I think this is edgeness of an edge case that can be safely ignored for this PR.

[alimirjamali]

Most of those are because pylint is confusing GTK3 and GTK4... There is a fix pending (on pylint side) but it wasn't merged yet.

Is there anything else that I should do for this patch @marmarek ? It is possible to properly encapsulate the Css Provider within a class inside qui/upater/utils.py to avoid the global variable. But I think we already have a lot of other pylint errors which makes the work a little bit confusing.

[marmarek]

PipelineRetry

[marmarek]

I'm okay with leaving it a global variable, but pylint needs to not complain. That's one # pylint: disable=global-statement and removing the other one (you don't need global if you don't assign to it). And change checking for None too.

[alimirjamali]

I'm okay with leaving it a global variable, but pylint needs to not complain. That's one # pylint: disable=global-statement and removing the other one (you don't need global if you don't assign to it). And change checking for None too.

Done. I made the pylint fixes in a new commit to make it easier for review.

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.54%. Comparing base (8d8839c) to head (d283842).
Report is 8 commits behind head on main.

Files	Patch %	Lines
qui/updater/utils.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #197   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files          57       57           
  Lines       10816    10833   +17     
=======================================
+ Hits        10118    10134   +16     
- Misses        698      699    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[marmarta]

looks good to me :)