Review Request: Use MetaQueryWidget in the advanced playlist generator.

[Ralf Engels]

> On 2010-10-18 16:45:28, Maximilian Kossick wrote:
> > src/core/meta/support/MetaConstants.cpp, line 20
> > <http://git.reviewboard.kde.org/r/100070/diff/1/?file=1066#file1066line20>
> >
> >     These mappings are bound to get out of sync. How about using K_GLOBAL_STATIC to create an instance of a simple class which would contain these mappings. The actual mappings can be set up in the class' constructor. There might be a KDE class ("KBiAssociativeContainer") which allows efficient bi-directional mappings (it seems to be in libkdepim for some reason though)

The backwards mapping (QString to qint64) is only used at very few places none is time critical.
The forward mapping is quite performat with the switch.

I agree that you have to take care to update the functions after adding a field.
It's still better than the current solution where you have at least three places to update (down from 6 from before my MetaQueryWidget).


> On 2010-10-18 16:45:28, Maximilian Kossick wrote:
> > src/core/meta/support/MetaConstants.cpp, line 97
> > <http://git.reviewboard.kde.org/r/100070/diff/1/?file=1066#file1066line97>
> >
> >     A slightly larger task would be replacing qint64 with a proper enum. In Amarok1, multiple field values where sometimes OR'd. I do not think we still do this in A2. That way the compiler could even tell you if an enum value is missing in a switch statement.

Good idea.
That is something for the next patch.


- Ralf


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


On 2010-10-18 12:07:31, Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100070/
> -----------------------------------------------------------
> 
> (Updated 2010-10-18 12:07: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 abdcee8 
>   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 6ec60b6 
>   src/dynamic/Bias.cpp dec2db2 
>   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 c720b6d 
>   src/widgets/MetaQueryWidget.cpp ae07e6b 
> 
> 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/20101018/d7788798/attachment-0001.htm