Adjust default color scheme and titlebar appearance for Tools Area
AbandonedPublic

Authored by ngraham on Mar 26 2020, 7:37 PM.

Details

Reviewers
None
Group Reviewers
VDG
Breeze
Maniphest Tasks
T10201: Window titlebars
Summary

This patch adjusts the default Breeze color scheme to produce the final Tools Area
appearance proposed in T10201. The following changes are made:

  • The titlebar background color is darkened a bit
  • The window background color is lightened a bit to increase contrast with the Tools Area
  • The view background is lightened a bit to preserve contrast with the window BG color
  • Other colors that were using the window and view BG colors are updated to the new values
  • The Breeze Light color scheme is removed as the new Breeze color scheme is effectively a light one
  • The titlebar gradient is turned off by default
  • The close button background circle is made red by default
Test Plan

Dolphin:

Kate:

Spectacle:

Konsole:

Okular:

File dialog:

Plasma config window:

Riot:

LibreOffice:

Active/inactive animation:

Diff Detail

Repository
R31 Breeze
Branch
tools-area-color-changes-v3
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25868
Build 25886: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham updated this revision to Diff 79063.Apr 1 2020, 4:33 PM

Make the close button red

ngraham updated this revision to Diff 79066.Apr 1 2020, 4:42 PM

Make 86851917557ea7deeaaac05b3d62e021d93041e9 the base commit

ngraham updated this revision to Diff 79069.Apr 1 2020, 5:15 PM

Lighten the button when the window is inactive

ngraham updated this revision to Diff 79070.Apr 1 2020, 5:16 PM

Make the Tools Area patch the base commit, again

ngraham edited the summary of this revision. (Show Details)Apr 1 2020, 5:17 PM
ngraham edited the test plan for this revision. (Show Details)Apr 1 2020, 5:42 PM
ndavis added a comment.Apr 1 2020, 6:04 PM

@ndavis like this?

yes

yes

Done. :)

ngraham edited the summary of this revision. (Show Details)Apr 1 2020, 6:11 PM
ndavis added a comment.Apr 1 2020, 6:12 PM

When I try to get this patch with arc patch on top of the branch for D27669, git says a cherry pick failed.

You don't need to already be on the other branch, just apply this on top of master. If all goes well, arc should first apply D27669, then this one.

ngraham edited the summary of this revision. (Show Details)Apr 1 2020, 6:13 PM
ndavis added a comment.Apr 1 2020, 6:15 PM

You don't need to already be on the other branch, just apply this on top of master. If all goes well, arc should first apply D27669, then this one.

If I do it that way, it says "This diff is against commit 86851917557ea7deeaaac05b3d62e021d93041e9, but the commit is nowhere in the working copy."

If you tell it to proceed after that, does it succeed or fail?

ndavis added a comment.Apr 1 2020, 6:16 PM

it succeeds. It probably doesn't need whatever commit it's trying to use.

ndavis added a comment.EditedApr 1 2020, 7:14 PM

Do we really still need Breeze Light to be separate from Breeze? I think it would make more sense to have just Breeze Light and Breeze Dark colorschemes (delete Breeze Light, rename Breeze to Breeze Light). Maybe Breeze Classic too if people want the old Breeze colorscheme, but that could be on store.kde.org instead. After that, we should refer to all light versions of Breeze as Breeze Light instead of just Breeze. Breeze should then only be used to refer to the complete theme or a color scheme compatible version of Breeze (i.e., Breeze plasma theme).

But would we still have Breeze, Breeze Light, and Breeze Dark Plasma themes? If we're going to get rid of the Breeze color scheme and just have light and dark, it might be a bit odd to still have a Breeze Plasma theme.

Also maybe we should discuss that after this patch, or separately?

ndavis added a comment.Apr 1 2020, 8:07 PM

But would we still have Breeze, Breeze Light, and Breeze Dark Plasma themes? If we're going to get rid of the Breeze color scheme and just have light and dark, it might be a bit odd to still have a Breeze Plasma theme.

[...] Breeze should then only be used to refer to the complete theme or a color scheme compatible version of Breeze (i.e., Breeze plasma theme).

I guess you could call it Breeze Adaptive or Breeze Chameleon, but I don't think it's necessary.

Also maybe we should discuss that after this patch, or separately?

I don't think eliminating one of the light color schemes needs to be done separately. Discussions about what to do about plasma themes and icon themes are outside the scope of this patch, but I just wanted you to see where I was going with that suggestion.

It's not a terrible idea, I just need to think about it a bit.

ngraham updated this revision to Diff 80759.Apr 21 2020, 12:49 PM

Remove Breeze Light color scheme, per @ndavis's suggestion (I came to agree that it doesn't make sense to keep it around here; we can put it on GHNS along with "Breeze Classic"

I think we may need a kconf update script to migrate people to the new colors, since the colors in the current color scheme get cached in their config files.

ngraham edited the summary of this revision. (Show Details)Apr 21 2020, 12:56 PM
ngraham updated this revision to Diff 80762.Apr 21 2020, 1:02 PM

