jsonrpc: add FooLibrary.SetFooDetails and VideoLibrary.RemoveFoo #836

Merged
merged 2 commits into from May 4, 2012

2 participants

@Montellese
Team Kodi member

These commits add the probably most requested methods to JSON-RPC and allow a somewhat controlled way of changing stuff in XBMC's databases i.e. without raw SQL queries etc. as a lot of clients still do through our HTTP API.

There are (at least) two "ugly" things in this PR:

  • There are no RemoveFoo methods for the AudioLibrary because we don't have any code that does this in CMusicDatabase (if I got some time I might add it but I'm not as familiar with CMusicDatabase as I am with CVideoDatabase).
  • VideoLibrary.SetFooDetails needs to manually call SetPlayCount() in case the user sets a new playcount and or lastplayed value. All the other properties can be updated through CVideoDatabase::SetDetailsForFoo()

Getting this in will be a huge step towards getting rid of the deprecated HTTP API. As far as I can tell the only missing other feature is library filter (for which I will present a PR soon (sometime in mid/end of april) as well).

@jmarshallnz
Team Kodi member

Does it make sense to think about a more content-agnostic naming for these, or are we too far down that road (just thinking about things other than movie/show/musicvideo/episode) to change now? We can ofcourse always do a more capable one later once other categories are available.

@jmarshallnz
Team Kodi member

Looks like 2fca2f5 and f422e79 could do with a quick rebase to minimize the cosmetic move?

c14ef62 could be gotten rid of later if/when SetDetails*() does an update query. Can be done later no problem though.

7c36585 could probably have the cosmetics move rebased out as well?

Regarding music library not allowing for removal - reason is that it's done on the theory that everything is in the song tags anyway, so a rescan (which would not be triggered initially, but would be as soon as something is added to the folder) would see it come straight back again - there's no excluded list here. The music scanner is actually more intelligent than the videoscanner with respect to how it handles changes (no separate cleaning required to remove stuff, changes to the files will be picked up on scan etc.)

I bet you're looking forward to marking HTTP-API as officially depreciated :)

@Montellese
Team Kodi member

Well concerning a more content-agnostic approach: Sure it makes sense but the advantage of this approach (apart from the fact that all the getter methods are named VideoLibrary.GetFooDetails etc) is that you can define which properties can be set/changed for which media type. So a user shouldn't be able to update "showtitle" for a movie because it doesn't make sense. Sure nothing would happen if the user would provide such a property because it isn't used in the SetDetailsForMovie() method but the way it is now the client will receive an error message telling him that "showtitle" can not be used for SetMovieDetails().

But I agree that (if we ever get a more modern database layout) it would make sense to consider a move to a more content-agnostic API.

I can squash the VideoLibrary.RemoveFoo() and VideoLibrary.SetFooDetails commit, no problem.
Yup the SetPlayCount is rather ugly but no way around it right now.
Not sure what you mean about 7c36585

I'll answer to your inline comments ASAP. Thanks for the review.

@jmarshallnz
Team Kodi member

Agreed regarding content agnostic - given it's not really like that now, there's no point pretending it is from the API perspective (particularly as we already have the Get routines for each type).

What I meant about VideoLibrary.RemoveFoo() and VideoLibrary.SetFooDetails() was that the second one shifted the newly added methods of the first one about. If you edit the second commit and break it into new stuff + shifted stuff, the shifted stuff can probably then be squashed into the first, leaving 2 clean commits. No big deal - just something I noticed.

For the songs, it looks like you're wanting to alter the information in the songs table? If so, you really want to go straight to the song table itself, rather than filtering everything through the vector of CSong's passed in GetAlbumInfo() which might not be related to actual songs in the user's collection at all - eg the user may have just 1 song from the album, but some small info (track number, track title, not sure what else) about all tracks from the album is downloaded from allmusic.com etc. when the album info is downloaded. It wasn't clear to me which one you were wanting to change.

@Montellese
Team Kodi member

OK I fixed the things that jmarshallnz commented on especially added an idFoo parameter to the original CVideoDatabase::DeleteFoo methods to avoid an extra query for the ID based on the path (because we already know the ID) and squashed the videolibrary related commits together so that the diff looks much cleaner.

@Montellese
Team Kodi member

@jmarshallnz concerning updating a song. If I understand you correctly the way it is currently done in AudioLibrary.SetSongDetails is far from ideal. Should I add a new method to CMusicDatabase which allows to update a single song based on it's path or database ID? That would also reduce the work needed to be done in the JSON-RPC implementation quite a bit because currently I have to retrieve the whole album + details, then look for the right song, change that song's data and then send everything back to the database.

Apart from that is there anything else that needs changing? Might be best to hold off on this one till #884 has been merged so I can update that in VideoLibrary.SetMusicVideoDetails to an array as well.

@jmarshallnz
Team Kodi member

Yeah, adding to CMusicDatabase to set details for a song based on database ID sounds good to me.

Once done I can review again if you like, but IIRC the rest looked pretty good.

@Montellese
Team Kodi member

OK I will add the respective method to CMusicDatabase and will report back once done to get it reviewed.

@Montellese Montellese was assigned Apr 16, 2012
@Montellese
Team Kodi member

OK after taking another look at this I pushed a new commit which adds a new method UpdateSong to CMusicDatabase. The main problem is that there's no method to remove a single song from the database, so I had to copy & paste code from RemoveSongsByPath (which removes all songs from a certain directory and not just one) and adjust it a bit to work with a single song's ID. After deleting all the info of the song it uses AddSong to re-add the song to the database. I tested it on 3 different songs and it looks ok but I'm not 100% comfortable with this.

I furthermore noticed that I will have to change all the SetFooDetails methods to return the new ID of the media item that was changed because all those implementations first remove the existing item from the database and then re-add it so the client will need to know about the new ID of hte item. I'll do that tomorrow/later after I got some sleep.

@jmarshallnz
Team Kodi member

I'd be tempted to switch it to delete from the link tables but not the song table, then modify addsong as needed to do an update. A shitload more work, but it means you don't need to return a new id, which IMO is counter-intuitive.

One potential way to modify addsong would be to on first add just add a blank row (so you get the id) then do an update query. If id is already known, we just use the update query. This is how the videodb does it. It's a little slower at insert (2 queries) but we don't insert much anyway, and when we do it's invariably under a transaction.

The videodb update methods should be a little easier to convert so they re-use the same ID I think?

@Montellese
Team Kodi member

I totally agree that the current behaviour/implementation is counter-intuitive. What I'm unsure about is whether it's possible to manually write a value into a field in the database which is marked as auto_increment. Kind of defies the purpose of that doesn't it?

I'll have to search the web to see if it's possible in the first place otherwise I won't even have to consider re-writing that code.

@jmarshallnz
Team Kodi member

I don't think you have to? All you need to do is change AddSong() to pass the existing songID in.

If the songID is not given (existing AddSong implementation) you do an INSERT query and just add a blank row (and assign the resulting rowid to songID).

Then you just execute an UPDATE query on the songID row.

@Montellese
Team Kodi member

Ah I was thinking along the line of not having to convert all the INSERT queries to UPDATE queries ;-) So you would delete the row but remember the ID and then do the already existing INSERT query with the remembered ID (if there is one). I just tested it both with MySQL and SQLite and it is possible to manually set the ID of a field that is marekd as auto_increment (and primary key). But I guess your approach would be cleaner ;-) I just hate to touch all those SQL queries as it's bound to break something.

@jmarshallnz
Team Kodi member

How about:

INSERT OR REPLACE ?

MySQL might be just REPLACE based on the sqlite docs: http://www.sqlite.org/lang_insert.html

REPLACE INTO foo (idFoo, blah...) VALUES(existing_id, blah...) works in sqlite3 no problem.

@Montellese
Team Kodi member

That actually sounds like a nice idea. I didn't know about REPLACE at all but it sounds like it could solve this problem. I'll give it a whirl later on.

@Montellese
Team Kodi member

OK I updated the last commit to use "replace" instead of "insert" in CMusicDatabase::AddSong and to use the song's ID instead of NULL in the "replace" query if it is known.

I tested it on SQLite with the music scanner and through JSON-RPC and didn't notice any problems.

While looking through CVideoDatabase I made two changes to SetDetailsFor(Movie|Episode|MusicVideo): one will save us an extra query for idFoo based on strFileName and the second one is actually a fix because otherwise we ended up with two movie/episode/musicvideo records in the database with more or less the same details but different IDs.

@jmarshallnz
Team Kodi member

Looks good to me - nice work!

@Montellese
Team Kodi member

@jmarshallnz Thanks for reviewing and for your feedback/help. I have squashed the two new commits into the other two and I'll add this to the May merge window.

@Montellese Montellese merged commit 0bd7924 into xbmc:master May 4, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment