Review Request: Move screensaver and locking functionality to kwin from krunner

Alex Merry kde at randomguy3.me.uk
Wed Jul 13 15:59:42 CEST 2011 


> On July 13, 2011, 12:54 p.m., Aaron J. Seigo wrote:
> > "I disabled it for Plasma active, but that may not be appropriate."
> > 
> > we still need screen locking in Active, so this probably isn't entirely correct. what we probably want, however, is a replacement for the actual lock process which isn't appropriate for Active. right now we have a QML based thing that doesn't use passwords, and it would be very nice to use that as a full screen app in replacement of the desktop-appropriate lock app. this would also neatly remove the desktopy screen savers from Active, and that's just fine.
> > 
> > in the patch i don't see where the lock process file get added to.. just lots and lots of code removals :) i assume this was a problem with the post-review script or some such and that you used `git mv` to move the files somewhere appropriate. i actulaly think the lock process should sit in the top level of kde-workspace, not hidden under whatever app launches it. and then we can provide both a password-based one for the desktop in there as well as a touch focused one; perhaps as build options, even.
> > 
> > one other thing that probably needs to be done: the screensaver/lock window should be identifitied as such to the window manager, just as we now do with the desktop's Dashboard windows. dasbhoard marking is done with a simple:
> > 
> > 
> > #ifdef Q_WS_X11
> >     XClassHint classHint;
> >     classHint.res_name = DASHBOARD_WIN_NAME;
> >     classHint.res_class = DASHBOARD_WIN_CLASS;
> >     XSetClassHint(QX11Info::display(), windowId, &classHint);
> > #endif
> > 
> > the name and class are defined in the file as simply "dashboard". we could do the same with the lock window and then when it is showing KWin can do clever things like not paint any other windows.
> > 
> > i like this patch so far, however, other than a few comments that follow inline. Martin also needs to comment on it, of course.

Yes, I moved the lock process to the kwin directory using git mv... clearly git diff didn't show it up properly.  I can move it up to the top directory, maybe as "kscreenlocker", since that's the name of the executable.

The window hinting is a good idea, but should probably go in as a separate patch.

I'll upload a new version addressing the points below this evening.


> On July 13, 2011, 12:54 p.m., Aaron J. Seigo wrote:
> > kwin/workspace.cpp, lines 132-134
> > <http://git.reviewboard.kde.org/r/101943/diff/1/?file=26943#file26943line132>
> >
> >     this is reduendant to line 236 and doesn't provide any useful initialization. the new should either be moved here, this line have a 0 added to it, or be removed.

Oh, that was supposed to have a 0 in it, in keeping with the other initializers in that class.


> On July 13, 2011, 12:54 p.m., Aaron J. Seigo wrote:
> > kwin/workspace.cpp, line 236
> > <http://git.reviewboard.kde.org/r/101943/diff/1/?file=26943#file26943line236>
> >
> >     should check KWIN_BUILD_SCREENSAVER?

Yes.


> On July 13, 2011, 12:54 p.m., Aaron J. Seigo wrote:
> > kwin/workspace.cpp, line 516
> > <http://git.reviewboard.kde.org/r/101943/diff/1/?file=26943#file26943line516>
> >
> >     should check KWIN_BUILD_SCREENSAVER?

Yes.


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101943/#review4680
-----------------------------------------------------------


