Bug 428024 - Toggle Night Color global hotkey broken after upgrading to Plasma 5.20
Summary: Toggle Night Color global hotkey broken after upgrading to Plasma 5.20
Status: RESOLVED FIXED
Alias: None
Product: systemsettings
Classification: Applications
Component: kcm_keys (show other bugs)
Version: 5.20.0
Platform: Gentoo Packages Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Michael Jansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-20 14:50 UTC by Michal Ziabkowski
Modified: 2020-11-24 18:28 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.20.4


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Ziabkowski 2020-10-20 14:50:27 UTC
SUMMARY

After upgrading to Plasma 5.20, the Toggle Night Color global hotkey I had set-up stopped working. The colors don't change and there's no OSD notification. I tried setting the hotkey again, but it didn't change anything.

For what it's worth, toggling from the systray icon works. I get the expected action - notification and color change. It's just the keyboard shortcut action that seems to be broken.


STEPS TO REPRODUCE
1. Set Toggle Night Color global hotkey
2. Press it

OBSERVED RESULT
Nothing happens.


EXPECTED RESULT
Night color is toggled.


SOFTWARE/OS VERSIONS
Operating System: Gentoo Linux
KDE Plasma Version: 5.20.0
KDE Frameworks Version: 5.75.0
Qt Version: 5.15.1
Comment 1 Nate Graham 2020-10-21 02:42:58 UTC
Works for me. Is kglobalaccel5 running? Do other global shortcuts work, like Meta+D to show the desktop? How did yo set the shortcut? In the Shortcuts KCM, or on the applet itself?
Comment 2 Michal Ziabkowski 2020-10-21 07:19:49 UTC
Yes, kglobalaccel5 is running. Tested several other global shortcuts from different groups and they do work just as intended. It's just this one.

I set the shortcut in the shortcuts KCM, under the KWin heading. It's set to Meta+N. It didn't conflict with any other shortcut before, nor should it now. I don't see any other global shortcuts with this combination, nor did I get a conflict warning. And it is indeed present in kglobalshortcutsrc.

The issue appeared right after upgrading to Plasma 5.20, even without me changing a thing in my global shortcuts config. But, as previously stated, I set the global shortcut again just in case. It didn't help.

Tried changing the hotkey to something else and no change either. The changes are saved to the config file, other shortcuts work, just not this one.
Comment 3 Nate Graham 2020-10-21 20:51:41 UTC
That is very odd. I have the action bound to Meta+N myself and it works fine. :/
Comment 4 Nate Graham 2020-10-21 20:52:00 UTC
(dummy comment)
Comment 5 Michal Ziabkowski 2020-10-23 19:37:30 UTC
So here's an interesting thing I've discovered:

The global shortcut seems to work on a fresh user account, but here's the kicker: the line written in kglobalshortcutsrc is different.

On my old, non-working profile, the global shortcut line is:
Toggle Night Color=Meta+N,none,Toggle Night Color

On the new, fresh one it's localized in Polish:
Przełącz barwy nocne=Meta+N,none,Przełącz barwy nocne

I remember this shortcut not having a localized caption when I first set it. And apparently now it's localized in Plasma 5.20. However, the global shortcuts kcm still shows the old, English name and not the new localized one, which apparently the shortcut not to work altogether.

But something about this seemed wrong, so I took a closer look at the KWin source code. This stood out to me in colorcorrection/manager.cpp:

    toggleAction->setProperty("componentName", QStringLiteral(KWIN_NAME));
    toggleAction->setObjectName(i18n("Toggle Night Color"));
    toggleAction->setText(i18n("Toggle Night Color"));

I believe the issue stems from setObjectName using i18n instead of QStringLiteral, so the localized caption is used as the shortcut identifier.

This bug wasn't reproducible on your setup, since you're likely using English locale, avoiding the issue entirely.
Comment 6 Nate Graham 2020-10-24 21:10:43 UTC
Now that seems relevant.

The line of code in question was introduced in b18351669a on July 9 of last year, but it doesn't seem to have changed before or after that. I can't see that it was ever unlocalized.

Is there a chance that you set this shortcut while using the system in English?

---

Of course when if that's what happened, this seems to reveal a problem: your shortcuts shouldn't be tied to your locale and stop working if you switch to another language. Even if your locale never changes, comparing translated strings is fraught with problems because the translation could change.
Comment 7 Michal Ziabkowski 2020-10-24 22:15:54 UTC
No, I've been on Polish locale for years and definitely haven't changed anything. But I do distinctly recall the name for this shortcut being in English at one point, just after it was introduced. Sometimes the translations are spotty right after a new major release and it seems like this was the case here.

Changing i18n to QStringLiteral in setObjectName and rebuilding KWin indeed made the shortcut work again for me.

Regardless, the root of the problem is that identifiers shouldn't be dependent on current locale, yes. And a cursory glance at kglobalshortcutsrc reveals that all other identifiers are in English, as I'd expect. And looking at the code for other global shortcuts in KWin, this use of i18n for the object name seems like a simple mistake. Others use QStringLiteral there, so the identifier is always in English. So it's a bug with how global shortcuts are saved in general, just an oversight with this particular entry.
Comment 8 Nate Graham 2020-10-24 22:17:44 UTC
Agreed.

Would you like to submit a merge request to fix it?
Comment 9 Nate Graham 2020-10-24 22:19:50 UTC
Oh and now I think I can piece together what happened: you set the shortcut  after the feature was added, but before there was a Polish translation for this piece of text, so it serialized the key to the config file in English. Then later a Polish translation was added, so the key in your config file no longer matched the new value it was looking for.
Comment 10 Michal Ziabkowski 2020-10-24 22:25:47 UTC
Yes, that seems to be exactly it. Which in itself wouldn't pose a problem, but the identifier used i18n by mistake, so it changed once the translation was in place.

About the merge request, sure, but can I use Github? I haven't used the new invent.kde.org infrastructure. If I need to use the latter, this will take a bit longer.
Comment 11 Nate Graham 2020-10-24 22:38:31 UTC
You gotta use invent.kde.org. :)

The workflow is nearly identical to GitHub though (typical fork-the-repo-and-submit-a-pull-request), so hopefully it should be simple enough. And here's the documentation: https://community.kde.org/Infrastructure/GitLab

Thanks!
Comment 12 David Edmundson 2020-10-24 23:14:03 UTC
    toggleAction->setObjectName(i18n("Toggle Night Color"));


Eww. yeah. Well spotted.
Though one thing to be wary of, fixing this is going to break it for every (non English) user.

It'll be a new shortcut that now clashes with an existing one.
Comment 13 David Redondo 2020-10-25 12:52:56 UTC
(In reply to David Edmundson from comment #12)
>     toggleAction->setObjectName(i18n("Toggle Night Color"));
> 
> 
> Eww. yeah. Well spotted.
> Though one thing to be wary of, fixing this is going to break it for every
> (non English) user.
> 
> It'll be a new shortcut that now clashes with an existing one.

As a way forward we could set two shortcuts. First the non localized one and then the old localized one.  This way new users get the "correct" shortcut and the old one doesn't break. Of course there would be now a duplciated entry in the config.
Comment 14 Nate Graham 2020-10-25 15:05:29 UTC
Or maybe... two keys for one shortcut?
Comment 15 Bug Janitor Service 2020-10-25 15:12:18 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/kwin/-/merge_requests/381
Comment 16 Bug Janitor Service 2020-11-23 13:17:42 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/kwin/-/merge_requests/476
Comment 17 David Edmundson 2020-11-24 17:49:31 UTC
Git commit b186f867861f3613b2c1d3a077f4cf1048aad8e7 by David Edmundson, on behalf of Michał Ziąbkowski.
Committed on 24/11/2020 at 17:49.
Pushed by davidedmundson into branch 'master'.

Fixed Toggle Night Color global shortcut, which used i18n in object name, leading to erratic behavior e.g. when system locale or translations changed.

M  +9    -1    colorcorrection/manager.cpp

https://invent.kde.org/plasma/kwin/commit/b186f867861f3613b2c1d3a077f4cf1048aad8e7
Comment 18 Nate Graham 2020-11-24 18:27:35 UTC
Git commit 6acab647182158d2ebbfc313b6d030fbfaa1acc9 by Nate Graham, on behalf of Michał Ziąbkowski.
Committed on 24/11/2020 at 18:27.
Pushed by ngraham into branch 'Plasma/5.20'.

Fixed Toggle Night Color global shortcut, which used i18n in object name, leading to erratic behavior e.g. when system locale or translations changed.


(cherry picked from commit b186f867861f3613b2c1d3a077f4cf1048aad8e7)

M  +9    -1    colorcorrection/manager.cpp

https://invent.kde.org/plasma/kwin/commit/6acab647182158d2ebbfc313b6d030fbfaa1acc9