mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
192 stars 85 forks source link

tabpopup: Use more contrasting background when not composited #764

Closed cwendling closed 8 months ago

cwendling commented 9 months ago

When composited, the tab popup uses an OSD style which typically has a dark background, so we use a light background highlight color. However, when not composited the popup uses a light color and should thus use a dark highlight. This was done for the workspace popup (#623), but not for the window one, leading to the highlight being hardly visible in several themes.

With a theme using plain white BG (high contrast):

Before After
before after
raveit65 commented 9 months ago

PR does what it promise, but why isn't the real selected color used which is blue for high-contrast and high-contrast-inverse(dark) ?

cwendling commented 9 months ago

but why isn't the real selected color used which is blue for high-contrast and high-contrast-inverse(dark) ?

It's a good question indeed. I can try and see what it gives, but I suggest it to be another PR -- especially as it might have a lot more unexpected impact depending on the theme.

raveit65 commented 9 months ago

Digging deeper i found out that hc and hc-inverse are simply variation of adwaita theme nowadays, and adwaita gives the same light grey color for the selected background than hc theme. I am thinking it is caused by the color definitions of those gtk standard themes. But with our default theme Menta(blue) the selected background color is green or blue. That means all is fine in our code and PR. Don't spend time on it :)

cwendling commented 8 months ago

@raveit65 ACK, thanks looking it up. However, maybe we could be a bit more forgiving and e.g. make the selected image have that flag and paint the theme's background… which seems to work for Adwaita, but might be more complex than that given I'm not entirely sure what the background is actually styled with. Anyway, this is research for later if at all I think indeed :)

Anybody else wanting to chime in? Otherwise I'll merge it in a couple days.

raveit65 commented 8 months ago

Anyway, let's merge. Should we claim this as a bugfix and cherry-pick to 1.26 branch?

cwendling commented 8 months ago

@raveit65 thanks. And yeah, we could consider this a bugfix worth making it to 1.26, as it makes the tabpopup hardly usable with some themes if not using composition, and the code seems simple enough to me to be low risk.

raveit65 commented 8 months ago

@cwendling Merge conflict

[rave@mother marco]$ git cherry-pick b87bed36282d767885da5450da26ddd92f6112fe
Auto-merging src/ui/tabpopup.c
CONFLICT (content): Merge conflict in src/ui/tabpopup.c
error: could not apply b87bed36... tabpopup: Use more contrasting background when not composited

Can you jump in?

cwendling commented 8 months ago

@raveit65 sure, will do, just a sec.

cwendling commented 8 months ago

The conflict is because of #759. Should it go as well? maybe, but then maybe the 1.26.3 will start being closer and closer to 1.27 :smile: Anyway, if I cherry-pick this one and then #759, one needs to beware that it should be updated to affect the the new call from this one! So: do you think we should cherry-pick #759 in 1.26? If so, it's easier to do it before cherry-picking this one.

WFIW, the diff of a cherry-pick as of now (without #759):

diff --git a/src/ui/tabpopup.c b/src/ui/tabpopup.c
index d3b9ba6..55549bc 100644
--- a/src/ui/tabpopup.c
+++ b/src/ui/tabpopup.c
@@ -882,7 +882,10 @@ meta_select_image_draw (GtkWidget *widget,
       h = requisition.height - OUTSIDE_SELECT_RECT * 2;

       gtk_style_context_set_state (context, GTK_STATE_FLAG_SELECTED);
-      meta_gtk_style_get_light_color (context, GTK_STATE_FLAG_SELECTED, &color);
+      if (meta_prefs_get_compositing_manager ())
+        meta_gtk_style_get_light_color (context, GTK_STATE_FLAG_SELECTED, &color);
+      else
+        meta_gtk_style_get_dark_color (context, GTK_STATE_FLAG_SELECTED, &color);

       /* We set the line width absurdly high to overflow it behind the icon. */
       cairo_set_line_width (cr, 256.0);
raveit65 commented 8 months ago

With a lot imagination we can consider https://github.com/mate-desktop/marco/pull/759 as a bugfix for x2go session. It is tested and @vkareh who knows marco well has approved the PR. Also we don't have a date for a new major release. I wouldn't remove conflicting code snippet without testing for stable branch. So yes, i agree with cherry-picking https://github.com/mate-desktop/marco/pull/759 to 1.26.

raveit65 commented 8 months ago

Hmm, do we need https://github.com/mate-desktop/marco/pull/758 too? I thought only https://github.com/mate-desktop/marco/pull/759 is needed.

raveit65 commented 8 months ago

Any way, cherry-pick https://github.com/mate-desktop/marco/pull/758 too, i don't mind. But i am in doubt that debian maintainer will use it , because of debian packaging politics.

lukefromdc commented 8 months ago

I have a 1.27 build out on my archive.org page that can replace Debian's older packages for anyone needing this

At intervals I upload fresh batches of my debian packages (1.27 at this point) to https://archive.org/details/DebianPackagesForMate-desktopWityGtk3AndCustomPanelTheme which dates all the way back to my initial GTK3 work thus the name. Thousands of views, don't have data breaking down downloads. These are all flagged as experimental and come with a warning that this is an unsigned website and to build the packages from the git source if there is a security concern. Matching source and binaries always uploaded

cwendling commented 8 months ago

I just cherry-picked #758, #759 and #764 in1.26