On July 13, 2011, 11:28 a.m., Alex Merry wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101943/
> -----------------------------------------------------------
> 
> (Updated July 13, 2011, 11:28 a.m.)
> 
> 
> Review request for kwin, Plasma, Aaron J. Seigo, and Martin Gräßlin.
> 
> 
> Summary
> -------
> 
> This transfers the screen locking and screensaver funcitonality wholesale from krunner to kwin.  This has been OK'd in principle by the relevant maintainers.
> 
> Most of the code is simply moved untouched.  Points of note:
> 
> I've introduced a KWIN_BUILD_SCREENSAVER option, to match the other kwin build options, like KWIN_BUILD_TABBOX.  I disabled it for Plasma active, but that may not be appropriate.
> 
> I put the screensaver stuff (creating the SaverEngine object and setting up the shortcut) in KWin::Workspace.  The shortcut stuff is actually in useractions.cpp, but this implements methods in KWin::Workspace.
> 
> I'm using the kglobalaccel D-Bus interface directly to steal the existing shortcut from KRunner.  I'm doing it like this because the KAction/KGlobalAccel interface is not rich enough to do this (probably wisely - this isn't something apps should generally be doing).  The shortcut deregistration happens in KWin rather than KRunner because I don't want to rely on the order in which KWin and KRunner are started (or even on KRunner being started at all).
> 
> 
> Diffs
> -----
> 
>   kcontrol/screensaver/CMakeLists.txt e4dcc3a 
>   krunner/CMakeLists.txt 4455847 
>   krunner/README c8b9740 
>   krunner/config-xautolock.h.cmake eadb0a6 
>   krunner/dbus/org.freedesktop.ScreenSaver.xml 5efd943 
>   krunner/dbus/org.kde.screensaver.xml e700b88 
>   krunner/kcfg/kscreensaversettings.kcfg c8f76f3 
>   krunner/kcfg/kscreensaversettings.kcfgc af9133d 
>   krunner/krunnerapp.h 82db725 
>   krunner/krunnerapp.cpp c143be5 
>   krunner/lock/CMakeLists.txt cf9a67e 
>   krunner/lock/autologout.h 0c44405 
>   krunner/lock/autologout.cc c86e29a 
>   krunner/lock/config-krunner-lock.h.cmake 7bfdfd6 
>   krunner/lock/kscreenlocker.notifyrc 2955c5f 
>   krunner/lock/lockdlg.h f25e55f 
>   krunner/lock/lockdlg.cc 6367216 
>   krunner/lock/lockprocess.h 8b6d9a8 
>   krunner/lock/lockprocess.cc ecc632f 
>   krunner/lock/main.h 8a60353 
>   krunner/lock/main.cc 9f1c9b8 
>   krunner/main.cpp 84a547b 
>   krunner/screensaver/saverengine.h 3384d4a 
>   krunner/screensaver/saverengine.cpp 6c15be6 
>   krunner/screensaver/xautolock.h 3db3233 
>   krunner/screensaver/xautolock.cpp 7124215 
>   krunner/screensaver/xautolock_c.h 3b82f5c 
>   krunner/screensaver/xautolock_diy.c b9df2f8 
>   krunner/screensaver/xautolock_engine.c d6d0cf5 
>   kwin/CMakeLists.txt 7d6ea52 
>   kwin/config-kwin.h.cmake a291859 
>   kwin/useractions.cpp 387e499 
>   kwin/workspace.h 66b9830 
>   kwin/workspace.cpp 8cf5299 
>   plasma/desktop/applets/kickoff/CMakeLists.txt bc5fa2e 
>   plasma/generic/applets/lock_logout/CMakeLists.txt 8381d46 
>   plasma/generic/containmentactions/contextmenu/CMakeLists.txt a1fc205 
>   plasma/generic/runners/sessions/CMakeLists.txt 1b8292c 
>   powerdevil/daemon/CMakeLists.txt bad3dae 
> 
> Diff: http://git.reviewboard.kde.org/r/101943/diff
> 
> 
> Testing
> -------
> 
> Allowing the screensaver to activate (both terminating the screensaver before it locks and after, with lock after 60 seconds set).
> 
> Using the lock screen action from krunner.
> 
> Stealing a non-default shortcut from KRunner (set the krunner Lock Session shortcut to another sequence, and ran KWin; KWin successfully deregistered krunner's Lock Session shortcut and assigned the key sequence to its own Lock Session shortcut).
> 
> Running KWin when no existing Lock Session shortcuts had been defined (either for krunner or kwin).  KWin successfully registered its Lock Session shortcut with the default key sequence (this is what would happen with a fresh user account).
> 
> 
> Thanks,
> 
> Alex
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20110713/a9c15d2d/attachment.htm

More information about the Plasma-devel mailing list