JuK - removing k3support libraries

[Georg Grabler]

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/5fd58030/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia