Xrandr applet: fix popup menu

[raveit65]

before

after

[monsta]

Hmm, the issue isn't reproducible in 1.16 and 1.18 (both tested with GTK+ 3.22).

[raveit65]

You don't need it to cherry-pick to 1.18/16 if the ugly colors in menus are fine.

[monsta]

I thought the problem was the misplaced background? Reverting https://github.com/mate-desktop/mate-settings-daemon/commit/2f1b581c28d96eec24a97e8a35f3b26024c554a2 places it under text, as before.

[raveit65]

Problem was

1. unreadable text with using dark hard coded color on dark bg.
2. a useless non working bg

Well, maybe i did something wrong with using gtk_widget_set_margin..... I will try to fix that.

But after using the PR several days i prefer to use a normal menu without ugly colors as background as design. Chances are high that i use that for fedora if ugly colors are back ;)

[raveit65]

Reverting https://github.com/mate-desktop/mate-settings-daemon/commit/2f1b581c28d96eec24a97e8a35f3b26024c554a2 gives me an unreadable title of the second monitor in menu. And borders of the rectangle are not correct. Do you like that?

[monsta]

Dammit, this sucks. Black background shouldn't be there - the color should be the same as in the main display properties UI (and also the popup on top-left). It uses light colors. Black border is invisible on black theme background too...

Ok, let me test this PR with a light theme.

[monsta]

Ok, this looks clearer. The only thing is that the monitor name label is grayed because it's an inactive menu item. Should we bring back the hack that made it show with active color?

[raveit65]

Which hack makes monitor-title show with active color ? :)

Inactive_fg_color is defined by themes. For me the monitor-title looks OK with Menta, HC or adwaita themes. Only with Traditional the monitor-title looks a bit pale. @sc0w What do you think?

Well, i can live with a bold active color.......

[monsta]

It's right there in the deleted code:

static gboolean
output_title_label_draw_cb (GtkWidget *widget, cairo_t *cr, gpointer data)
{

..........

        /* We want the label to always show up as if it were sensitive
         * ("style->fg[GTK_STATE_NORMAL]"), even though the label is insensitive
         * due to being inside an insensitive menu item.  So, here we have a
         * HACK in which we frob the label's state directly.  GtkLabel's expose
         * handler will be run after this function, so it will think that the
         * label is in GTK_STATE_NORMAL.  We reset the label's state back to
         * insensitive in output_title_label_after_expose_event_cb().
         *
         * Yay for fucking with GTK+'s internals.
         */

        gtk_widget_set_state (widget, GTK_STATE_NORMAL);

        return FALSE;
}

/* See the comment in output_title_event_box_expose_event_cb() about this funny label widget */
static gboolean
output_title_label_after_draw_cb (GtkWidget *widget, cairo_t *cr, gpointer data)
{
        g_assert (GTK_IS_LABEL (widget));
        gtk_widget_set_state (widget, GTK_STATE_INSENSITIVE);

        return FALSE;
}

        /* We want to paint a colored box as the background of the label, so we connect
         * to its expose-event signal.  See the comment in *** to see why need to connect
         * to the label both 'before' and 'after'.
         */
        g_signal_connect (label, "draw",
                          G_CALLBACK (output_title_label_draw_cb), manager);
        g_signal_connect_after (label, "draw",
                                G_CALLBACK (output_title_label_after_draw_cb) , manager);

[raveit65]

I tried hard, but applet crashes if i try to use parts of the old code. Feel free to try it out for yourself.

[monsta]

My quick test showed that the label is gray in various themes, including dark ones and high contrast ones...

[monsta]

I don't get any crashes, but the label is still gray if I bring back the two signal handlers (without hardcoded fg/bg colors of course).

[raveit65]

I think this hack is an old one and doesn't work with newer gtk+ version. Maybe it was from gtk+-2 ?

[monsta]

Good hint, I didn't think about it. Confirmed. I've removed the handlers again and instead reverted 2674157347d3950b3ed055cfecc0a3dd65bb614a locally. Only that change made the label black again, but that's bad for dark themes.

I guess this can be merged unless someone has any more ideas.

[raveit65]

I added a style class for the menuitem, i result we can set the insensitve label color in themes without matching other menus. Add this to mate-applications.css. Examples: Menta and Traditional themes:

/* xrandr applet */
.mate-panel-menu-bar menuitem.xrandr-applet:disabled > label {
    color: @menu_fg_color;
}

Submarine themes:

