KCM for controlling Night Color
ClosedPublic

Authored by romangg on May 22 2017, 1:21 AM.

Details

Summary

Simple but powerful KCM utilizing LibColorCorrect from Plasma Workspace to provide configuration of Night Color in KWin.

Video demonstration: https://www.youtube.com/watch?v=s5ltZFhEIVE

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg created this revision.May 22 2017, 1:21 AM
Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptMay 22 2017, 1:21 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

looks mostly good.

Make sure you test with --reverse, I think your activator checkbox will be broken

kcms/nightcolor/kcm_nightcolor.desktop
18

FYI, adding translations here is a waste of time

Scripty will ignore this when it extracts the text; then they'll be replaced when it puts the translations back

kcms/nightcolor/package/contents/ui/LocationsAutoView.qml
22

you shouldn't be using Plasma Components in KCMs. Use QQC directly.

Plasma Components follow the plasma theme which can lead to a white text on white situation.

kcms/nightcolor/package/contents/ui/main.qml
32

We have an actual implicitWidth from the layout itself, we don't need to guess

342–343

just fill the parent. It's already set to main.width*0.8

mart added a subscriber: mart.May 22 2017, 9:00 AM

small comment of the layout: align the labels (sunset begins etc) to the right instead of the left

romangg updated this revision to Diff 14753.May 22 2017, 9:25 AM
romangg marked 4 inline comments as done.

Diff update:

  • Fixed inline comments
  • Fixed @mart's comment
  • Labels aligned to the right
  • Slight renaming of labels ("Sunrise/set ends" -> "and ends")

Make sure you test with --reverse, I think your activator checkbox will be broken

You're right, but I'm just using the text attribute of QQC's CheckBox, so I'm not sure if I should write a workaround here or just wait for upstream to fix it. The visual fallout is minimal and the checkbox still usable:

kcms/nightcolor/kcm_nightcolor.desktop
18

Ok, will omit it next time. Thanks!

mart added a comment.May 22 2017, 9:29 AM

as far i seen, most qqc1 checkboxes around seem to have this problem

Make sure you test with --reverse, I think your activator checkbox will be broken

You're right, but I'm just using the text attribute of QQC's CheckBox, so I'm not sure if I should write a workaround here or just wait for upstream to fix it. The visual fallout is minimal and the checkbox still usable:

Heh, didn't expect that.

What I meant was that X would still be 0 rather than the item being right aligned.
With RTL anchors and layouts get flipped. Not specifying anything (so an implicit x==0) it doens't get flipped.

Though given the bug you showed maybe it's all a bit moot.

lueck added a subscriber: lueck.May 22 2017, 7:22 PM
lueck added inline comments.
kcms/nightcolor/CMakeLists.txt
3

The kcm loads a translation catalog, but where is the Messages.sh extracting the strings into the catalog "kcm_nightcolor"?

romangg added inline comments.May 22 2017, 7:39 PM
kcms/nightcolor/CMakeLists.txt
3

Ah, that's what these mysterious Messages.sh files everywhere are for. ;)

Is the Messages.sh right this way?

#! /usr/bin/env bash
$XGETTEXT `find . -name "*.qml"` -o $podir/kcm_nightcolor.pot
lueck added inline comments.May 24 2017, 7:23 PM
kcms/nightcolor/CMakeLists.txt
3

You need to add "*.cpp", see my comment in kcm.cpp

kcms/nightcolor/kcm.cpp
33
35

Using the button Help does not make sense without a docbook and whithout an entry X-DocPath in the desktop file

kcms/nightcolor/package/contents/ui/LocationsAutoView.qml
29

in CMakelists.txt you set the domain already to "kcm_nightcolor", therefore the i18nd call should be replaced by i18n() calls
that is less error prone if a catalogs has to be renamed.

Zren added a subscriber: Zren.May 25 2017, 12:12 PM

Is there a "daytime" setting or is it hardcoded to be 100% blue? Redshift Control for example:

romangg updated this revision to Diff 14917.EditedMay 28 2017, 7:06 PM
romangg marked 6 inline comments as done.

Burkhard's remarks:

  • Messages.sh
  • Translations in cpp file
  • i18n instead of i18nd in qml
  • Deactivate Help button
In D5932#111809, @Zren wrote:

Is there a "daytime" setting or is it hardcoded to be 100% blue? Redshift Control for example:

Correcting color values in general is part of the later to introduce Color Correction / Color Balance configuration, which is then done per screen. The Night Color value will then be only ever more a factor multiplied to it (the slider scale then probably has to change from an absolute temperature one to a relative one).

davidedmundson accepted this revision.Aug 19 2017, 3:00 PM
This revision is now accepted and ready to land.Aug 19 2017, 3:00 PM
romangg updated this revision to Diff 20969.Oct 19 2017, 11:04 AM

Merge current master.

romangg updated this revision to Diff 23701.EditedDec 9 2017, 9:27 PM
  • Small fixes,
  • merge current master.
This revision was automatically updated to reflect the committed changes.
dfaure added a subscriber: dfaure.Feb 3 2018, 10:46 AM
dfaure added inline comments.
CMakeLists.txt
59

Why is it required? Can't this KCM be skipped if the lib isn't available?

Also, this is missing a set_package_properties call so that one can find out what the lib is about and where to get it from, when hitting the raw cmake error about LibColorCorrect not being available.

Could not find a package configuration file provided by "LibColorCorrect" [...]

kcms/nightcolor/kcm_nightcolor.desktop
18

Typo: temperatur -> temperature.

graesslin added inline comments.
CMakeLists.txt
59

Why is it required? Can't this KCM be skipped if the lib isn't available?

This is just another library of Plasma. It's quite normal that we hard depend on other libraries of the same Plasma release.

dfaure added a comment.Feb 3 2018, 3:43 PM

Ah, the un-namespaced name made me think it was an external dependency.
OK, sorry for the noise, I must have updated plasma-desktop without updating plasma-workspace, it works now after a run of kdesrc-build.

ngraham added a subscriber: ngraham.Feb 3 2018, 5:09 PM