Add applet for controlling Night Color
ClosedPublic

Authored by zzag on Dec 13 2019, 1:23 PM.

Details

Summary

The new applet allows to temporarily disable Night Color, for example if
one wants to edit a video or an image.

BUG: 400418

Diff Detail

Repository
R114 Plasma Addons
Branch
nightcolor-applet
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19815
Build 19833: arc lint + arc unit
zzag created this revision.Dec 13 2019, 1:23 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 13 2019, 1:23 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
zzag requested review of this revision.Dec 13 2019, 1:23 PM
zzag added a comment.Dec 13 2019, 1:25 PM

This patch depends on D25946 and D25786.

zzag updated this revision to Diff 71438.Dec 13 2019, 1:48 PM

Delete stray include.

This is fantastic! My on UI suggestion is to only the applet in the system tray (when using "Auto" visibility) only when Night Color is actually activated or activating. People who want it in their tray all the time can manually set it to "shown". The one exception would be that when Night Color is is using the "Constant" mode, the applet should always be visible in the tray when using "Auto" visibility.

zzag updated this revision to Diff 71474.Dec 13 2019, 8:31 PM

Nate's suggestion.

ngraham accepted this revision as: VDG.Dec 13 2019, 8:34 PM
ngraham added a reviewer: VDG.

UI and interactivity seems spot-on to me now.

zzag updated this revision to Diff 71477.Dec 13 2019, 9:10 PM

Display the applet in the system tray when Night Color is inhibited.

Hmm, that last change broke clicking on the applet. Now it does nothing. +1 for the general ideal behind it though.

zzag updated this revision to Diff 71479.Dec 13 2019, 9:49 PM

Revert applet visibility changes.

Unfortunately, this may cause unnecessary shuffling of system tray items.

Thanks, that fixed it. I haven't noticed any shifting of the plasmoid in the system tray, but if you want to minimize that, you can add X-Plasma-NotificationAreaCategory=SystemServices to the metadata.desktop file. See D11352 for more details.

zzag updated this revision to Diff 71537.Dec 14 2019, 5:42 PM

Nate's suggestion.

ngraham accepted this revision.Dec 14 2019, 5:48 PM

UI and functionality seem spot-on to me!

This revision is now accepted and ready to land.Dec 14 2019, 5:48 PM
davidedmundson accepted this revision.Jan 7 2020, 1:52 PM
davidedmundson added a subscriber: davidedmundson.

Does it make sense to dump the C++ classes in libcolorcorrect?

It has an import for use client side usage for the KCM, his is just another client, especially inhibitor.

applets/nightcolor/package/contents/ui/main.qml
97

I don't think we have an official QML coding guideline, but convention throughout plasma has been to put {} for single line if statements.

99

Should

plasmoid.removeAction("configure");

be in an else?

zzag added a comment.EditedJan 7 2020, 3:25 PM

Does it make sense to dump the C++ classes in libcolorcorrect?

I think it will be better to keep them here for now. Once somebody else needs one of them, we can move them to libcolorcorrect (bad name).

applets/nightcolor/package/contents/ui/main.qml
99

Could you please explain why it needs to be in an else?

zzag updated this revision to Diff 72987.Jan 7 2020, 3:26 PM

Add braces.

davidedmundson added inline comments.Jan 7 2020, 3:29 PM
applets/nightcolor/package/contents/ui/main.qml
99

I assumed it was trying to remove the action created here:
plasmoid.setAction(..."configure");

I see now that "configure" on the setAction is the icon name and this is removing a different action

This revision was automatically updated to reflect the committed changes.

There is a small typo.
You need to replace the quotation marks.
Attached a patch.

There is a small typo.
You need to replace the quotation marks.
Attached a patch.

Fixed.
https://commits.kde.org/kdeplasma-addons/e76ed74f056bdbbe69c2242b75aa77624f7e471b