Bug 343891 - khotkeysrc destroyed upon changing a shortcut setting
Summary: khotkeysrc destroyed upon changing a shortcut setting
Status: RESOLVED FIXED
Alias: None
Product: khotkeys
Classification: Plasma
Component: general (show other bugs)
Version: 5.2.0
Platform: Arch Linux Linux
: NOR normal
Target Milestone: ---
Assignee: Michael Jansen
URL:
Keywords:
: 344383 345252 347031 352351 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-02-07 13:59 UTC by thomas gahr
Modified: 2016-07-09 06:31 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.5


Attachments
khotkeysrc before changing shortcut settings (29.56 KB, text/plain)
2015-02-07 14:01 UTC, thomas gahr
Details
khotkeysrc AFTER changing shortcut settings (774 bytes, text/plain)
2015-02-07 14:01 UTC, thomas gahr
Details

Note You need to log in before you can comment on or make changes to this bug.
Description thomas gahr 2015-02-07 13:59:32 UTC
Switched to Plasma 5.2 yesterday, renamed old .kde4 folder to start with a clean slate.
Tried to set global shortcuts, but the settings didn't take.
Logged out, renamed .config/khotkeysrc and logged back in.
.config/khotkeysrc created automatically
Added a shortcut:
- added new group "Mine" in Custom shortcuts
- added new shortcut in said group to launch konsole when pressing "meta+c"
- clicked "apply"
result: .config/khotkeysrc changed from 30K to 774 bytes in size
restarted SystemSettings to see that setting didn't take. Moreover: the only group in "Custom shortcuts" is now "KMenuEdit" while there were more before.

Reproducible: Always

Steps to Reproduce:
1. rename .config/khotkeysrc
2. log in & change global shortcuts settings, add custom shortcut
3. click apply

Actual Results:  
.config/khotkeysrc is screwed, custom shortcut does not work

Expected Results:  
.config/khotkeysrc to only grow in size, not shrink from 30K to a few bytes, custom shortcut should work

List of keyboard layouts configured:
- de (eliminate dead keys)
- us
Comment 1 thomas gahr 2015-02-07 14:01:08 UTC
Created attachment 90962 [details]
khotkeysrc before changing shortcut settings

this is the khotkeysrc that is created automatically after removing it first and then logging in
Comment 2 thomas gahr 2015-02-07 14:01:53 UTC
Created attachment 90963 [details]
khotkeysrc AFTER changing shortcut settings

khotkeysrc after trying to add a custom shortcut in SystemSettings and clicking "apply"
Comment 3 David Edmundson 2015-02-07 14:29:09 UTC
I reproduced this.. but only after a few attempts.

If you could find out what the pattern is that makes this happen that would really help here.
Comment 4 thomas gahr 2015-02-08 05:46:23 UTC
OK so I dug a little deeper... scroll down for [tl:dr] 

I put my .config dir into a git repo and added commits like this:

for file in $( ls ) ; git rm -r $file && sh -c "git commit -am \"$file\"" done 

this way I git-bisected to the commit that removed kglobalshortcutsrc (yay binary search!)

Once I removed BOTH kglobalshortcutsrc and khotkeysrc it worked.
Deleting khotkeysrc from the working setup does not break anything.
Moving the OLD kglobalshortcutsrc in place breaks it.
If I keep khotkeysrc in place now and move the new (saved from the working setup) kglobalshortcutsrc in place does NOT fix it - so the broken kglobalshortcutsrc breaks khotkeysrc.
Starting from this I narrowed it down to the following line in kglobalshortcutsrc:
{d03619b6-9b3c-48cc-9d9c-a2aadb485550}=none,none,Search
which is found in the section [khotkeys]
the whole diff between working and broken kglobalshortcutsrc is:

--- kglobalshortcutsrc.yes      2015-02-08 06:00:11.723950888 +0100
+++ kglobalshortcutsrc.nope     2015-02-08 05:51:26.913967273 +0100
@@ -20,9 +20,7 @@
 
 [khotkeys]
 _k_friendly_name=khotkeys
-{2069c78b-ac0f-43a6-95a4-7f9c57fadadf}=Print,none,PrintScreen
-{d03619b6-9b3c-48cc-9d9c-a2aadb485550}=,none,Search
-{f643b1ce-97fd-4a9d-8748-adbc6bd74b4b}=Meta+D,none,asddf
+{d03619b6-9b3c-48cc-9d9c-a2aadb485550}=none,none,Search
 
 [kmix]
 _k_friendly_name=KMix
@@ -208,4 +206,4 @@
 
 [yakuake]
 _k_friendly_name=Yakuake
-toggle-window-state=F12,F12,Open/Retract Yakuake
+toggle-window-state=Ctrl+1,F12,Open/Retract Yakuake


removing the faulty line fixes the problem.

[tl:dr] 
In fact changing the line from
{d03619b6-9b3c-48cc-9d9c-a2aadb485550}=none,none,Search
to
{d03619b6-9b3c-48cc-9d9c-a2aadb485550}=,none,Search
makes the problem go away.

Do you need anything else to reproduce this? I guess it'd do no harm to upload both files here?
Comment 5 thomas gahr 2015-02-09 08:43:13 UTC
Just tried it the other way around... made scratch user account, logged in, logged out, located the line mentioned above and added the "none". Add new custom shortcut - boom.
The question is: how did the "none" end up in my kglobalshortcutsrc? Did the old systemsettings do that and this is a backwards-compatibility issue? I most certainly did not add it myself ;)
Comment 6 Thomas Lübking 2015-10-10 21:49:01 UTC
*** Bug 345252 has been marked as a duplicate of this bug. ***
Comment 7 Thomas Lübking 2015-10-10 21:52:52 UTC
*** Bug 347031 has been marked as a duplicate of this bug. ***
Comment 8 Thomas Lübking 2015-10-10 21:56:36 UTC
*** Bug 352351 has been marked as a duplicate of this bug. ***
Comment 9 Thomas Lübking 2015-10-10 21:56:50 UTC
*** Bug 344383 has been marked as a duplicate of this bug. ***
Comment 10 Thomas Lübking 2015-10-11 20:38:41 UTC
It seems the kded module is the culprit.
When reading shortcuts, it causes a shortcut change, which in turn makes it write back the (partial) list.

This might be a bug/change in kglobalaccel (undelayed shortcut update?) but this patch shedules the save into the next event queue (so it's not performed *while* reading stuff)


After *reliably* nuking my config every single time I tried, it's now gone.
Anybody can give it a try?

@David, since Michael doesn't seem to maintain khotkeys, do you now?
Want a review request?

-------------------

diff --git a/app/kded.cpp b/app/kded.cpp
index 4e20ad4..307d6d4 100644
--- a/app/kded.cpp
+++ b/app/kded.cpp
@@ -39,6 +39,7 @@ using namespace KHotKeys;
 KHotKeysModule::KHotKeysModule(QObject* parent, const QList<QVariant>&)
     : KDEDModule(parent)
     , actions_root(NULL)
+    , _settingsDirty(false)
     , _settings()
     ,_initialized(false)
     {
@@ -64,7 +65,7 @@ void KHotKeysModule::initialize()
     // If a shortcut was changed (global shortcuts kcm), save
     connect(
             keyboard_handler.data(), SIGNAL(shortcutChanged()),
-            this, SLOT(save()));
+            this, SLOT(scheduleSave()));
 
     // Read the configuration from file khotkeysrc
     reread_configuration();
@@ -239,9 +240,18 @@ void KHotKeysModule::quit()
     deleteLater();
     }
 
+void KHotKeysModule::scheduleSave()
+    {
+    if (!_settingsDirty)
+        {
+        _settingsDirty = true;
+        QMetaObject::invokeMethod(this, "save", Qt::QueuedConnection);
+        }
+    }
 
 void KHotKeysModule::save()
     {
+    _settingsDirty = false;
     KHotKeys::khotkeys_set_active( false );
     _settings.write();
     KHotKeys::khotkeys_set_active( true );
diff --git a/app/kded.h b/app/kded.h
index 623b1e8..68bc6a3 100644
--- a/app/kded.h
+++ b/app/kded.h
@@ -62,6 +62,7 @@ class KHotKeysModule
     private Q_SLOTS:
 
         //! Save
+        void scheduleSave();
         void save();
 
         //! Initialize the module. Delayed initialization.
@@ -79,6 +80,7 @@ class KHotKeysModule
 
         //! The current settings
         KHotKeys::Settings _settings;
+        bool _settingsDirty;
 
         //! Is the module initialized
         bool _initialized;
Comment 11 Thomas Lübking 2015-10-11 20:39:05 UTC
Maybe I should also watch the bug ;-)
Comment 12 Thomas Lübking 2015-10-12 13:54:04 UTC
https://git.reviewboard.kde.org/r/125607/
Comment 13 Thomas Lübking 2015-10-12 14:02:57 UTC
Git commit 4747599badf67389530483ea62b6f54bc36ac9c3 by Thomas Lübking.
Committed on 12/10/2015 at 13:47.
Pushed by luebking into branch 'master'.

schedule saving to the next event cycle

notably since writing reading and applying
settings can cause a shortcut update which
itself triggers a save (of the partial-only
database)

But also since several changes can occur in
a row, so we don't write for everyone
FIXED-IN: 5.5

M  +11   -1    app/kded.cpp
M  +2    -0    app/kded.h

http://commits.kde.org/khotkeys/4747599badf67389530483ea62b6f54bc36ac9c3
Comment 14 Pulfer 2016-07-09 04:12:39 UTC
BTW, shouldn't khotkeys open config as KConfig::SimpleConfig? To avoid adding lines like:

[Icons]
Theme[$d]
Comment 15 Thomas Lübking 2016-07-09 06:31:10 UTC
Those wouldn't be avoided (local $d simply trumps cascading and moves to the application default) and using simple config would kill kiosk.