Review Request: Use MetaQueryWidget in the advanced playlist generator.

[Ralf Engels]

> On 2010-10-21 19:03:00, Soren Harward wrote:
> > Using the MetaQueryWidget does not prohibit using the TagMatch::Comparer nested class.  Removing the Comparer nested class from TagMatch is not a good idea, and replacing it with global functions is a genuinely bad one.  I split off the Comparer class because I found myself tweaking the comparison algorithms on a regular basis, and I wanted to make changes in only one place.  Please put it back.  Thanks.
> 
> Soren Harward wrote:
>     Also, the CompareLabels functionality in the comparer must be working before this patch can ship.  Right now it's a TODO.

Hi Soren,
thank you for your review. I was waiting for someone from the Playlist Generator to post a review.

Can you please explain to me a little more why the TagMatch::Compare was needed to be seperate?
There is still only one place to change for the algorithms.

Here are the simple reasons why I removed the comparer.
It was a complete static class. All of the functions and were in principle static and it didn't have any member variables.
Now, there are still reasons for having such a class (e.g. if you want to extend it) but this one was so tightly coupled to the rest of the constraint that I couldn't see the reason for having it separate.
Expecially since it seems to have some outdated functionality (the list model)

Also the whole class boiled down to five functions.

Do your really want to have a seperate .cpp file (plus .h, because it is confusing to have a .cpp without header) just for those five functions?
And how does this make tweaking easier?

Next point:
The CompareLabels functionality was not implemented as far as I can see. I would be very happy to implement that for you (even though I think that the labels are a big TODO over the whole Amarok).
However I think it would dillute this patch whose main concern was to have a consistent UI.
A seperate patch would be more appropriate for this.

Maybe we can meet on IRC somewhen (Sunday maybe because the next two days are quite full) and talk about some other ideas I had.


- Ralf


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100070/#review170
-----------------------------------------------------------


On 2010-10-18 12:07:31, Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100070/
> -----------------------------------------------------------
> 
> (Updated 2010-10-18 12:07:31)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> Instead of implementing the whole behavior of selecting field and values I am just using the MetaQueryWidget.
> 
> Also I am moving all field related texts to src/core/meta/support/MetaConstants.cpp
> This would also be a good place for the playlist to get it's texts from.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt abdcee8 
>   src/core-impl/collections/support/XmlQueryReader.h 0088608 
>   src/core-impl/collections/support/XmlQueryReader.cpp b5518f0 
>   src/core-impl/collections/support/XmlQueryWriter.h 11c818e 
>   src/core-impl/collections/support/XmlQueryWriter.cpp 657e0c8 
>   src/core/CMakeLists.txt 5863ca1 
>   src/core/meta/support/MetaConstants.h 40cad34 
>   src/core/meta/support/MetaConstants.cpp PRE-CREATION 
>   src/core/meta/support/MetaUtility.h b161bdc 
>   src/core/meta/support/MetaUtility.cpp 0bb29db 
>   src/dynamic/Bias.h 6ec60b6 
>   src/dynamic/Bias.cpp dec2db2 
>   src/playlistgenerator/PresetModel.cpp 9b74636 
>   src/playlistgenerator/constraints/PlaylistDuration.h 992215c 
>   src/playlistgenerator/constraints/PlaylistDuration.cpp 5d83dfe 
>   src/playlistgenerator/constraints/TagMatch.h 094a68b 
>   src/playlistgenerator/constraints/TagMatch.cpp a719825 
>   src/playlistgenerator/constraints/TagMatchEditWidget.ui b9d97c6 
>   src/playlistgenerator/constraints/TagMatchSupport.cpp d368931 
>   src/widgets/MetaQueryWidget.h c720b6d 
>   src/widgets/MetaQueryWidget.cpp ae07e6b 
> 
> Diff: http://git.reviewboard.kde.org/r/100070/diff
> 
> 
> Testing
> -------
> 
> Generated several advanced playlists testing a couple of the fields and all the conditions.
> 
> 
> Thanks,
> 
> Ralf
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101021/1796b821/attachment-0001.htm