D16353: Change color(red) of status bar in space info when storage exceeds 80%

[Sourabh Sharma]

sourabhboss created this revision.
sourabhboss added a reviewer: ngraham.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.
sourabhboss requested review of this revision.

REPOSITORY
R318 Dolphin

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D16353

AFFECTED FILES
src/statusbar/statusbarspaceinfo.cpp

To: sourabhboss, ngraham
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss added a comment.


My First Contribution

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss edited the summary of this revision.
sourabhboss added reviewers: pino, elvisangelaccio.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


Please add before and after screenshot.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: shubham, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: shubham, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss updated this revision to Diff 44029.
sourabhboss added a comment.


Threshold set to 90%

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16353?vs=44020&id=44029

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D16353

AFFECTED FILES
src/statusbar/statusbarspaceinfo.cpp

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Pino Toscano]

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss added a comment.


F6341750: Screenshot-20181021-212621.png <https://phabricator.kde.org/F6341750> Screenshot - when used storage is below threshold
F6341766: 8r2G1PU.png <https://phabricator.kde.org/F6341766> Screenshot - when used storage is above threshold

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss added a comment.

Post by Pino Toscano
- 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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a reviewer: Dolphin.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Christoph Feck]

cfeck added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

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%".
sourabhboss edited the summary of this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss updated this revision to Diff 44069.
sourabhboss added a comment.


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

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16353?vs=44029&id=44069

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D16353

AFFECTED FILES
src/statusbar/statusbarspaceinfo.cpp

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss added a comment.

Post by Nathaniel Graham
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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss updated this revision to Diff 44110.
sourabhboss added a comment.


Instead of hardcoded colors, ColorScheme is used

Thank You @ngraham and @cfeck

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16353?vs=44069&id=44110

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D16353

AFFECTED FILES
src/statusbar/statusbarspaceinfo.cpp

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

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%".
sourabhboss edited the summary of this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Christoph Feck]

cfeck added a comment.


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

INLINE COMMENTS

statusbarspaceinfo.cpp:28
#include <knewstuff_version.h>
+#include <kcolorscheme.h>

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

statusbarspaceinfo.cpp:117
+ }
+ else {
+ p.setBrush(QPalette::Highlight, colorScheme.background(KColorScheme::NormalBackground));

`} else {`

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss updated this revision to Diff 44148.
sourabhboss marked 2 inline comments as done.
sourabhboss added a comment.


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

} else { //done!!

import also correected

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16353?vs=44110&id=44148

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D16353

AFFECTED FILES
src/statusbar/statusbarspaceinfo.cpp

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

statusbarspaceinfo.cpp:111
+ QPalette p = palette();
+ KColorScheme colorScheme(QPalette::Active, KColorScheme::Selection);
+

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

We should probably use `KColorScheme::Window` instead

statusbarspaceinfo.cpp:114
+ if (percentUsed >= 90) {
+ p.setBrush(QPalette::Highlight, colorScheme.background(KColorScheme::NegativeBackground));
+ p.setBrush(QPalette::HighlightedText, colorScheme.foreground(KColorScheme::NegativeText));

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

statusbarspaceinfo.cpp:115
+ p.setBrush(QPalette::Highlight, colorScheme.background(KColorScheme::NegativeBackground));
+ p.setBrush(QPalette::HighlightedText, colorScheme.foreground(KColorScheme::NegativeText));
+ } else {

What's the second `setBrush()` needed for?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Christoph Feck]

cfeck added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sourabh Sharma]

sourabhboss marked an inline comment as done.
sourabhboss added a comment.


@elvisangelaccio i have used

KColorScheme::Window

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

INLINE COMMENTS

elvisangelaccio wrote in statusbarspaceinfo.cpp:114
Why `QPalette::Highlight` ? That's supposed to be used for the selected text's color

Then what should i use
For text i have used

QPalette::HighlightedText

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16353

To: sourabhboss, ngraham, pino, elvisangelaccio, #dolphin
Cc: cfeck, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp