[Amarok] aa4c4f5: Enable filtering by labels for the sql collections

[Maximilian Kossick]

Querying for labels ... here we go again.

Unless I'm very mistaken this approach does not work correctly. Due to
the m:n relation of tracks to labels this will not work when querying
for multiple labels, e.g. it will deliver incorrect results when
querying for "label:A AND label:B" or various other combinations. And
I should now, given that I had to deal with the bug reports in the A1
days. In my opionion we will need various subqueries to make it work
correctly, which was not possible at all with sqlite.

Personally I vote for reverting this feature because it is incomplete
and doing it properly in a future release instead of releasing it now.
Other opinions?

Oh, and why is there no unit test for a new feature in SqlCollection?
the test infrastructure is all there, so it's really easy to add that.
Can we start using a TDD approach please?

Cheers,
Max


On Thu, Feb 25, 2010 at 9:27 AM, Nikolaj Hald Nielsen <nhn at kde.org> wrote:
> commit aa4c4f5400b49ae7f5430f07a91a559c9292e25b
> Author: Nikolaj Hald Nielsen <nhn at kde.org>
> Date:   Thu Feb 25 09:19:53 2010 +0100
>
>    Enable filtering by labels for the sql collections.
>
>    Labels are not used in default filters as there is quite a bit of sql table joining goin on, but is used by adding the "label" keyword, i.e. label:"awesome tracks"
>
> diff --git a/src/browsers/CollectionTreeItemModelBase.cpp b/src/browsers/CollectionTreeItemModelBase.cpp
> index 0adc020..4181733 100644
> --- a/src/browsers/CollectionTreeItemModelBase.cpp
> +++ b/src/browsers/CollectionTreeItemModelBase.cpp
> @@ -608,6 +608,10 @@ CollectionTreeItemModelBase::addFilters( QueryMaker * qm ) const
>                 {
>                     ADD_OR_EXCLUDE_NUMBER_FILTER( Meta::valTrackNr, elem.text.toInt(), compare );
>                 }
> +                else if( lcField.compare( "label", Qt::CaseInsensitive ) == 0 /*|| lcField.compare( i18n( "label" ), Qt::CaseInsensitive ) == 0 */)
> +                {
> +                    ADD_OR_EXCLUDE_FILTER( Meta::valLabel, elem.text, false, false );
> +                }
>                 else if( lcField.compare( "added", Qt::CaseInsensitive ) == 0 || lcField.compare( i18n( "added" ), Qt::CaseInsensitive ) == 0 )
>                 {
>                     if( compare == QueryMaker::Equals ) // just do some basic string matching
> diff --git a/src/collection/sqlcollection/SqlQueryMaker.cpp b/src/collection/sqlcollection/SqlQueryMaker.cpp
> index 347f9d9..268add6 100644
> --- a/src/collection/sqlcollection/SqlQueryMaker.cpp
> +++ b/src/collection/sqlcollection/SqlQueryMaker.cpp
> @@ -73,7 +73,7 @@ class SqlWorkerThread : public ThreadWeaver::Job
>
>  struct SqlQueryMaker::Private
>  {
> -    enum { TAGS_TAB = 1, ARTIST_TAB = 2, ALBUM_TAB = 4, GENRE_TAB = 8, COMPOSER_TAB = 16, YEAR_TAB = 32, STATISTICS_TAB = 64, URLS_TAB = 128, ALBUMARTIST_TAB = 256 };
> +    enum { TAGS_TAB = 1, ARTIST_TAB = 2, ALBUM_TAB = 4, GENRE_TAB = 8, COMPOSER_TAB = 16, YEAR_TAB = 32, STATISTICS_TAB = 64, URLS_TAB = 128, ALBUMARTIST_TAB = 256, LABELS_TAB = 512, URLS_LABELS_TAB = 1024 };
>     int linkedTables;
>     QueryMaker::QueryType queryType;
>     QString query;
> @@ -730,6 +730,19 @@ SqlQueryMaker::linkTables()
>             d->queryFrom += " LEFT JOIN statistics ON tracks.url = statistics.url";
>         }
>     }
> +    if( d->linkedTables & Private::LABELS_TAB && d->linkedTables & Private::URLS_LABELS_TAB )
> +    {
> +        //these 2 tables should always be linked together, having only one of them makes no sense
> +        if( d->linkedTables & Private::URLS_TAB )
> +        {
> +            d->queryFrom += " LEFT JOIN urls_labels ON urls.id = urls_labels.url";
> +        }
> +        else
> +        {
> +            d->queryFrom += " LEFT JOIN urls_labels ON tracks.url = urls_labels.url";
> +        }
> +        d->queryFrom += " LEFT JOIN labels ON urls_labels.label = labels.id";
> +    }
>  }
>
>  void
> @@ -953,6 +966,10 @@ SqlQueryMaker::nameForValue( qint64 value )
>             //so add albums as well
>             d->linkedTables |= Private::ALBUM_TAB;
>             return "albumartists.name";
> +        case valLabel:
> +            d->linkedTables |= Private::LABELS_TAB;
> +            d->linkedTables |= Private::URLS_LABELS_TAB;
> +            return "labels.label";
>         default:
>             return "ERROR: unknown value in SqlQueryMaker::nameForValue(qint64): value=" + value;
>     }
> diff --git a/src/meta/MetaConstants.h b/src/meta/MetaConstants.h
> index b17a76e..c5435ff 100644
> --- a/src/meta/MetaConstants.h
> +++ b/src/meta/MetaConstants.h
> @@ -55,6 +55,9 @@ namespace Meta
>
>     static const qint64 valAlbumArtist  = 1LL << 27;
>
> +    //custom label support
> +    static const qint64 valLabel        = 1LL << 28;
> +
>     namespace Field
>     {
>         //actual string values are not final yet
>