<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/123705/">https://git.reviewboard.kde.org/r/123705/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 9th, 2015, 7: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;">With this API change, I wonder if it makes sense for kconfiggui.cpp to retain ownership of the KConfig object. Wouldn't it be safer if the caller got ownership of it, saved into it, and then discarded the KConfig instance? Then we wouldn't have this "lose unsaved changes" code path. Or is the idea that multiple parts of the code could connect to the qApp signal, so they should share the same KConfig instance?</p></pre>
</blockquote>
<p>On May 10th, 2015, 2:40 p.m. UTC, <b>Stefan Becker</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;">I understood that KConfigGui is responsible for the applications' session configuration. I.e. there are the following situations:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">application started without -session command line argumen:t KConfigGui::sessionConfig() must never be called. That's what canBeRestored() is all about.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">application started with -session command line argument: KConfig::sessionConfig() needs to create KConfig object based on that name so that the application can restore its state.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">session manager emits saveStateRequest signal. At this point the old session config file has become obsolete and a new one needs to be generated -> KConfigGui::sessionConfig(&sm). The application must file that KConfig object with the new status. If code calls KConfigGui::sessionConfig() it must return the new object.</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree that if kxmlgui KMainWindow is the only user of KConfigGui then the code could be refactored to squash KConfigGui into KMainWindow. But IMHO that is beyond the scope of this bug/review.</p></pre>
</blockquote>
<p>On May 10th, 2015, 2:51 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;">It should definitely be possible to do session management with KConfig and without KXmlGui. I wasn't suggesting otherwise.
I was only suggesting to move ownership, i.e. KConfigGui::sessionSavingConfig() saying "the returned KConfig instance is yours, you must delete it once you're done with it".
But I'm not 100% sure about this idea. In fact it would break the case were independent slots want to save stuff, so let's forget about that idea.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Your description does confirm my thinking that the new method should rather be called something like sessionSavingConfig(). Different signature, different method name, different purpose, different filename... and different underlying KConfig object.</p></pre>
</blockquote>
<p>On May 10th, 2015, 3:06 p.m. UTC, <b>Stefan Becker</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;">OK, but sessionConfig() must still return the new object. Just in case some other code in the application calls that function after sessionSavingConfig(sm&) has been called.</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;">Ah I see what you mean. This bugfix in the libs doesn't change what apps do. My idea (splitting loading and saving) is too radical at this point then.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe KConfigGui's new API should look more like a setter then, if we are changing state.
(We are not just requesting something, that method changes what sessionConfig() returns.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So maybe the new method should be setSessionConfig(QSessionManager) (but the naming isn't great) or maybe setSessionConfig(QString) and the caller (xmlgui) would use KConfigGui::setSessionConfig(KConfigGui::sessionConfigName(QSessionManager)).</p></pre>
<br />
<p>- David</p>
<br />
<p>On May 10th, 2015, 10:30 a.m. UTC, Stefan Becker 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 Rex Dieter.</div>
<div>By Stefan Becker.</div>
<p style="color: grey;"><i>Updated May 10, 2015, 10:30 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=346768">346768</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</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;">When the application receives a saveState signal it needs to replace the current KConfig object with a new one based on the QSessionManager information. Add a new variant that accepts the session manager object.</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;">On F22 with kwrite & konsole</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/gui/kconfiggui.h <span style="color: grey">(173400f)</span></li>
<li>src/gui/kconfiggui.cpp <span style="color: grey">(0048c60)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123705/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>