/* xrandr applet */
.mate-panel-menu-bar menuitem.xrandr-applet:disabled > label {
    color: @menu_fg_dark_color;
}

BlackMate, GreenLaguna:

/* xrandr applet */
.mate-panel-menu-bar menuitem.xrandr-applet:disabled > label {
    color: @menu_controls_color;
}

Please test again.

[raveit65]

Btw. i noticed that left-click menu of nm-applet have the same problem :)

[lukefromdc]

nm-applet is controlled by GNOME, not by us. I have to patch that locally to fix issues on it on my boxes

[monsta]

Good idea, but since this means setting fg color to "active" in every theme, maybe we should load our own CSS with that? Yes, this is hardcoding, but not hardcoding to some R/G/B values. It should look ok in all themes.

[monsta]

Notifying @alexarnaud just in case, as this change affects high contrast themes.

[raveit65]

Good idea, but since this means setting fg color to "active" in every theme, maybe we should load our own CSS with that?

I don't think that it is possible to match all themes with css file because we have dark, light and mixed themes like Submarine or Mate-Ambiant. Those are light themes with dark menus. And some themes like eg. BlackMate use other color names, see my examples. I saw css styling in gnome-panel for light and dark themes with loading it in a css, but this will never match mixed themes. I am sorry to say 'Life isn't a cherry bowle' and this far away from my coding skills. Feel free to add this for yourself. ... if this state isn't sufficient for you please close the PR, but i can't do more

[lukefromdc]

The issue this PR is designed to fix makes quite the mess in my theme:

The origjnal idea of using a label that matches what is shown on each monitor was a good one but the label background is so badly cut up as to be worse than useless. Without the label bg working, on a theme with a black background for the menu the text disappears entirely. HighContrastInverse is also affected though not quite as badly as the background for the menu is not entirely black there.

We really do need some kind of fix here

[lukefromdc]

Here's what the popup menu looks like in master with HighContrastInverse right not. The label BG appears partially behind the text for the first of two monitors and not at all behind the text for the second. It is quite broken.

[lukefromdc]

Reverting https://github.com/mate-desktop/mate-settings-daemon/commit/2f1b581c28d96eec24a97e8a35f3b26024c554a2 only fixes things for the FIRST monitor, as the background never gets applied to the second monitor:

[lukefromdc]

I can confirm that the latest version of this PR with the style class works, and that the style class allows changing the color of the monitor identification test. I am now trying to develop a sane fallback css to use as a default that themes can explicitly override.

[lukefromdc]

https://github.com/mate-desktop/mate-settings-daemon/pull/222/commits/992e5bcaff63191c1501d188ee8007653ecedf7f is an alternative approach that works in all themes. I tried to make it overrideable, but if I used GTK_STYLE_PROVIDER_PRIORITY_THEME it did not work at all (themes overrode it) and if I used GTK_STYLE_PROVIDER_PRIORITY_APPLICATION no theme could override it. Pretty sure GTK_STYLE_PROVIDER_PRIORITY_SETTINGS was the same as APPLICATION but nothing gave a default that themes could override.

This was the original behavior-and it matches the also non-themable rectangles on the monitors Those render yellow-green in all themes both before all of this and now on my machine, not sure how @raveit65 got the pink which is shown in the origjnal report: screenshot issue, video driver, something else? Anyway, here's what it looks like, in a composite image showing four different themes:

[lukefromdc]

I can tune this according to suggestions from others here if we want to use this approach

[lukefromdc]

I also tested trying to just set a color for the label in the CSS file, but any selector I could use broke about half the MATE themes. Thus that approach won't work: we can either leave this for themes to deal with (this PR as it stands), or set the original behavior as the non-overrideable label theme (the PR to the PR).

[raveit65]

not sure how raveit65 got the pink which is shown in the origjnal report: screenshot issue, video driver, something else?

We have always 2 bg colors here, one for the internal monitor (pink) and one for an external monitor (mint-green), only that the second color was never working in menu. Screenshot of m-d-p without PR or new branch

I think marking several monitors with different bg colors in menus too, was the idea of origin gnome developer. But i can't remember if that was ever working in MATE (gtk2) or gnome-2.0, as i don't use several monitors normal. And keep in mind that m-d-p comes from mate-control-center, looks like that the code with several bg colors is working there.

I will test addition PR.

[monsta]

For reference, here's how it looks in GTK+2 build of MATE 1.16 (system is Mint 18.1). I've bumped number of virtual monitors in my VirtualBox to three to show various colors. The screenshots are very wide, they were taken using VirtualBox's built-in screenshot feature - it captured all three screens at once.

With Mint-X theme:

With BlackMATE theme:

For some reason, label fg color is gray in GTK+2 build, though it's supposed to be black. Wonder if the active/inactive color hack ever worked with GTK+2 too... :smile:

Another thing, which is present with any GTK+, is that the border color is hardcoded to black. It doesn't look very good in a dark theme (and with completely black theme bg it won't even be visible).

[raveit65]

For some reason, label fg color is gray in GTK+2 build, though it's supposed to be black. Wonder if the active/inactive color hack ever worked with GTK+2 too... :smile:

So if the pale fg color isn't a regression, why not using this PR without last commit and dropping the color bg? :) Honestly this colored bg in menus is the ugliest design i ever saw....

[lukefromdc]

I would favor keeping the style class so theme developers have the option of removing the insensitive color from the label text.

EDIT: in my alternative approach, I actually had to set the border style in the css or no border showed up, I was attempting to copy the original approach but was unaware of multiple monitor colors as I had paid little attention to seeing it in the dialog and had never seen it work in the menu. That being so, all entries in the menu using the color of just ONE monitor is actually a source of confusion. To make the original approach work using css is probably close to impossible.

I tried to borrow code from the old code to simply remove the insensitive flag, but everything I tried other than using css either made the label selectable or left it in the insensitive color. No internal css file can be used, as the correct color selector varies too much among the MATE themes. Thus we are left with the approach of the insensitive color (will usually be visible) as the default and the ability of themes to change it if this PR (the main one) is merged as it stands

[lukefromdc]

The only way I can see to get the colors for the different monitors into the menu by by a CSS file would be some way of getting the output from mate_rr_labeler_get_rgba_for_output into the css-but we only have data (hardcoded strings), a file (one or more prewritten css files), or a resource (compressed and compiled files that contained hardcoded data) as a way to write such data. Seemingly every other way to set a background has been deprecated by GTK devs.

I wonder if the gnome fallback session offers an applet like this and if so, how they handled it. About to go looking

[muktupavels]

char *css = g_strdup_printf ("* { background: %s; }", "#ff0000");?

[lukefromdc]

Could that function be used inside gtk_css_provider_load_from_data, gtk_css_provider_load_from_file, gtk_css_provider_load_from_path, or gtk_css_provider_load_from_resource?

[muktupavels]

• get bg and/or fg color string
• build css string with g_strdup_printf or GString or whatever you want
• use dynamically built css with gtk_css_provider_load_from_data
• free allocated memory

You can look at gp-theme.c in gnome-panel: https://git.gnome.org/browse/gnome-panel/tree/gnome-panel/gp-theme.c

[lukefromdc]

As ugly as Raveit called the orginal theme, I'm almost thinking the thing to do would be:

1: find a way to get the insensitive color off the text without making the label selectable 2: pack an existing icon image of a monitor (from the icon theme) into that menuitem 3: use gtk-recolor to match its color to that shown on the monitor.

[lukefromdc]

I've pushed a development branch (more to come on it) to put recolorable icons in the popup menu monitor labels. The first commit to the branch just adds the symbolic icons, and both they and the labels still get the inactive grayout. It will take Albert's suggestions for dynamically building the css data to recolor the icons, and if possible it would also be good to get the label color before it is set insensitive and add that to the css as well, so it works in all themes and without requiring knowing the name the theme gives the color used for normal menuitem labels.

Note that the css syntax changes slightly as I needed a box to pack the image and the label together into the menuitem https://github.com/mate-desktop/mate-settings-daemon/tree/dev-xrandr-applet-monitor-icons

Here is my initial result, with recolorable but not yet recolored icons. The icon used (computer-symbolic) is present in the mate-icon-theme as a good symbolic icon, also in my own as one that might not be as good for that job but is enough for testing right now:

When this is done I want each icon matching the color of the rectangle on the monitor the menu item represents, and the text color matching the other menuitems in all themes.

[muktupavels]

That icon does not look like symbolic icon to me...

After setting sensitive FALSE for menu item, can not you use gtk_widget_unset_state_flags to remove :disabled / insensitive styling for label? Don't know if that will work...

[lukefromdc]

That test icon came out of my icon theme and was deliberately set up to look like that for another reason. It is still an svg icon and may or may not recolor properly. I set the icon name looking at mate-icon-theme, but with my normal theme got another one from my own icon theme.

With Menta I got this:

and with Adwaita-dark I got this:

I tried using gtk_widget_set_state_flags to get rid of the insensitive color but that did not work. I will now try gtk_widget_unset_state_flags

EDIT: if in the end the finished result of all this is merged and my own icon theme (I control it) needs to have the competing icon removed that is no big deal

[lukefromdc]

I found my theme was disabling the symbolic icon, as I normally have them turned off globally but need it here. As a test, I dropped this code into my theme to see what the popup would look like with the monitor rectangle colors on both the symbolic icon and the label. This is just a test with a hardcoded color, next is to build the css from strings inside the application to use the actual colors assigned to the separate monitor squares.

.mate-panel-menu-bar menuitem.xrandr-applet:disabled>box>image,
.mate-panel-menu-bar menuitem.xrandr-applet:disabled>box>label {
    -gtk-icon-style: symbolic;
    color: #e3ffaa; 
}

EDIT: you can see how this would become unreadable on a light theme, thus the need to color the label background instead. At least that works on all themes.

[lukefromdc]

I got recoloring both the text and icon to match the monitors to work-only to find that this is completely unreadable in light themes.

https://github.com/mate-desktop/mate-settings-daemon/commit/ee5c22fd7287b38433ebd71bff7b9baa360dd2be

So I had to return to a colored label background, now matching each monitor, though it looks much better to my eyes when an icon is included before the label.

https://github.com/mate-desktop/mate-settings-daemon/commit/9b729fb5c3bc423ee71752f2ffae1d8b240ec4a8

Trying a non-symbolic icon here, though a way to prevent it from being dimmed would be nice. A symbolic icon could be set to any color, and themes could specify that if desired if it is not hardcoded here. Got these results:

Although it would be possible to use the theme background, get the theme text foreground color, use that on the text, and just match a symbolic icon to the monitor color (my original idea) I found that in themes like Menta that makes the icon as well as the text nearly invisible if the monitor rectangle comes up light yellow.

[lukefromdc]

I opened https://github.com/mate-desktop/mate-settings-daemon/pull/223 against this branch as a second try at a usable follow-on to it. Latest results:

[raveit65]

Why there is any function in Gtk+ like this? Dear Widget please use a defined color from themes like theme_fg_color... This would make all simple here.

[lukefromdc]

For this test of themablity, I added

/*For mate-settings-daemon xrandr-applet's monitor labels*/
.mate-panel-menu-bar menuitem.xrandr-applet:disabled>box>label{
    color: black;
}

to BlueMenta's mate-applications.css file and removed

image,
image:disabled,
label,
label:disabled,

from the bloc below

image,
image:disabled,
label,
label:disabled,
box,
box:disabled,
grid,
grid:disabled {
    background-color: transparent;
}

[raveit65]

You can also use this branch from mate-themes. I did some styling suggestions for our themes for better testing this PR. https://github.com/mate-desktop/mate-themes/tree/xrandr-applet-suggestions

[lukefromdc]

Every theme from https://github.com/mate-desktop/mate-themes/tree/xrandr-applet-suggestions worked fine, I noticed you kept the background only in Traditional-Green and TOK. Screenshots coming

[lukefromdc]

[lukefromdc]

I did not squash most of this, as a squash on a previous merged PR led to issues when a bug had to be bisected

[raveit65]

Well, i don't think that it looks very nice with BlackMate or GreenLaguna. Mint and pink on brown or green menu backround......., does that look nice ? :) And Submarine themes i use for myself (mostly) and black color doesn't accept another color ;) If someone like to see the color background in Menta themes we can add it...... But actually i would prefer a clean serious design for Menta themes.

[lukefromdc]

A lot of the issue in using the available monitor color background is that it will be whatever is returned by the function setting the color in the first place and not predictable by themers. When it is used, the border should be used around it either with its default grey or a color set by the themer and should probably be prominant on any light theme. I get a yellow and a blue for the monitor colors, and they look fine with my theme. Using the border helps visually isolate a clashing color while still showing the color matching the rectangles displayed on each monitor. Also helps ensure these pastels are visible against a light theme.

When the theme background is not used, the border should be transparent so as to ensure it is not seen. Also we can clearly see in this row of screenshots the importance of letting themes control the text color given that the background use is also controlled by themes. Adwaita and the other GNOME themes do end up with the disabled color because of this but it is readable in all cases. It was impractical to make this work in FALLBACK, as seemingly everything matched it, and FALLBACK did not work in the GNOME themes for the text color whereas it worked fine for showing the backgrounds and borders, which look good in every GNOME theme.

Anyway, I kept as much of this themable as possible, even at the expense of themes that do not explicitly support MATE.