JuK - removing k3support libraries

Georg Grabler ggrabler at gmail.com
Sun May 10 21:22:44 BST 2009

Already recoded those parts. Will update the patch until wednesday (a bit
hard during the week when I'm working .. to be ture, it's sunday and I'm
currently at work :p).

On Sun, May 10, 2009 at 1:27 PM, Georg Grabler <ggrabler at gmail.com> wrote:

> Hi Michael,
> 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.
> 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.
> 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.
> Anyway, I learned about models, means I can continue with parts where I
> definitely will need my own models and delegates (covermanager, playlist).
> 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.
> Kind regards,
> Georg
> 2009/5/10 Michael Pyne <mpyne at purinchu.net>
>> On Saturday 09 May 2009 13:39:00 Georg Grabler wrote:
>> > Hello,
>> >
>> > First of all - hi Michael, I'm still alive (pest plants never die).
>> >
>> > I've started porting some JuK parts which still contain k3support
>> libraries
>> > to qt4. I started with the easier parts before i get to the harder
>> things
>> > as the playlist and the covermanager.
>> >
>> > I thought, I'd better send you what I did until now, for two reasons:
>> > - Review
>> > - Possible conflicts if somebody of you guys updates JuK
>> I see you've also put it up on review board.  I'd review it there
>> (assuming it
>> works in either Konq or Arora yet...) but my password was lost in the
>> Great
>> Kernel Panic of 2009 since it was newer than my last $KDEHOME backup, and
>> there is no way I can see to reset review board passwords.
>> So I'll just do it the old-fashioned way. ;)
>> Starting from tagguesserconfigdlg.cpp:
>> Removing the <kicon.h> include is probably not a good idea, even if it
>> still
>> compiles, as the KIcon constructor is used.
>> Also, some lines are re-arranged in the TagGuesserConfigDlg constructor,
>> for no
>> reason I can see.  Does this improve anything?  Reason I ask is because it
>> makes it harder to figure out why you removed some lines and then oh-wait,
>> they
>> just moved down somewhere else.
>> +    else if (m_child->bMoveUp->isEnabled() == false)
>> +        m_child->bMoveUp->setEnabled(true);
>> This can just be m_child->bMoveUp->setEnabled(true), doing the specific
>> check
>> isn't necessary.  If it is important that the result not be duplicated
>> then
>> typically the class with the setFoo method should ensure that, not calling
>> code.
>> +    if (item == model->index(model->rowCount(QModelIndex())-1, 0,
>> QModelIndex()))
>> +        m_child->bMoveDown->setEnabled(false);
>> There is probably a bug here if there are no schemes in the model, be sure
>> to
>> check for that as well.  Also my advice about checking for enabledness
>> applies
>> here as well.
>> On an overall note the ::accept() method is a bit convoluted trying to
>> extract
>> a QStringList out of the model.  But Qt already provides a
>> QStringListModel,
>> which you should either use directly and remove TagGuesserTreeModel, or
>> make
>> TagGuesserTreeModel a subclass of, that way your way of getting the
>> QStringList could be simply model->stringList().
>> Likewise the DirectoryList changes would probably easier using a
>> QStringListModel since it just holds a list of folder names.
>> I haven't actually had a chance to compile and run the patched version,
>> I'll
>> let you know if there are problems that weren't already present. ;)
>> Regards,
>>  - Michael Pyne
>> _______________________________________________
>> kde-multimedia mailing list
>> kde-multimedia at kde.org
>> https://mail.kde.org/mailman/listinfo/kde-multimedia
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20090510/218e8a48/attachment.htm>
-------------- next part --------------
kde-multimedia mailing list
kde-multimedia at kde.org

More information about the kde-multimedia mailing list