kupiqu / SierraBreezeEnhanced

Originally a fork of BreezeEnhanced to make it (arguably) more minimalistic and informative
GNU General Public License v3.0
216 stars 34 forks source link

Title bar text color doesn't respect the color theme setting #63

Closed nwwdles closed 2 years ago

nwwdles commented 4 years ago

Latest versions (after d4b4947b1824ddf53c432bd5eedafcdcabfc1654) seem not to respect the color setting. It's either white or black and you can't even choose which.

Screenshot_20191212_213922

Screenshot_20191212_213226

kupiqu commented 4 years ago

There is a heuristic to always show a color that has good contrast indep of the background color

nwwdles commented 4 years ago

This heuristic doesn't really work well, as depicted in the second screenshot. White on orange is harder to read than black on orange. Is there a way to disable it?

kupiqu commented 4 years ago

There is always room for improvement... but please note that I need to make it work for all possible colors, which is tricky.

There is no way to disable it. I guess I could use that of the color theme, and the heuristic when matchtitlebartowindowcolor is enabled, or set an option to enable the heuristic.

I'll think about it...

ray-holmium commented 4 years ago

I would also appreciate an option to disable the heuristic! In my case, I want to hide the titlebar text by matching its color to the background of the titlebar. I.e. black text on a black title bar = no more text, clean bars. Cheers!

kupiqu commented 4 years ago

I don't have time to work on this. If someone wants to submit a pull request, I will review it.

kupiqu commented 4 years ago

I'll try to find time to work on this issue

nwwdles commented 4 years ago

this would be great, thanks! i wanted to fix it but, basically, i just copy-pasted some code back from d4b4947 and called it a day

diff --git a/breezedecoration.cpp b/breezedecoration.cpp
index b78e22b..836ed02 100644
--- a/breezedecoration.cpp
+++ b/breezedecoration.cpp
@@ -309,6 +309,17 @@ namespace Breeze
     {
         auto c = client().toStrongRef().data();

+        if ( !matchColorForTitleBar() ) {
+            if ( m_animation->state() == QAbstractAnimation::Running ) {
+                return KColorUtils::mix(
+                    c->color( ColorGroup::Inactive, ColorRole::Foreground ),
+                    c->color( ColorGroup::Active, ColorRole::Foreground ),
+                    m_opacity );
+            }
+
+            return  c->color( c->isActive() ? ColorGroup::Active : ColorGroup::Inactive, ColorRole::Foreground );
+        }
+
         QColor darkTextColor( !c->isActive() && matchColorForTitleBar() ? QColor(81, 102, 107) : QColor(34, 45, 50) );
         QColor lightTextColor( !c->isActive() && matchColorForTitleBar() ? QColor(192, 193, 194) : QColor(250, 251, 252) );

as i was too lazy to go around and fix monochrome icon colors too (as IMO it feels weird when you have white buttons and dark text or vice versa (e.g with #eda771 titlebar color)). if you're fine with the diff (at least as a temporary solution), i could make a PR

StarrKiss commented 3 years ago

Can you add this as a PR please? The author doesn't seemt o be active...

StarrKiss commented 3 years ago

@nwwdles I tried this out on my own machine, but for some reason it only works if Match Titlebar to WIndow is ACTIVE, even though the code specifically says if it's NOT ACTIVE... I don't know why. image So idk about that. Works if I check it though!

StarrKiss commented 3 years ago

I tried changing the code to remove the ! in front of the boolean check- for some reason it didn't seem to change anything at all once built. SO I have no idea about that..

nwwdles commented 3 years ago

@StarrKiss, the patch seems to be still working for me.

screenshot ![Screenshot-20201215193352-1238x913](https://user-images.githubusercontent.com/29466521/102246907-53081f80-3f10-11eb-9255-c3323df69ad0.png)

It's not really good for PR, because it doesn't fix colors for symbolic sierra*-style buttons (but Gnome and Plasma styles are fine). I'm not going to use KDE for the time being, so I don't want to spend time on fixing it. But here's the project with the patch applied https://github.com/nwwdles/SierraBreezeEnhanced

kupiqu commented 3 years ago

I'll try to find time in christmas time to take a look (running very busy lately)

W54J04S07T commented 3 years ago

A simple "follow color scheme exactly" entry along with associated logic would probably be your fastest approach.

Although, I get the impression most here would be happier with a "Match TitleBar to Color Scheme" entry that might be even simpler.

BTW, this project is awesome!!

kupiqu commented 3 years ago

@nwwdles could you share this colortheme for testing purposes?

Screenshot_20191212_213226

nwwdles commented 3 years ago

@kupiqu sure, I think it should be this one https://store.kde.org/p/1314024/ (the screenshot seems to be mismatched)

kupiqu commented 3 years ago

I fail to reproduce. I see the same behavior in Breeze:

image

kupiqu commented 3 years ago

In SBE:

image

nwwdles commented 3 years ago

seems like i might have customized the theme and set the active fg color to black manually, this was a while ago. that's pretty much a one line edit

--- CommonalityClassic.colors   2020-12-27 03:47:08.357529884 +0300
+++ CommonalityClassic2.colors  2020-12-27 03:47:25.817530218 +0300
@@ -112,7 +112,7 @@
 [WM]
 activeBackground=237,167,113
 activeBlend=138,94,131
-activeForeground=248,248,248
+activeForeground=0,0,0
 inactiveBackground=151,153,155
 inactiveBlend=137,143,154
 inactiveForeground=248,248,248
kupiqu commented 3 years ago

It seems there's no more solution than adding an option to strictly follow the colortheme for the color buttons and titlebar text

Update: working on it

kupiqu commented 3 years ago

I created a branch for this, called issue63 (https://github.com/kupiqu/SierraBreezeEnhanced/tree/issue63). In its current state, I set true's and false's in the right places so the color theme is applied to titlebar text and markers instead of the heuristic.

What needs to be done:

  1. create the options in settings and pass them so true's/false's can be replaced with the proper option state

In addition, although less 'urgent', it would be nice if the monochrome symbols, sierra and dark-aurorae styles, could follow the same behavior of that of the plasma style.

Note: I'm super busy at the moment and I am not sure when I will be able to finish point (1) above, so if anyone wants to help on this, I am very much open to review pull requests on this. This shouldn't be that difficult as one just needs to see how options are managed and add a new one for using the default color theme vs the heuristic.

kupiqu commented 3 years ago

I'll set this as a low priority bug for now, maybe somebody picks it up.

kupiqu commented 2 years ago

My aim is that SBE is maintained by the community. This shouldn't be hard to fix. I hope someone in the community cares about SBE and submits a pull request about it... I could help if somebody wants to try but is lost where to start.