Review Request: Use MetaQueryWidget in the advanced playlist generator.

Soren Harward stharward at gmail.com
Thu Oct 28 05:13:26 CEST 2010



> 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.
>     
>     
>
> 
> Ralf Engels wrote:
>     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.
> 
> Soren Harward wrote:
>     You're right in that the Comparer does not *have* to be its own class.  But from a code design standpoint, I had it as a separate class for a few reasons:
>     
>     #1 - The TagMatch class is already quite complicated at well over 800 LOC without including the comparison functions.  Splitting the comparison functions out into a nested class is a rational way of encapsulating functionality that is used by, but not tightly coupled with, the rest of the constraint.
>     #2 - Nested classes are a design pattern I've used through other constraints.  While there were both the TagMatchFieldsModel and the Comparer, it made sense to have a separate file, but it might not anymore, but on the other hand we may as well keep it because ...
>     #3 ... I'm not finished with the Comparer class.  I am still working on including regex and fuzzy string matching functionality, both of which will make the Comparer class significantly more complicated.
>     
>     Removing the Comparer class does not improve the code; at best it breaks even with functionality, and as it stands right now, it's worse from a code design standpoint because the current patch removes a useful layer of encapsulation and replaces it with some globally-declared functions that have no business being declared globally.
>     
>     Whether you agree with these reasons or not, there's one reason that (from my standpoint) trumps all others, and it basically boils down to "because I said so".  I developed the APG, and for the foreseeable future, I'm the one that's going to have to maintain it.  Removing the Comparer class changes the code without conferring any clear benefit to either the program itself or the design of the code, which is why I have requested that it not be removed.
> 
> Ralf Engels wrote:
>     Hi Soren,
>     
>     1. The MetaQueryWidget allowed me to remove a couple of lines. I haven't counted it but I think it's ok.
>     2. I had a quick look over the playlist generator directory. While nested classes seems to be a pattern you use frequently a seperate MatchingConstraint class is only present in the tag matcher.
>     3. Ok. That is a reason.
>     4. That is probably a personal opinion, but static functions are far from global. A slight problem with them is that they look like C and some people don't like that. If you have a private class then usually you just put them there.
>     5. "because I say so" is not a reason. You having further plans with the code and maintaining it is one though.
>     
>     Here is a proposal from me:
>     Make it a stand-alone class (not nested). 
>     Reason: I would expect fuzzy compare functions to be usefull also in other constraints.
>     I just looked at the PlaylistLength and PlaylistDuration constraint and they have quite similar functions.
> 
> Ralf Engels wrote:
>     Small addition: 
>     "Make it a stand-alone class" means: I make it a stand alone class.
>     
>     Also I will not commit or push anything unless you are satisfied with it.

The Comparer should not be a standalone class.  For the reason why, look at the comparison functions in the PlaylistLength and PlaylistDuration classes again.  Even though they're superficially similar to the comparison functions in the TagMatch class, the mathematics are substantially different.  The fuzzy comparisons are functions that I've spent a lot of time tweaking because they have a big effect on how "good" the results from the APG are.  Each fuzzy comparison function is tailored to its Constraint, and thus should not be used outside its Constraint.  At the very least, the Comparison functions should be private to the TagMatch class.  And for the encapsulation reasons I gave above, I prefer to have them be a nested class within TagMatch, rather than a collection of private functions.

I know that "because I said so" is a dissatisfying reason for accepting one design decision over another.  But the fact is that many design decisions regarding Amarok (and any other sufficiently mature Open Source project, for that matter), from the formatting of the source code to the design of the UI, have been made based on the fiat of the authors and maintainers of this project and its components.  Being part of an Open Source project means not just dealing with the code, but with the people who wrote the code.  And this is one of those cases where the latter is unfortunately just as important as the former.

I'm sorry if by my criticism and stubbornness I've made you feel like I'm pushing you around.  It's not directed at you personally.  If Mark or Leo or any of the other authors had submitted this patch, I'd have said exactly the same thing to them.


- Soren


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


On 2010-10-22 00:20:31, Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100070/
> -----------------------------------------------------------
> 
> (Updated 2010-10-22 00:20: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 45a9493 
>   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 082327c 
>   src/dynamic/Bias.cpp 6934a70 
>   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 ba64e4c 
>   src/widgets/MetaQueryWidget.cpp 5316ec6 
> 
> 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/20101028/836d7fbc/attachment-0001.htm 


More information about the Amarok-devel mailing list