When used space in computer exceeds 90% of total storage the color of progressbar(On status bar) changes to NegativeBackground of KColorScheme
Details
- Reviewers
ngraham pino elvisangelaccio sourabhboss - Group Reviewers
Dolphin
Breeze dark:
Fusion Check Color:
(Code was tweaked to be in red at 50%)
Diff Detail
- Repository
- R318 Dolphin
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 4158 Build 4176: arc lint + arc unit
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.
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
Screenshot - when used storage is above threshold
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.
Thanks. But this is still using a hardcoded color instead of the Negative color from the color scheme.
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 | ||
---|---|---|
111 | Why KColorScheme::Selection ? That should be used for items that can be selected. We should probably use KColorScheme::Window instead | |
114 | Why QPalette::Highlight ? That's supposed to be used for the selected text's color | |
115 | What's the second setBrush() needed for? |
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.
@elvisangelaccio i have used
KColorScheme::Window
instead of ::Selection then on NormalBackground color appears to be white
src/statusbar/statusbarspaceinfo.cpp | ||
---|---|---|
114 | Then what should i use QPalette::HighlightedText |
src/statusbar/statusbarspaceinfo.cpp | ||
---|---|---|
116 | Shouldn't this be QPalette::HightlightedText? |
src/statusbar/statusbarspaceinfo.cpp | ||
---|---|---|
116 | ... 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 | ||
---|---|---|
110 | Please use a descriptive variable name (e.g. palette) |
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.
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.
src/statusbar/statusbarspaceinfo.cpp | ||
---|---|---|
122 | Is this the correct way to do this ? |
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)).
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)).
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.
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.
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.
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 ?
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 :)