Review Request 122981: add KGlobalAccel::loadShortcutFromGlobalSettings
Thomas Lübking
thomas.luebking at gmail.com
Sat Apr 4 20:38:17 UTC 2015
> On März 22, 2015, 3:56 nachm., Albert Astals Cid wrote:
> > src/kglobalaccel.h, line 260
> > <https://git.reviewboard.kde.org/r/122981/diff/2/?file=356138#file356138line260>
> >
> > typo in following
>
> Gregor Mi wrote:
> About why it is needed: see KGlobalShortcutTest::testLoadShortcutFromGlobalSettings: shortcut() does not behave as expected when the new method loadShortcutFromGlobalSettings() is not called
>
> I don't know if always calling loadShortcutFromGlobalSettings() before shortcut() would break existing code. E.g. in kwin/effects/desktopgrid/desktopgrid_config.cpp shortcut() is used without prior call of loadShortcutFromGlobalSettings().
>
> On the other hand, a call to loadShortcutFromGlobalSettings() might be harmless but I don't have a good overview of the KGlobalAccel usage patterns.
>
> Albert Astals Cid wrote:
> Honestly i have no idea on how this kglobalaccel thing works, but it seems to me you're just adding a way to workaround a bug instead of fixing it.
>
> Gregor Mi wrote:
> Me neither :-)
>
> I was hoping that someone with more experience with kglobalaccel can say more about the proposed solution.
>
> Like you Thomas suggested to put the loading method into shortcut() (which is essentially a one-liner: d->updateGlobalShortcut(action, KGlobalAccelPrivate::ActiveShortcut, KGlobalAccel::Autoloading);).
>
> Martin Gräßlin wrote:
> well we could try it: add the one-line change and try whether the unit tests pass and then try whether the case in DesktopGridConfig works.
>
> Unfortunately I'm also not that into KGloalAccel code yet to be 100 % whether it's a valid change :-(
>
> Gregor Mi wrote:
> I tried to call loadShortcutFromGlobalSettings() from within shortcut() and found that this violates const constraints:
>
> shortcut() is const
> d->updateGlobalShortcut is not const
>
> So this is not possible.
>
> How to proceed now?
>
> Albert Astals Cid wrote:
> make mutable the members that updateGlobalShortcut modifies
>
> Gregor Mi wrote:
> I tried again and the call was even possible with only introducing some more consts. But the call of loadShortcutFromGlobalSettings() from within shortcut() makes more unit tests fail than without.
>
> => the general call of loadShortcutFromGlobalSettings() from within shortcut() seems to harm more than it helps.
>
> Currently, I have a problem with getting the unit tests to all pass (which is independet of this change; it happens also in master branch). See my analysis below and comment of the unit test itself:
>
> /* These tests could be better. They don't include actually triggering actions,
> and we just choose very improbable shortcuts to avoid conflicts with real
> applications' shortcuts. */
>
> It would be nice to document some instructions of how to reset the global shortcuts to a defined state.
>
> Albert Astals Cid wrote:
> Use strace to see which files are being read.
>
> Gregor Mi wrote:
> I did
> ```
> strace -o strace1.txt ./kglobalshortcuttest testFindActionByKey
> sed -n '/open/p' strace1.txt > strace1.open.txt
> ```
>
> This is the complete output http://paste.opensuse.org/97828 and filtered by everything that contains "open": http://paste.opensuse.org/54497725.
>
> List all filenames:
> `sed -n 's/open("(.*)".*/\1/p' strace1.open.txt > strace1.files.txt`
>
> Remove duplicates:
> `sort strace1.files.txt | uniq -u > strace1.files.nodups.txt`
>
> Manually remove /dev/urandom from that list.
>
> Grep for testLoadShortcutFromGlobalSettings in the list.
> ```
> for f in `cat strace1.files.nodups.txt`; do grep testLoadShortcutFromGlobalSettings $f; done
> ```
>
> No result.
>
> The shortcuts seem to be retrieved via DBUS. Is this covered by strace as well?
>
> Albert Astals Cid wrote:
> No, strace doesn't cover dbus.
But "dbus-monitor" does ;-)
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122981/#review77912
-----------------------------------------------------------
On April 3, 2015, 5:50 nachm., Gregor Mi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122981/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 5:50 nachm.)
>
>
> Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
>
>
> Repository: kglobalaccel
>
>
> Description
> -------
>
> In some cases you need to call loadShortcutFromGlobalSettings() in order not to get a an empty list when calling shortcut() (which is const).
>
> See discussion in https://git.reviewboard.kde.org/r/122249/ ("libksysguard: add Kill Window to End Process button and show correct keyboard shortcut").
>
>
> Diffs
> -----
>
> autotests/kglobalshortcuttest.h b1122a8f5ca2f3f7afbe78f8edba87325426c1a6
> autotests/kglobalshortcuttest.cpp 3b661bbb612807a3bbbe34835d4ae712c2ec74da
> src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16
> src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1
> src/kglobalaccel_p.h eca7c52378ad60d0d5806561214b9788dd46a11e
>
> Diff: https://git.reviewboard.kde.org/r/122981/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gregor Mi
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150404/a527fa2e/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list