----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
