> On Sept. 22, 2012, 9:28 a.m., Oswald Buddenhagen wrote: > > 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. > > > > > > Jonathan Marten wrote: > 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. > > Thomas Lübking wrote: > 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?
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. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106524/#review19288 ----------------------------------------------------------- On Sept. 21, 2012, 12:57 p.m., Jonathan Marten wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106524/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2012, 12:57 p.m.) > > > Review request for KDE Runtime. > > > Description > ------- > > 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) > > > This addresses bug 57572. > http://bugs.kde.org/show_bug.cgi?id=57572 > > > Diffs > ----- > > kcontrol/screensaver/scrnsave.h 7c8deba > kcontrol/screensaver/scrnsave.cpp c0507d4 > kscreensaver/krandom_screensaver/random.cpp 4047184 > > Diff: http://git.reviewboard.kde.org/r/106524/diff/ > > > Testing > ------- > > 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. > > > Screenshots > ----------- > > kcmshell4 screensaver > http://git.reviewboard.kde.org/r/106524/s/733/ > > > Thanks, > > Jonathan Marten > >
