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.
> 
> Ralf Engels wrote:
>     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.
>     
>     
>

I was thinking about the labels stuff.
I remembered that this didn't work before. I was just checking and I am not so sure any more.
Was this working?
I am not even sure if you can use a QueryMaker to search for labels.
I will have a look at it again anyhow.


- 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/d5812c76/attachment.html