D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Peter J. Mello]

rscholar added a comment.


  In D27972#645517 , @pdabrowski 
wrote:
  
  > This widget could probably contain (at least) the information from the 
tooltip as mocked above or in D26688 . Or 
maybe even some settings (color temperature slidebar?).
  
  
  You beat me to it; adding a slidebar to manage the color temperate on-the-fly 
is the next logical evolution here (just like the Display Brightness Control 
widget, but hopefully with a much greater measure of aesthetic finesse).

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: rscholar, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski added a comment.


  In D27972#645441 , @ngraham wrote:
  
  > It almost sounds like we need a "full manual control" mode where it's 
expected that the system tray applet or keyboard shortcut will be used to turn 
it on and off on demand.
  
  
  Well, isn't this what current inhibition mode does anyway? Both in 
manual/always-on and in timed modes.
  
  And yes, I have a global shortcut set for this tray icon :) And it somewhat 
works.
  The only annoying issue is that the tray widget for "Night Color Control" 
gets shown whenever I press this shortcut.
  
  And btw "Night Color Control" widget is completely empty. And it is possible 
to run into this empty widget - either with the shortcut as mentioned above, or 
using the vertical bar with icons when the tray icon is hidden.
  This widget could probably contain (at least) the information from the 
tooltip as mocked above or in D26688 . Or 
maybe even some settings (color temperature slidebar?).

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Nathaniel Graham]

ngraham added a comment.


  It almost sounds like we need a "full manual control" mode where it's 
expected that the system tray applet or keyboard shortcut will be used to turn 
it on and off on demand.

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski added a comment.


  I totally missed the D26688  request.
  And it proposes some great ideas.
  So just to update my mockup accordingly:
  
  As for the tray icon visibility (for the states above):
  
  - Manual mode:
- disabled = F8230305: off22.png  
invisible ("Night Color is disabled. Click to enable.")
- active = F8230306: on22.png  
VISIBLE ("Night Color is active. Click to disable.")
  - Timed mode:
- disabled = F8230305: off22.png  
VISIBLE ("Night Color is disabled. Click to enable.")
- inactive = F8230305: off22.png  
invisible ("Night Color is inactive. Activation will begin at...")
- active = F8230306: on22.png  
VISIBLE ("Night Color is active. Colors will return to normal at...")
  
  And of course after setting the tray icon to "Always shown":
  
  - Manual mode:
- disabled = F8230305: off22.png  
VISIBLE ("Night Color is disabled. Click to enable.")
- active = F8230306: on22.png  
VISIBLE ("Night Color is active. Click to disable.")
  - Timed mode:
- disabled = F8230305: off22.png  
VISIBLE ("Night Color is disabled. Click to enable.")
- inactive = F8230305: off22.png  
VISIBLE ("Night Color is inactive. Activation will begin at...")
- active = F8230306: on22.png  
VISIBLE ("Night Color is active. Colors will return to normal at...")
  
  Plus "activating" and "deactivating" states.

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski added a comment.


  Maybe I should explain my motives and probably that would let us (including 
me) understand the goal better.
  
  I use Night Color only occasionally and I want to turn it on *manually*.
  And I like my tray clean.
  So I only need this icon when I have it manually engaged (i.e. un-inhibited).
  
  But I perfectly understand the need for Timed modes, which are a great 
feature.
  So they should provide a good user experience as well.
  
  **CURRENT STATE:**
  
  Currently there is no Manual mode available. You can select "Always on" in 
configuration, which will engage it every time you log in. This is not good.
  
  And also the "[ ] Activate Night Color" checkbox together with the 
possibility to inhibit the feature is confusing.
  There seem to be two separate ways of controlling this feature.
  
  F8230238: night-color-0.png 
  
  **PROPOSAL:**
  
  Maybe we could go with something like this:
  
  - Night Color system is always enabled (no "[ ] Activate Night Color" 
checkbox in the configuration)
  - by default set to Manual mode (see screenshot below)
  - Manual mode has a configuration option for setting the state at logon: "[ ] 
Active at logon" (unchecked by default)
  - the only way to disable the entire feature is what currently is named 
inhibition , which should be renamed to Disable
  
  F8230240: night-color-2.png 
  
  Also we could also polish the naming... current wording used all over this 
feature are very confusing:
  
  - "activate" in the configuration
  - "active", "inhibited", "not running", "disabled" in the tooltip
  - "On" and "Off" in the toast OSD message
  
  My suggestion (consistent with the proposal above):
  
  - "active", "inactive" (used in tooltip) = Night Color in effect or not
  - "disabled" (used both in the tooltip and OSD) = what currently is named 
inhibition
  
  **CONCLUSION:**
  
  So with the Manual mode you have 2 states toggled from the tray icon: 
"disabled" or "active".
  And in Timed modes you can have it "disabled" from the tray icon or 
"inactive" (daytime) and "active" (at night) otherwise.
  
  **TRAY ICON:**
  
  As for the tray icon visibility (for the states above):
  
  - Manual mode: disabled=invisible, active=visible
  - Timed mode: disabled=invisible, inactive=invisible, active=visible
  
  And of course after setting the tray icon to "Always shown":
  
  - Manual mode: disabled=visible, active=visible
  - Timed mode: disabled=visible, inactive=visible, active=visible

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Nathaniel Graham]

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


  In D27972#633289 , @zzag wrote:
  
  > > in "Always on" manual mode: when active (not inhibited)
  >
  > I still don't understand why we should hide the applet. If VDG is okay with 
such behavior, then feel free to push this change, I won't object or anything.
  
  
  Actually yeah, I agree with this. I didn't notice that change, sorry. If the 
user has deliberately turned it always on, then having the tray icon always 
active by default makes it into a quick on/off toggle, which I think is handy. 
Sorry for missing this with my earlier review.

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Vlad Zahorodnii]

zzag added a comment.


  > in "Always on" manual mode: when active (not inhibited)
  
  I still don't understand why we should hide the applet. If VDG is okay with 
such behavior, then feel free to push this change, I won't object or anything.

INLINE COMMENTS

> main.qml:26
>  
> +import org.kde.colorcorrect 0.1 as CC
> +

Please do not use `org.kde.colorcorrect` in this applet. If you need operation 
mode constants, please add them to the Monitor class. See D26688 
 for more details.

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  @zzag, is this okay for you?

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski updated this revision to Diff 78317.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27972?vs=78315=78317

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

AFFECTED FILES
  applets/nightcolor/package/contents/ui/main.qml
  applets/nightcolor/plugin/monitor.cpp
  applets/nightcolor/plugin/monitor.h
  applets/nightcolor/plugin/monitor_p.h

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski added a comment.


  In D27972#627281 , @ngraham wrote:
  
  > In D27972#626302 , @broulik 
wrote:
  >
  > >
  >
  >
  > Could you update the description to be a bit more clear about what exactly 
has been changed for which cases?
  
  
  Added a detailed comparison of the current and new behavior.

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski updated this revision to Diff 78315.
pdabrowski edited the summary of this revision.
pdabrowski edited the test plan for this revision.

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27972?vs=77473=78315

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

AFFECTED FILES
  applets/nightcolor/package/contents/ui/main.qml
  applets/nightcolor/plugin/monitor.cpp
  applets/nightcolor/plugin/monitor.h
  applets/nightcolor/plugin/monitor_p.h

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Nathaniel Graham]

ngraham added a comment.


  In D27972#626302 , @broulik wrote:
  
  > I think this needs a VDG/Usability discussion.
  >  Having the icon show when color correction is active (the status quo) imho 
makes sense.
  
  
  I agree.
  
  Could you update the description to be a bit more clear about what exactly 
has been changed for which cases?

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Vlad Zahorodnii]

zzag added a reviewer: VDG.
zzag added a comment.


  In D27972#626302 , @broulik wrote:
  
  > I think this needs a VDG/Usability discussion.
  
  
  Yes!

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham, #vdg
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Kai Uwe Broulik]

broulik added a comment.


  I think this needs a VDG/Usability discussion.
  Having the icon show when color correction is active (the status quo) imho 
makes sense.

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> pdabrowski wrote in main.qml:36-38
> > We want to keep the applet visible even if Night Color is inhibited so user 
> > can quickly uninhibit it.
> 
> You are right. I have modified the patch to show the icon when inhibited in 
> timed modes.

Why only in timed modes?

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski updated this revision to Diff 77473.

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27972?vs=77472=77473

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

AFFECTED FILES
  applets/nightcolor/package/contents/ui/main.qml
  applets/nightcolor/plugin/monitor.cpp
  applets/nightcolor/plugin/monitor.h
  applets/nightcolor/plugin/monitor_p.h

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

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


  In D27972#625596 , @zzag wrote:
  
  > > Fix for NightColor plasmoid active/passive status
  >
  > Could you please describe what's wrong with it right now?
  
  
  I wanted the icon to hide when Night Color is not affecting color correction 
(both in timed modes and manual "Always on" mode). Currently the icon is almost 
always visible.
  See updated description.

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski marked an inline comment as done.
pdabrowski added inline comments.

INLINE COMMENTS

> zzag wrote in main.qml:36-38
> We want to keep the applet visible even if Night Color is inhibited so user 
> can quickly uninhibit it. With this patch, the user would have to click the 
> system tray to show hidden icons and after that click the applet icon to 
> uninhibit Night Color.

> We want to keep the applet visible even if Night Color is inhibited so user 
> can quickly uninhibit it.

You are right. I have modified the patch to show the icon when inhibited in 
timed modes.

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski updated this revision to Diff 77472.
pdabrowski edited the summary of this revision.
pdabrowski added a comment.


  new patch

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27972?vs=77379=77472

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

AFFECTED FILES
  applets/nightcolor/package/contents/ui/main.qml
  applets/nightcolor/plugin/monitor.cpp
  applets/nightcolor/plugin/monitor.h
  applets/nightcolor/plugin/monitor_p.h

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> main.qml:36-38
> -if (inhibitor.state != Inhibitor.Uninhibited) {
> -return PlasmaCore.Types.ActiveStatus;
>  }

We want to keep the applet visible even if Night Color is inhibited so user can 
quickly uninhibit it. With this patch, the user would have to click the system 
tray to show hidden icons and after that click the applet icon to uninhibit 
Night Color.

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Vlad Zahorodnii]

zzag added a comment.


  > Fix for NightColor plasmoid active/passive status
  
  Could you please describe what's wrong with it right now?

REPOSITORY
  R114 Plasma Addons

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

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski updated this revision to Diff 77379.
pdabrowski added a comment.


  more context lines in diff

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27972?vs=77378=77379

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

AFFECTED FILES
  applets/nightcolor/package/contents/ui/main.qml

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27972: [NightColor] fix for plasmoid status and tooltip subtext

[Piotr Dabrowski]

pdabrowski created this revision.
pdabrowski added reviewers: Plasma, zzag, ngraham.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
pdabrowski requested review of this revision.

REVISION SUMMARY
  Fix for NightColor plasmoid active/passive status and its tooltip subtext.
  Especially noticeable when the tray icon is set to "Show when relevant".

REPOSITORY
  R114 Plasma Addons

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

AFFECTED FILES
  applets/nightcolor/package/contents/ui/main.qml

To: pdabrowski, #plasma, zzag, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart