Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting album user rating using album ID not GetAlbumIdByPath #10553

Merged
merged 1 commit into from Sep 30, 2016

Conversation

@DaveTBlake
Copy link
Member

commented Sep 24, 2016

Fixes a bug when setting album user rating when the user has songs from more than one album in the same folder.

There is nothing to say that all albums must have a unique folder that does not contain songs from any other album, even if this is the common way to organise your music files. Path may not be sufficient to identify an album uniquely. Hence GetAlbumIdByPath needs to allow for more than one album being found for a path, just as GetAlbumByName and GetArtistByName do, rather than return the ID of the first in the results set.

Setting album user rating was using path to attempt to identify the album when the unique album ID is readily available. Switch to use ID not path.

Also SetAlbumVotes was removed as it was unused. The idea seems to be that "rating" and "votes" are values loaded from scraping NFO or online sources, and are never set via the GUI, hence this routine was unnecessary as votes are never set in isolation ( and there is no routine to set rating). Rating and votes are loaded by CAlbum::Load.

@Razzeee perhaps you would like to comment as you did the original work on this in #8405
@zag2me most likely to have data to test this out.

…a folder can contain songs from more than one album
@DaveTBlake DaveTBlake force-pushed the DaveTBlake:ManyAlbumsInFolder branch from 6581ebc to 0fc46e8 Sep 25, 2016
@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2016

To test this fix put music files from more than one album all in the same folder. Add folder as new music source and scan into music library. Edit the user rating for the different albums from the Album Info dialog, and see them being correctly applied when you look at the albums node (sorted by "my rating").

Without this fix when you change the user rating for an album once you leave the Album Info dialog then actual edit is always applied to the first album.

@Razzeee

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

LGTM

Have you tried if votes can still be set via json?

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2016

Votes, for songs nor albums, have never been added as something that can be set from JSON

@Razzeee

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

Sorry, I always forget how scrapping is done. So I didn't ment json, but
reading from nfo.

On Tue, Sep 27, 2016, 11:37 Dave Blake notifications@github.com wrote:

Votes, for songs nor albums, have never been added as something that can
be set from JSON


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10553 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFqyZEhl5pBBfccC5uDCvB-g_07Fvjfnks5quOPNgaJpZM4KFtKS
.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2016

Yes, reading of votes, rating and userrating from NFO is fine. All done within CAlbum::load, SetAlbumVotes is not called anywhere. But, as I mentioned on Slack, album rating is limited to 5. I will fix that

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2016

Probably a Feature Request... but setting the album and song rating via JSON would be very useful to me to cloud backup my user ratings.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2016

PR #10570 to fix the album rating range from being trucated at 5 when loaded.

@zag2me you can set rating and user rating for both songs and albums via JSON, just not votes. But if votes were to be added by another PR it still would not need the SetAlbumVotes routine I removed.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2016

jenkins build this please

1 similar comment
@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

jenkins build this please

@DaveTBlake DaveTBlake merged commit 4e1c581 into xbmc:master Sep 30, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@DaveTBlake DaveTBlake added this to the Krypton 17.0-beta3 milestone Sep 30, 2016
@DaveTBlake DaveTBlake deleted the DaveTBlake:ManyAlbumsInFolder branch Sep 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.