[colorcorrection] Introduce toggle Night Color shortcut
ClosedPublic

Authored by zzag on Jul 5 2019, 10:35 AM.

Details

Summary

The new shortcut can be useful if a user wants to quickly disable the
Night Color manager for a brief moment.

FEATURE: 409083

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zzag created this revision.Jul 5 2019, 10:35 AM
Restricted Application added a project: KWin. · View Herald TranscriptJul 5 2019, 10:35 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jul 5 2019, 10:35 AM
davidedmundson added inline comments.
colorcorrection/manager.cpp
174

I don't think it needs the word "Manager" in the UI.

zzag updated this revision to Diff 61203.Jul 5 2019, 10:46 AM

Address David's inline comment.

zzag marked an inline comment as done.Jul 5 2019, 10:47 AM
broulik added inline comments.
colorcorrection/manager.cpp
172

Everywhere else in KWin (and other apps) we use a KActionCollection for this:

m_actionCollection = new KActionCollection(this, QStringLiteral("kwin"));
m_actionCollection->setComponentDisplayName(i18n("KWin"));

with m_actionCollection->addAction (which sets the objectName)

176

Given we don't actually set a shortcut, isn't

KGlobalAccel::self()->setShortcut(action, QList<QKeySequence>{});

the proper way?

zzag updated this revision to Diff 61208.Jul 5 2019, 11:34 AM

Address Kai's inline comment.

zzag marked an inline comment as done.Jul 5 2019, 11:35 AM
zzag added inline comments.
colorcorrection/manager.cpp
172

Everywhere else in KWin

That's inaccurate.

We don't use KActionCollection in KWin core because we don't need most of its features.

176

That's a copy-paste thing.

ngraham added a subscriber: ngraham.Jul 7 2019, 1:38 AM
romangg added a subscriber: romangg.Jul 9 2019, 1:46 PM

Having a shortcut for Night Color activation/toggle: +1

We need some kind of visual feedback when the shortcut gets triggered, like the volume indicator.

Wouldn't the visible feedback be that the screen's colors change? Or actually I guess that's not guaranteed because you might be toggling it on, but the current mode would make it activate later in the day. In that case, yeah, showing an OSD would be nice.

zzag marked an inline comment as done.Jul 9 2019, 2:01 PM

We can display an OSD on Wayland, but not on X11.

We can display an OSD on Wayland, but not on X11.

qdbus org.kde.plasmashell /org/kde/osdService org.kde.osdService.showText document-edit 'redshift wooo!!!!1'

zzag added a comment.EditedJul 9 2019, 2:14 PM

This would put plasmashell as a dependency of kwin. I'd rather go with using the internal osd service. Once we finish porting our QPA away from Wayland, the OSD service should become available on X11.

This would put plasmashell as a dependency of kwin

It's not a dependency, it's a dbus call that could be implemented by someone else. It is also already used in KWin for when toggling the virtual keyboard and keyboard layouts, so this is only consistent.

zzag updated this revision to Diff 61413.Jul 9 2019, 2:43 PM

Show an OSD when the active state is toggled.

zzag updated this revision to Diff 61415.Jul 9 2019, 2:45 PM

Use bool instead of int in showStatusOsd.

davidedmundson accepted this revision.Jul 9 2019, 2:47 PM
davidedmundson added inline comments.
colorcorrection/manager.cpp
176

It's not. There's a difference.

one calls

updateGlobalShortcut(KGlobalAccelPrivate::DefaultShortcut | KGlobalAccelPrivate::ActiveShortcut);

the other

updateGlobalShortcut(KGlobalAccelPrivate::ActiveShortcut);

but it doesn't really matter in this case

This revision is now accepted and ready to land.Jul 9 2019, 2:47 PM
romangg accepted this revision.Jul 9 2019, 2:48 PM
romangg added inline comments.
colorcorrection/manager.cpp
170

What would be nice is if it shows the current active mode. I.e. Night Color: Automatic. But can be added later.

This revision was automatically updated to reflect the committed changes.