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

[python] add dbid to musicinfotag #10858

Merged
merged 2 commits into from Jan 4, 2017

Conversation

Projects
None yet
4 participants
@ronie
Copy link
Member

commented Nov 4, 2016

allow python addons to get/set the dbid of a (music) listitem.

added logging for invalid videoinfo tags (we already do this for musicinfo).

@ronie ronie force-pushed the ronie:python-musicinfotag-dbid branch from 41fca33 to c570ae3 Nov 5, 2016

///
///
///-----------------------------------------------------------------------
///

This comment has been minimized.

Copy link
@phil65

phil65 Nov 5, 2016

Member

version note is missing.

This comment has been minimized.

Copy link
@phil65

phil65 Nov 5, 2016

Member

I think this returns -1 in case there is no dbid? might be worth adding a comment for that. (in case that info is correct)

This comment has been minimized.

Copy link
@ronie

ronie Nov 5, 2016

Author Member

wish that was all :-)
still working on it....

This comment has been minimized.

Copy link
@phil65

phil65 Nov 5, 2016

Member

ok. 👍

@ronie ronie force-pushed the ronie:python-musicinfotag-dbid branch 3 times, most recently from a8f6266 to b511724 Nov 5, 2016

@ronie

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2016

added dbid to setInfo() as well.
it's a bit cumberstone as you also need to pass the mediatype.

@@ -479,6 +479,20 @@ namespace XBMCAddon
}
else if (strcmpi(type, "music") == 0)
{
std::string type;
for (InfoLabelDict::const_iterator it = infoLabels.begin(); it != infoLabels.end(); ++it)

This comment has been minimized.

Copy link
@phil65

phil65 Nov 5, 2016

Member

auto + range based for loop

const InfoLabelValue& alt = it->second;
const String value(alt.which() == first ? alt.former() : emptyString);

if (key == "mediatype")

This comment has been minimized.

Copy link
@phil65

phil65 Nov 5, 2016

Member

you can combine these two checks with AND.

if (key == "mediatype")
{
if (CMediaTypes::IsValidMediaType(value))
type = value;

This comment has been minimized.

Copy link
@phil65

phil65 Nov 5, 2016

Member

You can also already set the mediatype here (setType), that way you can remove it from below.

This comment has been minimized.

Copy link
@ronie

ronie Nov 5, 2016

Author Member

good thinking, thx!

@ronie ronie force-pushed the ronie:python-musicinfotag-dbid branch from b511724 to e72770b Nov 5, 2016

@ronie

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2016

updated

@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta6 milestone Nov 6, 2016

@tamland

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

I'm realizing it was a big mistake exposing dbid as int in the api as this will most likely break with the future database refactor. To be future proof it should be returned as a string.

@@ -488,7 +509,9 @@ namespace XBMCAddon
const String value(alt.which() == first ? alt.former() : emptyString);

//! @todo add the rest of the infolabels
if (key == "tracknumber")
if (key == "dbid" and !type.empty())

This comment has been minimized.

Copy link
@tamland

tamland Nov 6, 2016

Member

that's.. not c++

This comment has been minimized.

Copy link
@ronie

ronie Nov 6, 2016

Author Member

lol, fixed :-)

@ronie ronie force-pushed the ronie:python-musicinfotag-dbid branch from e72770b to 6ded0ff Nov 6, 2016

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

So if you want to switch it to be a string we'd better do it now before we hit RC (same for video?)

@ronie

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2016

no idea.. json returns dbid's as an int as well, is that also a problem?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

Only if the new db is going to return something else as an int somewhere in the future

@ronie

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2016

i'd say let's address all that when we get there.
'till then we should keep things unified between interfaces imo.

@tamland

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

For this PR, at least remove the whole '-1' documentation as api users have no business knowing about/relying on this implementation detail.

Depending on how much is already use this, it might be worth fixing now rather than later when more addons are using it. The main issue here is that the id type is a such a low level internal thing of the database. A slight api misuse and you have coupled an addon to the current database.

@ronie

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2016

ok, i'll drop the '-1' comment.

as for converting it to a string, i'm afraid i don't understand the problem.
in my simple logic dbid is an int, same like year is an int, so we return it as an int.

if we return it as a string, i can convert it back to an int in my python code, then what have we gained?

ping @phil65 since your addons are the ones that are using it currently.

@phil65

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

Will adapt in case we change behaviour.

@ronie ronie force-pushed the ronie:python-musicinfotag-dbid branch from 6ded0ff to 081824b Nov 6, 2016

@tamland

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

as for converting it to a string, i'm afraid i don't understand the problem.
in my simple logic dbid is an int, same like year is an int, so we return it as an int.

if we return it as a string, i can convert it back to an int in my python code, then what have we gained?

The point is: dbids are opaque identifiers, not ints. they just happens to currently be represented by ints. Using a string is just one way to make it very explicit that it is opaque. If you don't treat it like that and do things with it, that's your own doing. But just go ahead with this. I can make a pr to 'clarify' this api.

@ronie

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2016

thanx for the feedback.
i'm in no rush to get this in.

will get over it again when the time comes to merge this.

@MartijnKaijser MartijnKaijser removed this from the Krypton 17.0-beta6 milestone Nov 30, 2016

@MartijnKaijser MartijnKaijser removed this from the Krypton 17.0-beta6 milestone Nov 30, 2016

@ronie ronie force-pushed the ronie:python-musicinfotag-dbid branch from 081824b to aa3d711 Dec 13, 2016

@ronie

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2016

jenkins build this please

@ronie ronie merged commit f239d48 into xbmc:master Jan 4, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Jan 5, 2017

@ronie ronie deleted the ronie:python-musicinfotag-dbid branch Jan 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.