Change color(NegativeBackground) of status bar in space info when storage exceeds 90%
Needs ReviewPublic

Authored by meven on Oct 21 2018, 2:12 PM.

Details

Summary

When used space in computer exceeds 90% of total storage the color of progressbar(On status bar) changes to NegativeBackground of KColorScheme

Test Plan

Breeze dark:

Fusion Check Color:

(Code was tweaked to be in red at 50%)

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D16353_2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21237
Build 21255: arc lint + arc unit
sourabhboss created this revision.Oct 21 2018, 2:12 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptOct 21 2018, 2:12 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
sourabhboss requested review of this revision.Oct 21 2018, 2:12 PM

My First Contribution

sourabhboss edited the summary of this revision. (Show Details)Oct 21 2018, 2:39 PM
sourabhboss added reviewers: pino, elvisangelaccio.

Please add before and after screenshot.

Indeed, screenshots would be nice. Thanks for the patch!

I would also say that the threshold might be better set at 90% than 80%. The typical recommendation *for any OS) is to leave 10% of the boot disk free, which a threshold of 90% would match.

shubham removed a subscriber: shubham.Oct 21 2018, 3:52 PM

Threshold set to 90%

pino requested changes to this revision.Oct 21 2018, 4:03 PM

Not sure why I was added as reviewer... anyway:

  • no need to use this-> to call own class members, unless there is a conflict (which does not look like)
  • please never hardcode colors! use KColorScheme instead
  • it does not seem that the palette is reverted back when the space changes to less than the threshold
This revision now requires changes to proceed.Oct 21 2018, 4:03 PM

Screenshot - when used storage is below threshold
Screenshot - when used storage is above threshold

In D16353#346792, @pino wrote:

Not sure why I was added as reviewer... anyway:

  • no need to use this-> to call own class members, unless there is a conflict (which does not look like)
  • please never hardcode colors! use KColorScheme instead
  • it does not seem that the palette is reverted back when the space changes to less than the threshold

Thanks @pino I will update revision soon

Also, you'll need to change the title, which still says 80%.

cfeck added a subscriber: cfeck.Oct 22 2018, 1:19 AM

Additionally, it could remember which of the two colors it currently uses to not always re-allocate a new palette for each change, but only for crossings.

sourabhboss retitled this revision from Change color(red) of status bar in space info when storage exceeds 80% to Change color(red) of status bar in space info when storage exceeds 90%.Oct 22 2018, 2:10 AM
sourabhboss edited the summary of this revision. (Show Details)

this-> code removed
if used space reduce it revert back

ngraham requested changes to this revision.Oct 22 2018, 1:50 PM

Thanks. But this is still using a hardcoded color instead of the Negative color from the color scheme.

This revision now requires changes to proceed.Oct 22 2018, 1:50 PM

Thanks. But this is still using a hardcoded color instead of the Negative color from the color scheme.

Yeah. I will soon update diff with required changes

Instead of hardcoded colors, ColorScheme is used

Thank You @ngraham and @cfeck

sourabhboss retitled this revision from Change color(red) of status bar in space info when storage exceeds 90% to Change color(NegativeBackground) of status bar in space info when storage exceeds 90%.Oct 23 2018, 2:16 PM
sourabhboss edited the summary of this revision. (Show Details)
cfeck added a comment.Oct 23 2018, 2:27 PM

Bonus points if you only change the palette when crossing the 90% mark, not on every change.

src/statusbar/statusbarspaceinfo.cpp
28