Don't include merge conflict bits lol

This time when I try to apply the patch, it fails. I have the most up to date version of D27669 too.

╭ …/src/kde/workspace/breeze   master  
╰ arc patch D28317
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D28317.
Branch name arcpatch-D27669 already exists; trying a new name.
Created and checked out branch arcpatch-D27669_1.
Checking patch kstyle/config/ui/breezestyleconfig.ui...
Checking patch kstyle/config/breezestyleconfig.cpp...
Checking patch kstyle/breezetoolsareamanager.h...
Checking patch kstyle/breezetoolsareamanager.cpp...
Checking patch kstyle/breezestyle.h...
Checking patch kstyle/breezestyle.cpp...
Checking patch kstyle/breezehelper.h...
Checking patch kstyle/breezehelper.cpp...
Checking patch kstyle/breeze.kcfg...
Checking patch kstyle/CMakeLists.txt...
Applied patch kstyle/config/ui/breezestyleconfig.ui cleanly.
Applied patch kstyle/config/breezestyleconfig.cpp cleanly.
Applied patch kstyle/breezetoolsareamanager.h cleanly.
Applied patch kstyle/breezetoolsareamanager.cpp cleanly.
Applied patch kstyle/breezestyle.h cleanly.
Applied patch kstyle/breezestyle.cpp cleanly.
Applied patch kstyle/breezehelper.h cleanly.
Applied patch kstyle/breezehelper.cpp cleanly.
Applied patch kstyle/breeze.kcfg cleanly.
Applied patch kstyle/CMakeLists.txt cleanly.
 COMMITTED  Successfully committed patch.


    This diff is against commit 33be688c002cb4829fee475f0199cbffbb8dc265, but
    the commit is nowhere in the working copy. Try to apply it against the
    current working copy state? (944d47cd098ff8abf85f08823cbe7d033eee082d)
    [Y/n] y

Checking patch kstyle/breezetoolsareamanager.cpp...
error: while searching for:
                    this, [this]() {
                        emit toolbarUpdated();
                    });
<<<<<<< HEAD
            toolbar->installEventFilter(this);
=======
>>>>>>> c2487fc1... [kstyle] Tools area
        }
        connect(widget, &QObject::destroyed,
                this, [this, widget]() {

error: patch failed: kstyle/breezetoolsareamanager.cpp:187
error: while searching for:

    void ToolsAreaManager::unregisterWidget(QWidget *widget)
    {
<<<<<<< HEAD
        if (qobject_cast<QToolBar*>(widget)) {
            widget->setContentsMargins(0,0,0,0);
            widget->removeEventFilter(this);
        }
=======
        if (qobject_cast<QToolBar*>(widget)) widget->setContentsMargins(0,0,0,0);
>>>>>>> c2487fc1... [kstyle] Tools area
        _registeredWidgets.remove(widget);
        QList<QWindow*> toRemove;
        for (auto window : animationMap.keys()) {

error: patch failed: kstyle/breezetoolsareamanager.cpp:213
Checking patch kdecoration/breezesettingsdata.kcfg...
Checking patch kdecoration/breezebutton.cpp...
Checking patch colors/BreezeLight.colors...
Checking patch colors/Breeze.colors...
Applying patch kstyle/breezetoolsareamanager.cpp with 2 rejects...
Rejected hunk #1.
Rejected hunk #2.
Applied patch kdecoration/breezesettingsdata.kcfg cleanly.
Applied patch kdecoration/breezebutton.cpp cleanly.
Applied patch colors/BreezeLight.colors cleanly.
Applied patch colors/Breeze.colors cleanly.

 Patch Failed!
Usage Exception: Unable to apply patch!

the cmake file in colors/ also needs to be updated for the patch to build.

ngraham updated this revision to Diff 81021.Apr 23 2020, 5:02 PM

Rebase on e9d658fcb1f9751a39180e79ccc66aff3a18599d

ngraham updated this revision to Diff 81022.Apr 23 2020, 5:05 PM

Remove leftover CMake bit

ngraham updated this revision to Diff 81023.Apr 23 2020, 5:07 PM

Rebase against tools area patch

ngraham updated this revision to Diff 81024.Apr 23 2020, 5:11 PM

Remove merge conflict

ngraham updated this revision to Diff 81025.Apr 23 2020, 5:11 PM

Change the patch base, again

ngraham updated this revision to Diff 81026.Apr 23 2020, 5:12 PM

Arc, you are the worst

How are you guys feeling about the colors here? The Tools Area color currently used in this patch and the mockups demos well, but I'm starting to wonder if we're regressing usability in the process. I'm starting to think that the color isn't dark enough to adequately signal that a window is active or inactive. Current Breeze is very obvious about this, but the new color is quite subtle, especially for windows with only a titlebar. It's more obvious in windows with a large Tools Area like Okular or Gwenview since there's a large area of color change and all the text and icons lighten too, but I feel like for windows with less stuff in the Tools Area (e.g. dialogs and 3rd-party apps) it takes me longer to figure out at a glance which window is active than it did before. Do other folks find the same thing? I wonder if we might want to experiment with making the Titlebar/Tools Area gray color a touch darker. Thoughts?

ndavis added a comment.EditedApr 23 2020, 6:14 PM

It's the change to kstyle/breezetoolsareamanager.cpp that causes this patch to fail to apply. That file doesn't exist anywhere. It's not even in the tools area patch.

kstyle/breezetoolsareamanager.cpp is added in D27669.

Maybe the problem is that D27669 has three commits in it, so arc gets confused?

Never mind, I was wrong, I see it in arcpatch-D27669. However, just to see what would happen, I made my arcpatch-D27669 branch the upstream of arcpatch-D28317 and it said that arcpatch-D28317 was behind a commit. Are you sure you've rebased against the tools area patch correctly?

Definitely not sure. If you have an idea of what went wrong or how to fix it, I'm all ears.

The event filter in breezetoolsareamanager.cpp already exists in the tools area patch and the other changes are incompatible with the current version of the tools area patch.

I mean, how to fix it with arc. :)

I mean, how to fix it with arc. :)

I don't know what the branch looks like on your end. Maybe just zip up the whole repo and send it to me?

ndavis added a comment.EditedApr 23 2020, 7:04 PM

No wait, that would be half a gigabyte of stuff. Maybe not the best idea to zip it up unless you use something like Google Drive to send it.

ngraham updated this revision to Diff 81039.Apr 23 2020, 7:14 PM

Hopefully fix it

Anyway color-wise, here's an example of using a darker tools area color to improve the at-a-glance detectability of the active window:

Thoughts?

ndavis added a comment.EditedApr 29 2020, 12:24 PM

Try this colorscheme for a while and see what you think of it:

Try this colorscheme for a while and see what you think of it:

Not bad! I like that the buttons are lighter colored. In fact I feel like they could be a bit lighter still and look even better. But I feel like the Tools Area background isn't sufficiently darker than the normal window background--especially for windows where the Tools Area is just a titlebar. IMO the former needs to be darkened or the latter lightened. Also using the normal text color for selected items kind of feels like it needs the default selection color to be lighter or else readability is impaired a bit.

ndavis added a comment.EditedApr 29 2020, 5:39 PM

Also using the normal text color for selected items kind of feels like it needs the default selection color to be lighter or else readability is impaired a bit.

Oops, that's a leftover bit from when I was testing my changes to the Breeze QStyle. This color scheme is something I've been slowly working on for a while.

New version with fixed selection color and darker titlebar:

Thanks, that's fixed now. Is the tooltip background supposed to be this grayish color though?

If we're go with a light tooltip, I think I'd prefer it to look like the current Plasma tooltip, which IMO is nicer with a lighter background.

ndavis added a comment.EditedApr 29 2020, 8:01 PM

Thanks, that's fixed now. Is the tooltip background supposed to be this grayish color though?

If we're go with a light tooltip, I think I'd prefer it to look like the current Plasma tooltip, which IMO is nicer with a lighter background.

I wanted it to stick out from the window and view backgrounds a bit more. Here I've changed it to use the color of buttons:

Forgot to change a few more of the selection colors. I might as well post the updated Breeze Dark version too.

Do we really still need Breeze Light to be separate from Breeze? I think it would make more sense to have just Breeze Light and Breeze Dark colorschemes (delete Breeze Light, rename Breeze to Breeze Light). Maybe Breeze Classic too if people want the old Breeze colorscheme, but that could be on store.kde.org instead. After that, we should refer to all light versions of Breeze as Breeze Light instead of just Breeze. Breeze should then only be used to refer to the complete theme or a color scheme compatible version of Breeze (i.e., Breeze plasma theme).

Back to this: Now that Breeze Light is gone, what if we rename Breeze to Breeze Light? How will that affect people upgrading from the old version of Breeze or Breeze Light?

Also, since I'm working on the color schemes, do you think it would make more sense for the color scheme to be a separate patch from the titlebar button changes?

Do we really still need Breeze Light to be separate from Breeze? I think it would make more sense to have just Breeze Light and Breeze Dark colorschemes (delete Breeze Light, rename Breeze to Breeze Light). Maybe Breeze Classic too if people want the old Breeze colorscheme, but that could be on store.kde.org instead. After that, we should refer to all light versions of Breeze as Breeze Light instead of just Breeze. Breeze should then only be used to refer to the complete theme or a color scheme compatible version of Breeze (i.e., Breeze plasma theme).

Back to this: Now that Breeze Light is gone, what if we rename Breeze to Breeze Light? How will that affect people upgrading from the old version of Breeze or Breeze Light?

Also, since I'm working on the color schemes, do you think it would make more sense for the color scheme to be a separate patch from the titlebar button changes?

Yeah, and it might make sense to roll the titlebar close button and defaults changes into the parent patch (@cblack, feel free to do that) and then I'll abandon this patch and you can do the color scheme patch.

Changing the name makes sense I think, Then we'll just have "Breeze Light" and "Breeze Dark". If we do this, we should put the current Breeze on store.kde.org via GHNS for sure.