Musicvideo artist in setinfo #884

Merged
merged 3 commits into from May 2, 2012

Projects

None yet

3 participants

@jmarshallnz
Member

Two changes here:

  1. CVideoInfoTag::m_strArtist to use a vector (this one was missed in #394).
  2. Support a list for artist for musicvideos in setInfo (previously not available at all).

@Montellese: I didn't like adding those headers to the AnnounceManager. Your thoughts on allowing the array there instead?

@Montellese
Member

First of all thanks for this. I didn't realize that I had missed that one in #394.

Concerning the headers in AnnouncementManager: I agree that including settings/AdvancedSettings.h is far from ideal (StringUtils.h isn't that big of a deal IMO). I was thinking anyway if it would make sense for all JSON-RPC properties which are built using StringUtils::Join to use a fixed seperator instead of the one from g_advancedSettings. This will make it easier for clients to parse those properties because they don't have to know about the seperator in case an XBMC user has changed it to something else than a forward slash.

Making that "artist" property an array would break backwards-compatibility to JSON-RPC in Eden and I'd like to avoid that for a while.

@jmarshallnz
Member

What separator do you want - just use the hardcoded " / " in case clients are already using it, or change to something else (eg a pipe?)

We should do the same thing in VideoInfoTag -> CVariant conversion as well I guess?

@Montellese
Member

I'd go with the forward slash as you suggested because that's probably what everyone is using. And yeah this should be done in CVideoInfoTag::Serialize() and CMusicInfoTag::Serialize() as well.

But that was just an idea I had while considering the problem at hand.

@Montellese
Member

I pushed a commit (Montellese@a0e2d7d) which changes the calls to StringUtils::Join to use a fixed " / " separator instead of the one from CAdvancedSettings. Should I provide it as a PR or do you want to pull it into this one and adjust your code accordingly?

@jmarshallnz
Member

Thanks - I'll pull that in here.

@jmarshallnz
Member

Updated + rebased.

@jmarshallnz jmarshallnz was assigned Apr 22, 2012
@Montellese Montellese commented on the diff Apr 22, 2012
xbmc/video/VideoInfoTag.cpp
@@ -610,10 +609,10 @@ void CVideoInfoTag::ParseNative(const TiXmlElement* movie, bool prioritise)
if (pValue)
{
const char* clear=node->Attribute("clear");
- if (m_strArtist.IsEmpty() || (clear && stricmp(clear,"true")==0))
- m_strArtist = pValue;
- else
- m_strArtist += g_advancedSettings.m_videoItemSeparator + pValue;
+ if (clear && stricmp(clear,"true")==0)
+ m_artist.clear();
+ vector<string> artists = StringUtils::Split(pValue, g_advancedSettings.m_videoItemSeparator);
+ m_artist.insert(m_artist.end(), artists.begin(), artists.end());
@Montellese
Montellese Apr 22, 2012 Member

Is this extra parsing (instead of using XMLUtils::GetStringArray() really necessary? Is it because of the optional < name > tag?

@jmarshallnz
jmarshallnz Apr 22, 2012 Member

Yeah, because of the optional tag. No idea if it's used or not in the scrapers. @vdrfan you don't happen to know?

@mkortstiege
mkortstiege Apr 23, 2012 Member

Not sure about this one, sorry.

@Montellese
Member

Looks good to me.

@jmarshallnz jmarshallnz merged commit 58bdd8c into xbmc:master May 2, 2012
@mikedm139 mikedm139 pushed a commit to mikedm139/plex-home-theater-public that referenced this pull request Nov 30, 2013
@tru tru If the server doesn't specify "transcoderAudio" or "transcoderVideo" …
…we shouldn't default to true.

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