Review Request 122419: [screenlocker] Support global shortcuts in the lock screen

[Martin Gräßlin]

> On Feb. 4, 2015, 11:28 a.m., David Edmundson wrote:
> > ksmserver/screenlocker/globalaccel.cpp, line 273
> > <https://git.reviewboard.kde.org/r/122419/diff/1/?file=346752#file346752line273>
> >
> >     you're not handling emacs style shortcuts here.

AFAIK emacs style shortcuts are not supported as global shortcuts. If I am wrong in that, please show me an example, that I can look at it.


> On Feb. 4, 2015, 11:28 a.m., David Edmundson wrote:
> > ksmserver/screenlocker/globalaccel.cpp, line 66
> > <https://git.reviewboard.kde.org/r/122419/diff/1/?file=346752#file346752line66>
> >
> >     why bother doing this and the check in init? It's not like kglobal accel is going to reset whilst the locker is active, and even if it does close we don't need to react differently.
> >     
> >     It's DBus activated so the right thing to do (TM) is to call ListNames regardless, and to call Invoke regardless. The DBus server will start up kglobalaccel5 if it's not running; and if it can't run you get an error which you handle anyway.
> >     
> >     you can just remove m_connected, and the entirity of init().
> >     
> >     
> >     Also as-is you have a race if lock is called before init's ListName finishes which would make shortcuts not work in that instance

I do not think that the lock screen should start kglobalaccel (should not change state of session). That's why I only want to interact with it if it's available and why I did the checks. I think they are useful.

> Also as-is you have a race if lock is called before init's ListName finishes which would make shortcuts not work in that instance

doesn't matter, could only happen during session startup - it's ksmserver after all.


> On Feb. 4, 2015, 11:28 a.m., David Edmundson wrote:
> > ksmserver/screenlocker/globalaccel.cpp, line 113
> > <https://git.reviewboard.kde.org/r/122419/diff/1/?file=346752#file346752line113>
> >
> >     if we're treating this as a bool, why not just make it a bool?
> >     
> >     The only person that increments this is this method, and that will only do it when it's m_updatingInformation is 0.

there are multiple places increasing m_updatingInformation.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122419/#review75369
-----------------------------------------------------------


On Feb. 4, 2015, 10:02 a.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122419/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2015, 10:02 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 198097
>     https://bugs.kde.org/show_bug.cgi?id=198097
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> This change implements support for white listed global shortcuts in
> the lock screen. It interacts with KGlobalAccel to fetch shortcuts
> and checks them when a key is pressed. For more detailed information
> on how this functions, please see the documentation added to the new
> file globalacel.h.
> 
> So far only shortcuts from kmix are white listed. This allows to
> mute and change volume while the screen is locked.
> 
> CCBUG: 148228
> CCBUG: 104353
> FEATURE: 198097
> FIXED-IN: 5.3.0
> 
> 
> Diffs
> -----
> 
>   ksmserver/screenlocker/CMakeLists.txt f73ec98bdc05d5cea7802c5ccb1354b6a3efa2f5 
>   ksmserver/screenlocker/autotests/CMakeLists.txt 9c940a8fe97ae488aeea53d1f1abb3c38c2e13de 
>   ksmserver/screenlocker/globalaccel.h PRE-CREATION 
>   ksmserver/screenlocker/globalaccel.cpp PRE-CREATION 
>   ksmserver/screenlocker/ksldapp.h 2e2e5dcc721d3854fad4ae4019e767a7d1a33718 
>   ksmserver/screenlocker/ksldapp.cpp 8c8607d86d700ade3e1e5b34cbf5c0233897d9ce 
>   ksmserver/screenlocker/lockwindow.h cad62ed0f3583f78b0bdb2d96990c8441b8d3b9d 
>   ksmserver/screenlocker/lockwindow.cpp 0d5afa879051e0802cf1b676ec6024783d3da959 
> 
> Diff: https://git.reviewboard.kde.org/r/122419/diff/
> 
> 
> Testing
> -------
> 
> So far only with the test application
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150204/18f145ee/attachment.html>