[prev in list] [next in list] [prev in thread] [next in thread]
List: kdevelop-devel
Subject: Re: Review Request: Move lockfile remove request dialog (from KDevelop's main()) to kdevplatform.
From: "Andreas Pakulat" <apaku () gmx ! de>
Date: 2012-08-18 10:42:33
Message-ID: 20120818104233.4482.37628 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Aug. 16, 2012, 7:20 p.m., Milian Wolff wrote:
> > shell/sessioncontroller.cpp, line 1038
> > <http://git.reviewboard.kde.org/r/105917/diff/1/?file=76522#file76522line1038>
> >
> > now that this is in kdevplatform, we'll need to figure out what to do with this..
> >
> > org.kdevelop.kdevelop-... is not correct for anything but the kdevelop app \
> > itself. what about quanta? what about ktechlab? if we cannot figure out the name \
> > from e.g. QCoreApplication, we might need to extend our ShellExtension class with \
> > the required dbus bits and pieces :(
>
> Ivan Shapovalov wrote:
> Damn. :(
> Haven't thought about interface name.
> Everything else's trivial, but what is the name for different kdev-based \
> applications? Does it follow any conventions (like org.$appname.$appname)?
> Ivan Shapovalov wrote:
> ...Or is that interface KDevelop-specific?
>
> Milian Wolff wrote:
> A hint from Kevin Kramer:
>
> ~~~~~
> You could consider using a D-Bus "lock" instead of a lock file.
>
> Basically when opening a session you register a D-Bus name which includes the
> session identifier.
> Only one process can have that name registered and the registration is undone
> no matter how the process exits.
>
> The name can also be used to send the other KDevelop instance the request to
> regain focus.
> ~~~~~
>
> I like this, it's easy and apparently quite widely used (KDEPIM also uses that, as \
> does KUniqueApplication). So I'm afraid we first have to do that, and then can move \
> the lock-detection code to KDevplatform.
> Ivan Shapovalov wrote:
> That seems an elegant solution. How can we register such an unique name? I guess \
> we'll need to avoid registering a usual (org.kdevelop.$APPNAME-$PID) name \
> somehow...
> Ivan Shapovalov wrote:
> ..And what happens to the scenario when the app is "crashed or hanging (stopped by \
> DrKonqi, ...)" and one currently can just force-remove the lockfile?
> Milian Wolff wrote:
> The names must be chosen by kdevplatform, in a unique manner for a given \
> session-id+app pair (quanta sessions should not interfer with kdevelop sessions \
> though).
> Thus, I propose to use a scheme such as org.kdevplatform.$appname-$sessionid or \
> similar.
> Regarding crashed/hanging processes: Well, if the app has crashed, it should have \
> deregistered its dbus interface, hence this should not be a problem.
> Now, if a crash handler is called and the dbus interface stays open, or worse, if \
> the app hangs, then we should imo offer the user a way to kill the process and try \
> again. Of course, with a fat warning.
> Ivan Shapovalov wrote:
> Well, a name scheme is a secondary question. _Who_ will register these names?
> Currently registration is done by KApplication itself, but it's naming scheme is \
> AFAIK fixed to $domain.$app-$pid.
> Moreover, killing a stopped (by a handler such as DrKonqi) process is not possible \
> (unless you use signal 9, but that's discouraged as resources are not freed)...
I think thats what Milian was getting at, register the name from kdevelop or via a \
shared kdeveplatform function.
I disagree on the naming scheme though, I'd rather say it ought to be \
org.kde.kdevplatform.$appname-$sessionid so its still clear this is a KDE app and \
there's not really any domain called kdevplatform.org...
In addition, neither a hanging process nor one crashed and sitting in dr.konqi needs \
to be of concern IMHO, except for notifying the user when trying to start that same \
session again. If the name is still registered simply tell the user that there's \
still a process with that session and he should check for hanging kdevelop instances \
or a dr.konqi window. In reality though he'll probably have killed the process or \
closed dr.konqi already anyway.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105917/#review17551
-----------------------------------------------------------
On Aug. 7, 2012, 7:02 p.m., Ivan Shapovalov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105917/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2012, 7:02 p.m.)
>
>
> Review request for KDevelop.
>
>
> Description
> -------
>
> 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.
> This allows to get rid of nasty non-informative error messages about a session \
> being already used and replace them with partially refactored code from KDevelop's \
> main() which does the following things: 1) Attempts a DBus call to make a running \
> instance visible; 2) If it didn't succeed, a dialog window is shown asking \
> permission to force-remove the lockfile or show session chooser dialog; 3) If a \
> newly-picked session is also locked, the entire procedure is repeated.
> 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
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105917/">http://git.reviewboard.kde.org/r/105917/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On August 16th, 2012, 7:20 p.m., <b>Milian \
Wolff</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/105917/diff/1/?file=76522#file76522line1038" \
style="color: black; font-weight: bold; text-decoration: \
underline;">shell/sessioncontroller.cpp</a> <span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
"></pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: \
0; ">QString SessionController::showForceOpenDialog( const QString& sessionName, \
const QString& sessionId, const SessionController::LockSessionState& state \
)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">1024</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">QDBusInterface</span> <span class="n">interface</span><span \
class="p">(</span><span class="n">QString</span><span class="p">(</span><span \
class="s">"org.kdevelop.kdevelop-%1"</span><span class="p">).</span><span \
class="n">arg</span><span class="p">(</span><span class="n">state</span><span \
class="p">.</span><span class="n">holderPid</span><span \
class="p">),</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">now that this is in \
kdevplatform, we'll need to figure out what to do with this..
org.kdevelop.kdevelop-... is not correct for anything but the kdevelop app itself. \
what about quanta? what about ktechlab? if we cannot figure out the name from e.g. \
QCoreApplication, we might need to extend our ShellExtension class with the required \
dbus bits and pieces :(</pre> </blockquote>
<p>On August 16th, 2012, 7:43 p.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Damn. :( Haven't \
thought about interface name. Everything else's trivial, but what is the name for \
different kdev-based applications? Does it follow any conventions (like \
org.$appname.$appname)?</pre> </blockquote>
<p>On August 16th, 2012, 7:48 p.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">...Or is that interface \
KDevelop-specific?</pre> </blockquote>
<p>On August 18th, 2012, 12:47 a.m., <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A hint from Kevin \
Kramer:
~~~~~
You could consider using a D-Bus "lock" instead of a lock file.
Basically when opening a session you register a D-Bus name which includes the
session identifier.
Only one process can have that name registered and the registration is undone
no matter how the process exits.
The name can also be used to send the other KDevelop instance the request to
regain focus.
~~~~~
I like this, it's easy and apparently quite widely used (KDEPIM also uses that, \
as does KUniqueApplication). So I'm afraid we first have to do that, and then can \
move the lock-detection code to KDevplatform.</pre> </blockquote>
<p>On August 18th, 2012, 8:16 a.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That seems an elegant \
solution. How can we register such an unique name? I guess we'll need to avoid \
registering a usual (org.kdevelop.$APPNAME-$PID) name somehow...</pre> </blockquote>
<p>On August 18th, 2012, 8:19 a.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">..And what happens to \
the scenario when the app is "crashed or hanging (stopped by DrKonqi, ...)" \
and one currently can just force-remove the lockfile?</pre> </blockquote>
<p>On August 18th, 2012, 9:54 a.m., <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The names must be chosen \
by kdevplatform, in a unique manner for a given session-id+app pair (quanta sessions \
should not interfer with kdevelop sessions though).
Thus, I propose to use a scheme such as org.kdevplatform.$appname-$sessionid or \
similar.
Regarding crashed/hanging processes: Well, if the app has crashed, it should have \
deregistered its dbus interface, hence this should not be a problem.
Now, if a crash handler is called and the dbus interface stays open, or worse, if the \
app hangs, then we should imo offer the user a way to kill the process and try again. \
Of course, with a fat warning.</pre> </blockquote>
<p>On August 18th, 2012, 10:21 a.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, a name scheme is a \
secondary question. _Who_ will register these names? Currently registration is done \
by KApplication itself, but it's naming scheme is AFAIK fixed to \
$domain.$app-$pid.
Moreover, killing a stopped (by a handler such as DrKonqi) process is not possible \
(unless you use signal 9, but that's discouraged as resources are not \
freed)...</pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think \
thats what Milian was getting at, register the name from kdevelop or via a shared \
kdeveplatform function.
I disagree on the naming scheme though, I'd rather say it ought to be \
org.kde.kdevplatform.$appname-$sessionid so its still clear this is a KDE app and \
there's not really any domain called kdevplatform.org...
In addition, neither a hanging process nor one crashed and sitting in dr.konqi needs \
to be of concern IMHO, except for notifying the user when trying to start that same \
session again. If the name is still registered simply tell the user that there's \
still a process with that session and he should check for hanging kdevelop instances \
or a dr.konqi window. In reality though he'll probably have killed the process or \
closed dr.konqi already anyway.</pre> <br />
<p>- Andreas</p>
<br />
<p>On August 7th, 2012, 7:02 p.m., Ivan Shapovalov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Ivan Shapovalov.</div>
<p style="color: grey;"><i>Updated Aug. 7, 2012, 7:02 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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.
This allows to get rid of nasty non-informative error messages about a session being \
already used and replace them with partially refactored code from KDevelop's \
main() which does the following things: 1) Attempts a DBus call to make a running \
instance visible; 2) If it didn't succeed, a dialog window is shown asking \
permission to force-remove the lockfile or show session chooser dialog; 3) If a \
newly-picked session is also locked, the entire procedure is repeated.
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/ .</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Existing unit-tests + manual UI testing.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>shell/core.cpp <span style="color: grey">(206d48d)</span></li>
<li>shell/sessioncontroller.h <span style="color: grey">(551894d)</span></li>
<li>shell/sessioncontroller.cpp <span style="color: grey">(c9fac67)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105917/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
KDevelop-devel mailing list
KDevelop-devel@kdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic