Review Request: Use D-Bus to lock sessions and move "session locked" dialog (from KDevelop's main()) to kdevplatform

[Milian Wolff]

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



shell/sessioncontroller.h
<http://git.reviewboard.kde.org/r/105917/#comment14064>

    I'd rather move this to a SessionLock class/interface (see below for the reasoning)



shell/sessioncontroller.cpp
<http://git.reviewboard.kde.org/r/105917/#comment14047>

    rename to DBusSerivceNameForSession, and you could just remove the first and last chars of the id, no? i.e. using .mid()?
    
    also I think andreas comment is still valid, rename the service to
    
    org.kde.kdevplatform-lock-$app-$id
    
    The reason to include $app is that Quanta sessions are not shared with KDevelop sessions e.g.



shell/sessioncontroller.cpp
<http://git.reviewboard.kde.org/r/105917/#comment14046>

    lowercase and rename to ownDBusServiceName



shell/sessioncontroller.cpp
<http://git.reviewboard.kde.org/r/105917/#comment14049>

    this looks still wrong (kdevelop still hardcoded).
    
    I'm not yet very acquainted with DBUS, so I'm not sure how to fix this - is the path and interface name also app-name specific? and for the interface name, could we maybe introduce a SessionLock class and publish it to DBUS at the root path (/) and name its DBUS interface org.kde.kdevplatform.sessionlock?



shell/sessioncontroller.cpp
<http://git.reviewboard.kde.org/r/105917/#comment14060>

    if possible, I'd prefer if we could query the session lock once for all three parameters. esp. if the other app is hanging, all three dbus calls must now timeout before we can do something, no?



shell/sessioncontroller.cpp
<http://git.reviewboard.kde.org/r/105917/#comment14061>

    here again it is hardcoded to kdevelop. also, I'd prefer if this method is merged with the session lock.
    
    I find the whole code a bit convoluted, I think I'll have to apply it locally and try to grasp it there, instead of gluing the diff bits and pieces together.
    
    what I had in mind was more something like
    
    - lock session (by registering dbus interface)
    - if failed b/c dbus interface exists, try to make session visible
    - if that fails, we are out of luck, since the app has crashed, or is hanging. in neither case can we query the app for it's pid, name or similar via dbus. instead, we could (and should) only offer the user a way to pick a different session to launch.
    
    if we really want to have the pid + name and such, we'll have to also keep the klockfile...



shell/sessioncontroller.cpp
<http://git.reviewboard.kde.org/r/105917/#comment14062>

    are you sure these enums map to the reset, configure and quit items?



shell/sessioncontroller.cpp
<http://git.reviewboard.kde.org/r/105917/#comment14063>

    looking at the source code of the showSessionChooserDialog, it has a race condition when it is shown and afterwards the user opens one of the shown sessions via the console e.g.
    
    it does not validate its return value, and thus you could potentially return the session id of a locked session here.


- Milian Wolff


On Aug. 19, 2012, 11:13 p.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105917/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2012, 11:13 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> 1: Use D-Bus instead of lock-files to lock kdevplatform sessions.
> 
> 2: Moved the lockfile remove dialog which currently resides in KDevelop's main() - and which is usable only when the session name is explicitly given to the application - to kdevplatform's SessionController, adjusting the dialog for "lockfile -> dbus" changes.
>     
> This allows to get rid of nasty non-informative error messages about a session being already used and replace them with code that does the following things:
> 1) Attempts a DBus call to make a running instance visible;
> 2) If it didn't succeed, shows a dialog window asking permission to retry, show session chooser dialog or quit;
> 3) Repeats the entire procedure if a newly-picked session is locked too.
>     
> This code is also used when one picks a session from "Sessions" menu, so a nasty error message has also been removed also from there.
> 
> Related change to KDevelop is here: https://git.reviewboard.kde.org/r/105918/ .
> 
> 
> Diffs
> -----
> 
>   shell/core.cpp 206d48d 
>   shell/sessioncontroller.h 551894d 
>   shell/sessioncontroller.cpp c9fac67 
> 
> Diff: http://git.reviewboard.kde.org/r/105917/diff/
> 
> 
> Testing
> -------
> 
> Existing unit-tests + manual UI testing.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120822/cef61f6d/attachment.html>