Review Request: Use MetaQueryWidget in the advanced playlist generator.

[Soren Harward]

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


Okay, here are my comments on r5:

- Now that the TagMatch class has been pared down quite a bit, I agree with your original idea to get rid of the Support file.  Let's merge the code from the TagMatchComparer.cpp file into TagMatch.cpp.

- The name of a Tag Match constraint that uses "Simple Search" doesn't display nicely in the "Constraint Tree" section of the "APG Preset Editor" dialog.

- The trickery with having 30 days equal 1 month in the MQWidget is too much.  Not every month is 30 days, so just keep it at "X days" and add a further dropdown unit for months.

- The date fields display properly for TagMatch in the "Constraint Tree" section (with the exception of the "months" trickery), but they don't function properly.  Running the APG with a constraint that has a date comparison causes it to behave, well, abnormally.

- When changing the type of match in the MQWidget (eg, from number to string), the comparison in the Constraint name doesn't get updated properly

- The "Match" label should be disabled along with the "Loose -|----- Strict" slider when a string type is selected in the MQWidget.

- The date for date matching on a new constraint should default to something more reasonable than 31-Dec-1969; the best default would probably be "today".

- Removing the CompareDate function from the Comparer is actually a problem; there's a long standing bug with date comparisons that I started to fix, but shelved until after the change to MQWidget.  Comparing dates isn't as easy as just comparing two numbers.  For example "added to collection on [date]" isn't just a straight "equals" comparison, because "on [date]" semantically covers the ~86400 seconds during that day.  So the comparison of "equals" actually has to encompass a range.  So date comparisons should be kept as a separate function so they can be treated properly.

The Widget itself looks great, and I think the idea to restructure the Comparer class to use static functions rather than a const object was a good one.  Once we get these regressions fixed, the patch will be good to go.

The date-handling interaction between the MQWidget and the TagMatch constraint is likely to be a bit complicated.  Could you please push this branch to a repo so we can work on it together?  It would also give me the chance to fix the CompareDate function.

- Soren


On 2010-11-07 19:06:34, Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100070/
> -----------------------------------------------------------
> 
> (Updated 2010-11-07 19:06:34)
> 
> 
> 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 2160670 
>   src/core-impl/collections/support/XmlQueryReader.h 0088608 
>   src/core-impl/collections/support/XmlQueryReader.cpp b5518f0 
>   src/core-impl/collections/support/XmlQueryWriter.h 5caf7bc 
>   src/core-impl/collections/support/XmlQueryWriter.cpp 3c9cf41 
>   src/core/CMakeLists.txt 8272804 
>   src/core/meta/support/MetaConstants.h 73d03fc 
>   src/core/meta/support/MetaConstants.cpp PRE-CREATION 
>   src/core/meta/support/MetaUtility.h 47e7c24 
>   src/core/meta/support/MetaUtility.cpp 5cf4519 
>   src/dynamic/Bias.h 082327c 
>   src/dynamic/Bias.cpp 8a8ab97 
>   src/playlistgenerator/PresetModel.cpp 9b74636 
>   src/playlistgenerator/constraints/PlaylistDuration.h 992215c 
>   src/playlistgenerator/constraints/PlaylistDuration.cpp 5d83dfe 
>   src/playlistgenerator/constraints/TagMatch.h f23d562 
>   src/playlistgenerator/constraints/TagMatch.cpp d8ed7db 
>   src/playlistgenerator/constraints/TagMatchComparer.cpp PRE-CREATION 
>   src/playlistgenerator/constraints/TagMatchEditWidget.ui b9d97c6 
>   src/playlistgenerator/constraints/TagMatchSupport.cpp 6b777b1 
>   src/widgets/MetaQueryWidget.h ba64e4c 
>   src/widgets/MetaQueryWidget.cpp 32f3f24 
> 
> 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/20101107/c73c1789/attachment.htm