Differential D7537

KDE logout screen - change black icons & texts to white/greyish
Needs RevisionPublic
Actions

Authored by pekkah on Aug 25 2017, 12:18 PM.

Details

Reviewers
mart
Group Reviewers
Plasma: Workspaces
Plasma
Breeze
Summary

The patch partially fixes an issue where all logout-related buttons are colored as black.

Issue:
Current logout screen uses a black background which renders blackish logout-related buttons and icons difficult to read for an user.

Solution:
This patch changes color of logout-related buttons and texts to white so they can easily be seen in the logout view which uses black as background color. The trick here is use contrary colors which improves the user experience.

Todo:

This patch does not fix an issue which regards to user icon and user name. In code, they are combined into one element. Currently, if we change color of the element to white, a custom user icon is filled with white as well. If default KDE icon is used, the issue exists but is not so visible because the icon element itself is partially a transparent element and only opaque parts are filled with white. Solution here would be to use user name as a separate element so it can be colored individually, and possibly make some code changes into file "/lookandfeel/contents/components/UserDelegate.qml" where the icon style is defined.

Currently, user name and user icon are not re-colored to white. So this patch is more or less a partial implementation/fix.

Unfinished code is somewhat commented out in the patch file but can be used later on if needed.

Test Plan

This patch changes the colors as defined in summary. No side effects seen so far.

Using the patch requires compiling plasma-workspace package from source. The patch has been tested on the latest plasma-workspace package version available on Arch Linux (5.10.5-2) which has been compiled from source using a slightly customised PKGBUILD (added the patch file into source list) available on Arch Linux Package Repository: https://git.archlinux.org/svntogit/packages.git/tree/trunk?h=packages/plasma-workspace

Without the patch, the issue is not fixed. This behavior has been tested & confirmed before the patch file was implemented into source code.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
pekkah created this revision.Aug 25 2017, 12:18 PM
Restricted Application edited projects, added Plasma; removed Plasma: Workspaces. · View Herald TranscriptAug 25 2017, 12:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pekkah retitled this revision from KDE logout window - change black icons & texts to white/greyish to KDE logout screen - change black icons & texts to white/greyish.Aug 25 2017, 12:24 PM
pekkah added a reviewer: Breeze.Aug 25 2017, 1:17 PM
pekkah added a subscriber: Breeze.
bshah added a subscriber: bshah.Aug 25 2017, 1:27 PM

Can you provide screenshot?

mart requested changes to this revision.Aug 25 2017, 1:32 PM
mart added a subscriber: mart.

screenshots are necessary for visual changes

lookandfeel/contents/logout/Logout.qml
140

remove dead code

170

why is changing buttons order?

202

no hardcoded colors, ever, also on any theme that has colored icons (so anything but breeze) this will break

216

no hardcoded colors

This revision now requires changes to proceed.Aug 25 2017, 1:32 PM
pekkah added a comment.EditedAug 25 2017, 1:38 PM
In D7537#139913, @bshah wrote:

Can you provide screenshot?

Sure. I have opened a new bug issue on KDE Bugzilla. You can find it here: https://bugs.kde.org/show_bug.cgi?id=382264

Screenshots

I have submitted them as a part of the Bugzilla bug report. Direct links below.

Before: https://bugs.kde.org/attachment.cgi?id=106572

  • Before the patch has been applied

After: https://bugs.kde.org/attachment.cgi?id=106617

  • After the patch has been applied
mart added a comment.Aug 25 2017, 1:56 PM

phabricator supports screenshots by itself, that should be used.

ngraham added a subscriber: ngraham.Mar 11 2018, 12:28 AM

@pekkah? Can you add the screenshots here and address the review comments?

ngraham added a comment.Mar 11 2018, 12:28 AM

Also, is this even still relevant?

mvourlakos added a subscriber: mvourlakos.Mar 11 2018, 1:07 AM

I dont know if I can help in this... I used the ColorOverlay approach lately with Latte in order to color properly the panel contents based on the underlying background.
This approach could be also be used here I think.

The luminosity of the textColor and the backgroundColor of the plasma theme could be calculated and the one that provides the most brightness is chosen ...

Pitel added a subscriber: Pitel.Mar 11 2018, 6:55 AM

I have the same problem in 5.12.2 release. I think the root of it is assuming that foreground in PlasmaCore.Theme.ComplementaryColorGroup is white(ish) and background is dark (this is true only if normal color scheme uses dark text on light background, which is default) and forcing that background is really dark (since D5036). My personal hack is just to replace ComplementaryColorGroup with NormalColorGroup (which obviously breaks default setup). A bit less of a hack would be choosing either NormalColorGroup or ComplementaryColorGroup depending on which one has darker background.

The same problem also hits lock screen and user switcher.

mvourlakos added a comment.Mar 11 2018, 8:09 AM

@Pitel how did you manage to reproduce this, with what color theme? In my system logout screen always appears whitish in master branch

Pitel added a comment.Mar 11 2018, 9:01 AM

@mvourlakos I did some more testing and I am really confused. The important color seems to be button text color in Plasma theme setting. Results:

With colorGroup: PlasmaCore.Theme.ComplementaryColorGroup (which is currently hardcoded):

  • with white button text color: all text is white -- OK
  • with black button text color: all text is black -- not OK for text outside of buttons

With colorGroup: PlasmaCore.Theme.NormalColorGroup (or just deleting colorGroup property):

  • with white button text color: all text is white -- OK
  • with black button text color: text on buttons is black, other text is white -- OK

I have no idea why button text color affects color of text outside of buttons with Complementary color group but does not affect it with Normal.

(By buttons I mean OK and Cancel who draw their backgrounds.)

Pitel added a comment.Mar 11 2018, 9:58 AM

I guess I am not that confused anymore -- with normal color group the color of text (outside of buttons) is determined by text color theme setting but with complementary group by button text color.

I think correct solution would be to construct ColorScope with textColor set to default text color if it is not too dark and white otherwise (the background color is handled in similar way). The problem is I do not see any way to construct ColorScope with custom colors -- it always takes colors from some color group of current theme.

davidedmundson added a subscriber: davidedmundson.Mar 12 2018, 11:49 AM

I'm a bit confused.

Your "before" screenshot doesn't match what I see, and yet your bug report mentions "normal Breeze theme".

In the current code, it should be loading the complementary colour set (which effectively is the "dark background colour set") which will theme icons appropriately.
Clearly we have some issue there that needs fixing, but it's a bug in the colourscope/icon handling code, not in the frontend QML.

I'll follow that up on your bug report.

Pitel added a comment.Mar 12 2018, 12:50 PM

I finally found what was wrong here: the background should not be black when button background color is light but the calculation introduced by D5036 is wrong. My attempt to fix it is D11262.

ngraham added a comment.Mar 14 2018, 6:58 PM

You can abandon the revision in favor of that other one using Phabricator's "Add Action..." menu above the comment field.

Revision Contents
Changeset List

Diff 18746

View Options

lookandfeel/contents/logout/Logout.qml

Loading...