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

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_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20367
Build 20385: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
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.Dec 28 2019, 1:55 PM
meven added a reviewer: sourabhboss.
meven updated this revision to Diff 72294.Dec 28 2019, 1:56 PM
meven marked an inline comment as not done.

Update following review, commandeer revision, rebase

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

Shouldn't this be QPalette::HightlightedText?

cfeck added inline comments.Dec 28 2019, 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.Dec 29 2019, 1:03 PM
meven marked 2 inline comments as done.

Fix percent, improve variable name

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

The missing screenshot (with breeze dark) :

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

Add a tooltip displaying the % of disk usage

cfeck added a comment.Dec 29 2019, 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.EditedDec 29 2019, 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.Jan 16 2020, 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.Jan 16 2020, 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)Jan 16 2020, 3:40 PM
meven updated this revision to Diff 73714.Jan 16 2020, 3:43 PM

Support fusion style based KCapacityBar

meven added a comment.EditedJan 16 2020, 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.

meven added inline comments.Feb 18 2020, 3:14 PM
src/statusbar/statusbarspaceinfo.cpp
121

Is this the correct way to do this ?

cfeck added a comment.EditedFeb 18 2020, 11:30 PM

Sorry, it is still wrong. QPalette and KColorScheme have a fixed set of compatible "bg + fg" colors.

For QPalette:

  • Base/AlternateBase + Text
  • Window + WindowText
  • Button + ButtonText
  • Highlight + HighlightedText
  • ToolTipBase + ToolTipText

(note that Background + Foreground is an obsolete way to express Window + WindowText)

For KColorScheme, there are many more compatible combinations. Basically, for each "set" of colors (View, Window, Button, Selection, ToolTip, which map to the QPalette backgrounds above), there are many separate background roles and matching foreground/text roles.

So the statement palette.setBrush(QPalette::Highlight, colorScheme.foreground(KColorScheme::ActiveText)) is wrong, because you are trying to set a fg/text color as a background color. Same for palette.setBrush(QPalette::Background, colorScheme.foreground(KColorScheme::NegativeText)).

meven updated this revision to Diff 76154.Feb 22 2020, 10:24 AM
  • Fix colors following semantics
meven added a comment.EditedFeb 22 2020, 10:52 AM

Sorry, it is still wrong.

I am glad to learn.

QPalette and KColorScheme have a fixed set of compatible "bg + fg" colors.

For QPalette:

  • Base/AlternateBase + Text
  • Window + WindowText
  • Button + ButtonText
  • Highlight + HighlightedText
  • ToolTipBase + ToolTipText (note that Background + Foreground is an obsolete way to express Window + WindowText)

    For KColorScheme, there are many more compatible combinations. Basically, for each "set" of colors (View, Window, Button, Selection, ToolTip, which map to the QPalette backgrounds above), there are many separate background roles and matching foreground/text roles.

Thanks I did not know those (implicit?) rules (do we have documentation ?).

So the statement palette.setBrush(QPalette::Highlight, colorScheme.foreground(KColorScheme::ActiveText)) is wrong, because you are trying to set a fg/text color as a background color. Same for palette.setBrush(QPalette::Background, colorScheme.foreground(KColorScheme::NegativeText)).

Sorry, it is still wrong. QPalette and KColorScheme have a fixed set of compatible "bg + fg" colors.

For QPalette:

  • Base/AlternateBase + Text
  • Window + WindowText
  • Button + ButtonText
  • Highlight + HighlightedText
  • ToolTipBase + ToolTipText (note that Background + Foreground is an obsolete way to express Window + WindowText)

    For KColorScheme, there are many more compatible combinations. Basically, for each "set" of colors (View, Window, Button, Selection, ToolTip, which map to the QPalette backgrounds above), there are many separate background roles and matching foreground/text roles.

    So the statement palette.setBrush(QPalette::Highlight, colorScheme.foreground(KColorScheme::ActiveText)) is wrong, because you are trying to set a fg/text color as a background color. Same for palette.setBrush(QPalette::Background, colorScheme.foreground(KColorScheme::NegativeText)).

What would be the correct statement then ?

If I understood you correctly I updated the code accordingly.

But the colors are off, for instance with breeze.

Negative Background:

Positive Background:

This was the reason I chose those values in the first place : I adapted the colors to those the :CapacityBar uses rather than semantics because this components don't use strictly the semantics.

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

I had to find those based on KCapacityBar (palette().base()) and Breeze (Style::drawProgressBarContentsControl palette.color( QPalette::Highlight )) the QPalette colors they use.

I don't care so much about semantics (as KCapacityBar and Breeze do) than if it works and would work for all themes.

cfeck added a comment.Feb 22 2020, 3:03 PM

I see KCapacityBar is created with DrawTextInline attribute. Is the text actually drawn by KCapacityBar, or is it a separate widget?

meven added a comment.Feb 22 2020, 4:18 PM

I see KCapacityBar is created with DrawTextInline attribute. Is the text actually drawn by KCapacityBar, or is it a separate widget?

KCapacityBar draws it indeed (as you can see in all of my screenshots), see KCapacityBar::drawCapacityBar
Breeze just position the text elsewhere.

But this has nothing to do with the issue here.

meven added a comment.Feb 28 2020, 8:59 AM

I beginning to think, it would be best to extend KWidgetsAddons::KCapacityBar to add it either a setAutoShowDangerousFill(bool) or a setState(CaparcityBarState) with CaparcityBarState {Normal, Negative/Dangerous/Highlighted} and let the theme implementation deal with it.
Does it make sense ?

cfeck added a comment.Feb 28 2020, 2:33 PM

If you add Positive to the CapacityState, then we could also use it in the NewPasswordDialog to colorize security 'progress' while you type. Skulpture style used to have a hack for that :)

meven abandoned this revision.Dec 4 2020, 9:46 PM