Hi Michael,<br><br>Well, first of all - i dropped it here before I could the reviewboard working :p ... had some troubles finding Scott or you in there as a reviewer.<br><br>Secondly, the swapping of the lines does not really improve anything, they got turned when I edited the parts. I'll try to update this accordingly, so the patch is smaller.<br>
<br>I've never looked at QStringListModel (to be true, I didn't know it even exists, since the documentation had a stringlistmodel as an example, i though there isn't anything like that). I'll take a look at those parts, and will send a updated version of the patch (a link this time, had troubles getting this patch in the lists due to being taller than 40kb) and update it on the reviewboard.<br>
<br>Anyway, I learned about models, means I can continue with parts where I definitely will need my own models and delegates (covermanager, playlist).<br><br>For the directorylist .. well, since I didn't know about QStringListModel, it was likely that the solution is the easiest I did know. Will also take a look at this.<br>
<br>Kind regards,<br>Georg<br><br><div class="gmail_quote">2009/5/10 Michael Pyne <span dir="ltr"><<a href="mailto:mpyne@purinchu.net">mpyne@purinchu.net</a>></span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">On Saturday 09 May 2009 13:39:00 Georg Grabler wrote:<br>
> Hello,<br>
><br>
> First of all - hi Michael, I'm still alive (pest plants never die).<br>
><br>
> I've started porting some JuK parts which still contain k3support libraries<br>
> to qt4. I started with the easier parts before i get to the harder things<br>
> as the playlist and the covermanager.<br>
><br>
> I thought, I'd better send you what I did until now, for two reasons:<br>
> - Review<br>
> - Possible conflicts if somebody of you guys updates JuK<br>
<br>
</div>I see you've also put it up on review board.  I'd review it there (assuming it<br>
works in either Konq or Arora yet...) but my password was lost in the Great<br>
Kernel Panic of 2009 since it was newer than my last $KDEHOME backup, and<br>
there is no way I can see to reset review board passwords.<br>
<br>
So I'll just do it the old-fashioned way. ;)<br>
<br>
Starting from tagguesserconfigdlg.cpp:<br>
<br>
Removing the <kicon.h> include is probably not a good idea, even if it still<br>
compiles, as the KIcon constructor is used.<br>
<br>
Also, some lines are re-arranged in the TagGuesserConfigDlg constructor, for no<br>
reason I can see.  Does this improve anything?  Reason I ask is because it<br>
makes it harder to figure out why you removed some lines and then oh-wait, they<br>
just moved down somewhere else.<br>
<br>
+    else if (m_child->bMoveUp->isEnabled() == false)<br>
+        m_child->bMoveUp->setEnabled(true);<br>
<br>
This can just be m_child->bMoveUp->setEnabled(true), doing the specific check<br>
isn't necessary.  If it is important that the result not be duplicated then<br>
typically the class with the setFoo method should ensure that, not calling<br>
code.<br>
<br>
+    if (item == model->index(model->rowCount(QModelIndex())-1, 0,<br>
QModelIndex()))<br>
+        m_child->bMoveDown->setEnabled(false);<br>
<br>
There is probably a bug here if there are no schemes in the model, be sure to<br>
check for that as well.  Also my advice about checking for enabledness applies<br>
here as well.<br>
<br>
On an overall note the ::accept() method is a bit convoluted trying to extract<br>
a QStringList out of the model.  But Qt already provides a QStringListModel,<br>
which you should either use directly and remove TagGuesserTreeModel, or make<br>
TagGuesserTreeModel a subclass of, that way your way of getting the<br>
QStringList could be simply model->stringList().<br>
<br>
Likewise the DirectoryList changes would probably easier using a<br>
QStringListModel since it just holds a list of folder names.<br>
<br>
I haven't actually had a chance to compile and run the patched version, I'll<br>
let you know if there are problems that weren't already present. ;)<br>
<br>
Regards,<br>
<font color="#888888"> - Michael Pyne<br>
</font><br>_______________________________________________<br>
kde-multimedia mailing list<br>
<a href="mailto:kde-multimedia@kde.org">kde-multimedia@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/kde-multimedia" target="_blank">https://mail.kde.org/mailman/listinfo/kde-multimedia</a><br>
<br></blockquote></div><br>