[GSoC] artist-based smartplaylists #1249

Merged
merged 2 commits into from Aug 10, 2012

Conversation

Projects
None yet
3 participants
@Montellese
Member

Montellese commented Aug 6, 2012

This adds very basic support for artist-based smartplaylists. There are only very few possible rules i.e. artist, genre, moods, styles, born, bandformed, disbanded, died, biography, instruments and a compilation artist switch (not sure if this one makes sense I never really understood how that works). These rules could be extended if necessary.

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Aug 6, 2012

Member

I extracted the changes to SmartPlaylist.cpp which were actually fixes for existing bugs (which nobody noticed so far it seems) and already pushed them to master.

@jmarshallnz or @night199uk could one of you shed some light on the implications of enabling/disabling "include artists who appear only on compilations"? Does it change the way the scraper works (i.e. it ignores those artists if enabled) or does it only change the way the SQL queries in CMusicDatabase::GetArtistsNav work (with the albumArtistsOnly parameter)? If the former holds true the "compilation artists" rule I added does not make any sense otherwise IMO it does.

Member

Montellese commented Aug 6, 2012

I extracted the changes to SmartPlaylist.cpp which were actually fixes for existing bugs (which nobody noticed so far it seems) and already pushed them to master.

@jmarshallnz or @night199uk could one of you shed some light on the implications of enabling/disabling "include artists who appear only on compilations"? Does it change the way the scraper works (i.e. it ignores those artists if enabled) or does it only change the way the SQL queries in CMusicDatabase::GetArtistsNav work (with the albumArtistsOnly parameter)? If the former holds true the "compilation artists" rule I added does not make any sense otherwise IMO it does.

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Aug 6, 2012

Member

The advanced setting effects only the SQL queries. IIRC we used to scan
based on the full artist string. This has since changed to scan all
artists, so it may well be that it might make sense to make the setting
apply to scraping as well (to speed things up?) - not sure.

On Tue, Aug 7, 2012 at 7:56 AM, Sascha Montellese
notifications@github.comwrote:

I extracted the changes to SmartPlaylist.cpp which were actually fixes for
existing bugs (which nobody noticed so far it seems) and already pushed
them to master.

@jmarshallnz https://github.com/jmarshallnz or @night199ukhttps://github.com/night199ukcould one of you shed some light on the implications of enabling/disabling
"include artists who appear only on compilations"? Does it change the way
the scraper works (i.e. it ignores those artists if enabled) or does it
only change the way the SQL queries in CMusicDatabase::GetArtistsNav work
(with the albumArtistsOnly parameter)? If the former holds true the
"compilation artists" rule I added does not make any sense otherwise IMO it
does.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1249#issuecomment-7533863.

Member

jmarshallnz commented Aug 6, 2012

The advanced setting effects only the SQL queries. IIRC we used to scan
based on the full artist string. This has since changed to scan all
artists, so it may well be that it might make sense to make the setting
apply to scraping as well (to speed things up?) - not sure.

On Tue, Aug 7, 2012 at 7:56 AM, Sascha Montellese
notifications@github.comwrote:

I extracted the changes to SmartPlaylist.cpp which were actually fixes for
existing bugs (which nobody noticed so far it seems) and already pushed
them to master.

@jmarshallnz https://github.com/jmarshallnz or @night199ukhttps://github.com/night199ukcould one of you shed some light on the implications of enabling/disabling
"include artists who appear only on compilations"? Does it change the way
the scraper works (i.e. it ignores those artists if enabled) or does it
only change the way the SQL queries in CMusicDatabase::GetArtistsNav work
(with the albumArtistsOnly parameter)? If the former holds true the
"compilation artists" rule I added does not make any sense otherwise IMO it
does.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1249#issuecomment-7533863.

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Aug 7, 2012

Member

It's not an advanced setting it's a GUI setting in Settings -> Music -> Library. I always keep it disabled to get a clean list of artists but if I understood @night199uk right he plans to improve scraping/organization of all those "Foo feat. Bar" so it might make sense to just keep it on the SQL level i.e. grab the information for all the artists but in Music -> Artists only show the album artists. Then maybe add a new node Music -> Compilation artists where only those artists are shown.

Thinking about it I'll remove that rule/filter because I have neither the understanding for it nor the items/database to actually test it.

Member

