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?

Address Kai's inline comment.

Harbormaster completed remote builds in B13643: Diff 61208.Jul 5 2019, 11:34 AM

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

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.

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'

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.

Show an OSD when the active state is toggled.

Harbormaster completed remote builds in B13747: Diff 61413.Jul 9 2019, 2:43 PM

Use bool instead of int in showStatusOsd.

Harbormaster completed remote builds in B13748: Diff 61415.Jul 9 2019, 2:45 PM

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 ↗	(On Diff #61208)	What would be nice is if it shows the current active mode. I.e. Night Color: Automatic. But can be added later.

Closed by commit R108:b18351669af9: [colorcorrection] Introduce toggle Night Color shortcut (authored by zzag). · Explain WhyJul 9 2019, 3:43 PM

This revision was automatically updated to reflect the committed changes.

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	61202	d21f09d		Jul 5 2019, 10:35 AM	★	★
Diff 2	61203	d21f09d	Address David's inline comment.	Jul 5 2019, 10:46 AM	★	★
Diff 3	61208	d21f09d	Address Kai's inline comment.	Jul 5 2019, 11:34 AM	★	★
Diff 4	61413	d21f09d	Show an OSD when the active state is toggled.	Jul 9 2019, 2:43 PM	★	★
Diff 5	61415	d21f09d	Use bool instead of int in showStatusOsd.	Jul 9 2019, 2:45 PM	★	★
Diff 6	61423	6f7bea3	R108:b18351669af91fbb2cfbf2329f0e4bbf730f2b1c	Jul 9 2019, 3:43 PM	★	★

[colorcorrection] Introduce toggle Night Color shortcut
ClosedPublic
Actions

Details

Diff Detail

Showing Only Differences

Revision Contents
Changeset List

Diff 61413

colorcorrection/manager.cpp