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

Wed Aug 22 08:52:45 UTC 2012

> On Aug. 22, 2012, 8 a.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 314
> > <http://git.reviewboard.kde.org/r/105917/diff/2/?file=79183#file79183line314>
> >
> >     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.

D-Bus tutorials (http://techbase.kde.org/Development/Tutorials/D-Bus/Introduction#Services) say that we should use reverse domain of the project's homepage for service names... Just confirm that you want "org.kde.kdevplatform" instead of "org.kdevelop"?

> On Aug. 22, 2012, 8 a.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 853
> > <http://git.reviewboard.kde.org/r/105917/diff/2/?file=79183#file79183line853>
> >
> >     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?

The service/interface names are hardcoded everywhere in kdevplatform, see above for explanation...
Though "org.kdevelop.kdevelop.KDevelop" here is misaligned with just "org.kdevelop" in every other exported service, but that's for another review (maybe).

> On Aug. 22, 2012, 8 a.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 321
> > <http://git.reviewboard.kde.org/r/105917/diff/2/?file=79183#file79183line321>
> >
> >     lowercase and rename to ownDBusServiceName

Ok.

> On Aug. 22, 2012, 8 a.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 865
> > <http://git.reviewboard.kde.org/r/105917/diff/2/?file=79183#file79183line865>
> >
> >     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?

Ok, will do one method returning a list of strings.

> On Aug. 22, 2012, 8 a.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 1071
> > <http://git.reviewboard.kde.org/r/105917/diff/2/?file=79183#file79183line1071>
> >
> >     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...

We need pid (just pid, no attempts to lock/regain/annoy user/whatever) for kdevelop's main() and argument "--pid" there.
Though ok, I'll try to merge with tryLockSession() using a boolean parameter there to tell whether shall we attempt to show dialog.

But - without appname/pid the dialog will lose a part of its informativity (probably the most important one in case of a hanging process).
And another note: a shutting down instance of kdev* behaves identically to hanging; it would improve user experience if we could distinguish these situations and, in case of app shutting down, wait for it to exit instead of bailing out with an error.

> On Aug. 22, 2012, 8 a.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 1102
> > <http://git.reviewboard.kde.org/r/105917/diff/2/?file=79183#file79183line1102>
> >
> >     are you sure these enums map to the reset, configure and quit items?

Sure. We've got a "yesNoCancelDialog()" with parameters for yes-, no- and cancel-buttons.

> On Aug. 22, 2012, 8 a.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 1113
> > <http://git.reviewboard.kde.org/r/105917/diff/2/?file=79183#file79183line1113>
> >
> >     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.

This is intentionally left; the dialog shows all sessions and their statuses (so it is left to the user to behave consciously) - and, for the fool-proof, session-locking is made iterative (that is, a session whose id returned by handleLockedSessionDialog() is still subject for locking).

- Ivan

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

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/b87eaed0/attachment.html>