<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/106524/">http://git.reviewboard.kde.org/r/106524/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 22nd, 2012, 9:28 a.m., <b>Oswald Buddenhagen</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;">this UI is completely opaque to the user and thus not acceptable.
you have two options:
- add a proper header to the tree and put the checkboxes into a separate column "allow in 'random'". the list's double function is still confusing, though.
- put it into a separate config dialog after all. with some minor refactoring there shouldn't be much code duplication. i'd prefer this approach.
i'm pretty sure this patch will massively conflict with http://git.reviewboard.kde.org/r/106124/
please use kdelibs (or even better qt, as far as i'm concerned) coding style for new code. at the very least stay consistent with yourself.
</pre>
</blockquote>
<p>On September 22nd, 2012, 6:24 p.m., <b>Jonathan Marten</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;">Thanks for the pointer to the very involved review currently in progress; I knew about the screensaver/locker move to ksmserver and QML (but thought that that had all been comitted ages ago); not sure why the changes to the kcontrol module didn't get into my personal repo here.
Agree that the logical place for the selection to go is in the "Setup..." dialogue for the random saver, but the problem is that that is managed by krandom.kss which is in a completely different place in the source tree. So I thought it may not be a good idea to either make the two separate components interdependent, or make maintenance more difficult by copying a big block of code from one to the other. Could resolve this by having the setup for the random saver (and that one saver only) be a special case internally managed by the kcmodule, then the same code could be used to generate both lists.
Or could leave the krandom.kss setup dialogue alone, and have a separate "Select Random Savers..." button in the kcmodule.
Given that more changes are obviously in the pipeline, I'll keep this review on hold for the moment. When the big screenlocker merge is resolved then I'll look again at the random selection following your suggestions.</pre>
</blockquote>
<p>On September 22nd, 2012, 6:30 p.m., <b>Thomas Lübking</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;">Is it not possible to use the general screensaver QTreeWidget (sigh) model() (in doubt export it?) for a second QTreeView in the krandom config dialog (filter or disable the random.kss item) and use the selectionModel() of that config page to write the krandom config?</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;">Yes, that would be my preferred way of doing it. Means that the random saver is a special case, but then it is already. Also some UI gets moved from krandom.kss back to the kcmodule, but it's only 2 checkboxes.
</pre>
<br />
<p>- Jonathan</p>
<br />
<p>On September 21st, 2012, 12:57 p.m., Jonathan Marten 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 KDE Runtime.</div>
<div>By Jonathan Marten.</div>
<p style="color: grey;"><i>Updated Sept. 21, 2012, 12:57 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;">The referenced bug suggests that it should be possible to individually select screen savers that are to be considered when the "Random" option is chosen. This patch implements that.
In order to avoid having to duplicate the complete saver tree view (and the code to generate it) within the random saver's setup mode, check boxes are added to the kcontrol saver list (with the exception of the random saver itself). The state of these is saved to the random saver's config file. This selection is in addition to the two options within the saver's setup dialogue, so if for example "Use OpenGL screen savers" is not checked than any OpenGL savers will be ignored even if their boxes are checked in the tree view.
(Submitting to kde-runtime, there is no kde-workspace group)
</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;">Built saver and kcontrol module with these changes. Checked operation of kcontrol module, saving of settings in the config file and operation of the random saver.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=57572">57572</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kcontrol/screensaver/scrnsave.h <span style="color: grey">(7c8deba)</span></li>
<li>kcontrol/screensaver/scrnsave.cpp <span style="color: grey">(c0507d4)</span></li>
<li>kscreensaver/krandom_screensaver/random.cpp <span style="color: grey">(4047184)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106524/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/106524/s/733/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/09/21/screensaver-random_400x100.png" style="border: 1px black solid;" alt="kcmshell4 screensaver" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>