<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127271/">https://git.reviewboard.kde.org/r/127271/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 5th, 2016, 9:27 a.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looks good and more portable than the
qunsetenv("SESSION_MANAGER");
which is used in many other places...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not sure both connects are necessary though?</p></pre>
</blockquote>
<p>On March 5th, 2016, 6:34 p.m. UTC, <b>Xuetian Weng</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is not to disable the whole session manager thing, but just disable the restore. So session manager is still able to terminate kwalletd.</p></pre>
</blockquote>
<p>On March 5th, 2016, 6:56 p.m. UTC, <b>David Faure</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;">Ah. Do you mean that with this code, the process is terminated more gracefully than with the qunsetenv solution which e.g. kiod or kded use (I guess they simply die with an X error when Xorg disappears). Sounds like another reason for this approach indeed, although it probably doesn't make much difference from a user's perspective [other than portability of course].
Sounds like we should apply the same code in all these:
kded/src/kded.cpp:682: qunsetenv("SESSION_MANAGER");
kdesu/src/ptyprocess.cpp:305: unsetenv("SESSION_MANAGER");
kglobalaccel/src/runtime/main.cpp:56: qunsetenv( "SESSION_MANAGER" );
kinit/src/klauncher/klauncher_main.cpp:153: qunsetenv("SESSION_MANAGER");
kio/src/kiod/kiod_main.cpp:89: qunsetenv("SESSION_MANAGER");
This also makes me wonder what happens to non-GUI daemons, i.e. what will terminate them when logging out... (all of the above are GUI enabled, but e.g. kio_http_cache_cleaner is core-only).</pre>
</blockquote>
<p>On March 5th, 2016, 8:19 p.m. UTC, <b>Xuetian Weng</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, I doubt this method is useful for cases above. kiod/kded might not want to be exit by session manager maybe? Because when session terminates, it is still possible for application like kwrite/kate to use kio to write or open something.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think most of them (at least for klauncher/kded/kiod) are currently doing the right thing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kio_http_cache_cleaner should be managed by kio so I don't think we need to worry about it. core-only application could possible leave some garbage behind, for example: https://git.reviewboard.kde.org/r/125746/</p></pre>
</blockquote>
<p>On March 5th, 2016, 8:42 p.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ah, but then it's the same for kwalletd.... if kwrite uses KIO to save to an FTP directory, this might require a password, which would then require kwalletd. I see no difference between kwalletd and kiod/kded in that respect.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But I don't even think that's a problem. Closing the session happens in several steps (asking all apps to save state, then asking all apps to prompt the user for saving open files, and only then closing all windows and quitting the apps). So I don't think klauncher/kded/kiod/kwalletd being asked to quit by the session manager would break anything, that's only the very last step after all the saving is done.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kio_http_cache_cleaner is started by the first kio_http-using apps, but quitting it is difficult: it's a "singleton" process, so kio_http can't just make it quit on exit. It would need refcounting [difficult in case kio_http crashes] .... or indeed it could be QGuiApplication (not good for possible core-only kio usage). Tricky.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then you would need to use things like https://git.reviewboard.kde.org/r/127125/ . But I still think there could be some order issue. I get the idea that kwalletd could probably just use the unset QSESSION_MANAGER method.</p></pre>
<br />
<p>- Xuetian</p>
<br />
<p>On March 3rd, 2016, 8:34 p.m. UTC, Xuetian Weng wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and Martin Klapetek.</div>
<div>By Xuetian Weng.</div>
<p style="color: grey;"><i>Updated March 3, 2016, 8:34 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwallet
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I notice a kwalletd5 with "-session ....." in its command line started on my desktop and kwallet-pam doesn't work.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also kwalletd is dbus activated in other cases there's no point to let session manager to restore it.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kwallet-pam back to work.</p></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>src/runtime/kwalletd/main.cpp <span style="color: grey">(740e670)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127271/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>