`#include <KColorScheme>' (and move above the KLocalizedString, we (sometimes) try to keep includes alphabetical)

116

} else {

sourabhboss updated this revision to Diff 44148.EditedOct 24 2018, 10:47 AM
sourabhboss marked 2 inline comments as done.

Minor Changes done
It is reverting back on increasing the storage to 90% on current window (tested)

} else { //done!!

import also corrected

src/statusbar/statusbarspaceinfo.cpp
110

Why KColorScheme::Selection ? That should be used for items that can be selected.

We should probably use KColorScheme::Window instead

113

Why QPalette::Highlight ? That's supposed to be used for the selected text's color

114

What's the second setBrush() needed for?

cfeck added a comment.Oct 24 2018, 8:57 PM

The intension is to colorize the 'progressbar', which uses Highlight color from the palette. For styles that draw the progressbar text inside the progress bar, you need to also change the HighlightedText color, to ensure that the text stays readable on the changed background.

sourabhboss marked an inline comment as done.Oct 25 2018, 10:40 AM

@elvisangelaccio i have used

KColorScheme::Window

instead of ::Selection then on NormalBackground color appears to be white

src/statusbar/statusbarspaceinfo.cpp
113

Then what should i use
For text i have used

QPalette::HighlightedText

Ping! Any update on this?

meven added a subscriber: meven.Dec 18 2019, 8:27 AM

ping @sourabhboss
That's a nice change, not much work still needed IMO

meven commandeered this revision.Sat, Dec 28, 1:55 PM
meven added a reviewer: sourabhboss.
meven updated this revision to Diff 72294.Sat, Dec 28, 1:56 PM
meven marked an inline comment as not done.

Update following review, commandeer revision, rebase

meven marked 4 inline comments as done.Sat, Dec 28, 1:56 PM
ngraham accepted this revision.Sat, Dec 28, 4:46 PM
cfeck added inline comments.Sat, Dec 28, 5:03 PM
src/statusbar/statusbarspaceinfo.cpp
115

Shouldn't this be QPalette::HightlightedText?

cfeck added inline comments.Sat, Dec 28, 5:07 PM
src/statusbar/statusbarspaceinfo.cpp
115

... and it should also be in the previous if branch ? I was confused until I read the actual mail.

So the threshold is now 50%? Isn't that a bit too low?

src/statusbar/statusbarspaceinfo.cpp
109

Please use a descriptive variable name (e.g. palette)

meven updated this revision to Diff 72327.Sun, Dec 29, 1:03 PM
meven marked 2 inline comments as done.

Fix percent, improve variable name

meven added a comment.Sun, Dec 29, 1:04 PM

The missing screenshot (with breeze dark) :

meven updated this revision to Diff 72328.Sun, Dec 29, 1:09 PM

Add a tooltip displaying the % of disk usage

cfeck added a comment.Sun, Dec 29, 2:35 PM

You marked my issues as Done but didn't change the code there. Please test with CheckColorRoles on Fusion widget style.

meven marked an inline comment as done.EditedSun, Dec 29, 4:46 PM

You marked my issues as Done but didn't change the code there. Please test with CheckColorRoles on Fusion widget style.

Thank you for pointing this out.

Default KCapactityBar style uses palette().window().color() to get its filling color (QPalette::Base color).

But breeze uses:

//! styled painting for KCapacityBar
QStyle::ControlElement CE_CapacityBar;

from breeze/kstyle/breezestyle.h

My curront code works for breeze but not for Fusion.

Using for fusion :

new_palette.setBrush(QPalette::Base, colorScheme.foreground(KColorScheme::NegativeText));

Works.

So I guess I have to cover the two cases, breeze and more fusion like themes.

meven added a comment.Thu, Jan 16, 1:51 PM

You marked my issues as Done but didn't change the code there. Please test with CheckColorRoles on Fusion widget style.

Thank you for pointing this out.

Default KCapactityBar style uses palette().window().color() to get its filling color (QPalette::Base color).

But breeze uses:

//! styled painting for KCapacityBar
QStyle::ControlElement CE_CapacityBar;

from breeze/kstyle/breezestyle.h

My curront code works for breeze but not for Fusion.

Using for fusion :

new_palette.setBrush(QPalette::Base, colorScheme.foreground(KColorScheme::NegativeText));

Works.

So I guess I have to cover the two cases, breeze and more fusion like themes.

Does this make sense @cfeck ?

cfeck added a comment.Thu, Jan 16, 2:57 PM

Could you please first update the code here to request feedback? I still see new_palette.setBrush(QPalette::Highlight, colorScheme.foreground(KColorScheme::ActiveText)) which is wrong, because QPalette::Highlight is a background color role, not a foreground color role.

meven edited the test plan for this revision. (Show Details)Thu, Jan 16, 3:40 PM
meven updated this revision to Diff 73714.Thu, Jan 16, 3:43 PM

Support fusion style based KCapacityBar

meven added a comment.EditedThu, Jan 16, 3:44 PM

Could you please first update the code here to request feedback? I still see new_palette.setBrush(QPalette::Highlight, colorScheme.foreground(KColorScheme::ActiveText)) which is wrong, because QPalette::Highlight is a background color role, not a foreground color role.

Well as I said breeze uses QPalette::Highlight to draw CapacityBar ( rendered as ProgressBar by breeze) in breezestyle.cpp Style::drawProgressBarContentsControl( const QStyleOption* option, QPainter* painter, const QWidget* ) const

So I have updated the code to handle both case : breeze style and Fusion based style.

I have added screenshots to the commit comment.