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