Review Request: Use MetaQueryWidget in the advanced playlist generator.

[Ralf Engels]

> On 2010-11-07 22:07:29, Soren Harward wrote:
> > 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.
> 
> Ralf Engels wrote:
>     First: the repository is at http://gitorious.org/~rengels/amarok/rengelss-amarok/commits/metaquery-patch
>     I would need your Gitorious user name to add you as collaborator. We should have done this before :)
>     
>     The trickery with 30 days is really a trickery. But I have no idea how you could convert months or years to seconds otherwise. We could use the sun year and on 1/12 as month. Actually that is a good idea. 
>     
>     Date default is easy to do. Currently it set's every numeric field (dates included) to 0. 
>     
>     Regarding the CompareDate. You are right. date == date actually means "on the same day".
>

Soren,
I was wondering. 
Is the branch OK now? Should I commit it?
I am for committing it once we are relatively sure that it's better than the current mainline, which it is in my opinion.


- Ralf


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


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/20101120/1e55518d/attachment-0001.htm