Montellese commented Aug 7, 2012

It's not an advanced setting it's a GUI setting in Settings -> Music -> Library. I always keep it disabled to get a clean list of artists but if I understood @night199uk right he plans to improve scraping/organization of all those "Foo feat. Bar" so it might make sense to just keep it on the SQL level i.e. grab the information for all the artists but in Music -> Artists only show the album artists. Then maybe add a new node Music -> Compilation artists where only those artists are shown.

Thinking about it I'll remove that rule/filter because I have neither the understanding for it nor the items/database to actually test it.

@night199uk

This comment has been minimized.

Show comment
Hide comment
@night199uk

night199uk Aug 7, 2012

Contributor

Yeah. I'm out of town right now so I can't look at the code but basically
it just changes the query in GetArtistsNav to include artists on albums
that have the 'bCompilation' flag set or not. The changes I will make will
all be on the front side of the scanner, I.e. changing the way artists are
created in the database. SmartPlaylists are back side (I.e. querying
artists from ye database) so shouldn't need to be any changes for that.

I can take a look at this more deeply when I'm back on the 17th.

Sent from my iPhone

On 7 Aug 2012, at 03:11, Sascha Montellese notifications@github.com wrote:

It's not an advanced setting it's a GUI setting in Settings -> Music ->
Library. I always keep it disabled to get a clean list of artists but if I
understood @night199uk https://github.com/night199uk right he plans to
improve scraping/organization of all those "Foo feat. Bar" so it might make
sense to just keep it on the SQL level i.e. grab the information for all
the artists but in Music -> Artists only show the album artists. Then maybe
add a new node Music -> Compilation artists where only those artists are
shown.

Thinking about it I'll remove that rule/filter because I have neither the
understanding for it nor the items/database to actually test it.


Reply to this email directly or view it on
GitHubhttps://github.com/xbmc/xbmc/pull/1249#issuecomment-7545462.

Contributor

night199uk commented Aug 7, 2012

Yeah. I'm out of town right now so I can't look at the code but basically
it just changes the query in GetArtistsNav to include artists on albums
that have the 'bCompilation' flag set or not. The changes I will make will
all be on the front side of the scanner, I.e. changing the way artists are
created in the database. SmartPlaylists are back side (I.e. querying
artists from ye database) so shouldn't need to be any changes for that.

I can take a look at this more deeply when I'm back on the 17th.

Sent from my iPhone

On 7 Aug 2012, at 03:11, Sascha Montellese notifications@github.com wrote:

It's not an advanced setting it's a GUI setting in Settings -> Music ->
Library. I always keep it disabled to get a clean list of artists but if I
understood @night199uk https://github.com/night199uk right he plans to
improve scraping/organization of all those "Foo feat. Bar" so it might make
sense to just keep it on the SQL level i.e. grab the information for all
the artists but in Music -> Artists only show the album artists. Then maybe
add a new node Music -> Compilation artists where only those artists are
shown.

Thinking about it I'll remove that rule/filter because I have neither the
understanding for it nor the items/database to actually test it.


Reply to this email directly or view it on
GitHubhttps://github.com/xbmc/xbmc/pull/1249#issuecomment-7545462.

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Aug 7, 2012

Member

@night199uk: I've removed that part for now because I can't test it anyway. I was also confused by boolFeatured and bCompilation so I can still add it back once your musicdatabase changes/refactor have been merged as well.

Member

Montellese commented Aug 7, 2012

@night199uk: I've removed that part for now because I can't test it anyway. I was also confused by boolFeatured and bCompilation so I can still add it back once your musicdatabase changes/refactor have been merged as well.

@ghost ghost assigned Montellese Aug 8, 2012

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Aug 8, 2012

Member

@cptspiff Thanks for the review and for spotting that oopsie. I fixed the issues you pointed out.

Member

Montellese commented Aug 8, 2012

@cptspiff Thanks for the review and for spotting that oopsie. I fixed the issues you pointed out.

Montellese added a commit that referenced this pull request Aug 10, 2012

Merge pull request #1249 from Montellese/gsoc_xsp_artists
[GSoC] artist-based smartplaylists

@Montellese Montellese merged commit 5817696 into xbmc:master Aug 10, 2012

@Montellese Montellese deleted the Montellese:gsoc_xsp_artists branch Apr 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment