CVideoInfoTag: Moving members from CStdString to more appropriate types (std::vector<>, CDateTime) #394

Merged
merged 23 commits into from Mar 30, 2012

Projects

None yet

4 participants

@Montellese
Member

I started working on this quite a while ago and already discussed some of it with jonathan in some PMs but I wanted to put it out here to get the feedback/opinions of some more people. This is not a finished work as the changes only apply to CVideoInfoTag but they could/should be extended to CAlbum, CArtist, CSong, CMusicInfoTag etc.

The idea is to get rid of having all kinds of members of CVideoInfoTag as CStdStrings if there are better suited data types. An example is the m_strGenre member which contains all genres in one string separated by a character (e.g. / ). While this is useful as an info label for skins, it is complicating things a lot in other areas like JSONRPC where clients have to parse and split the genre string they get to be able to fit movies into their genres.

In the end I adjusted the types of the following members:

  • m_strGenre --> m_gerne: std::vector< std::string >
  • m_strSet --> m_set: std::vector< std::string > (already in master with an earlier PR)
  • m_strSetId --> m_setId: std::vector< int > (already in master with an earlier PR)
  • m_strCountry --> m_country: std::vector< std::string >
  • m_strDirector --> m_director: std::vector< std::string >
  • m_strWritingCredits --> m_writingCredits: std::vector< std::string >
  • m_strStudio --> m_studio: std::vector< std::string >
  • m_strShowLink --> m_showLink: std::vector< std::string >
  • m_strPremiered --> m_premiered: CDateTime
  • m_strFirstAired --> m_firstAired: CDateTime

Like I said I put this out as a PR to get some feedback/opinions on the general idea. Furthermore this has only been tested on win32 and I wasn't able to test all the changes I made (e.g. to UPnP, RSSDirectory, python etc). What I tested is whether the InfoLabels are still the same, whether NFO loading still works and if the JSONRPC output matches. All these things have been confirmed for all the changed members.

So please let me know if you think this idea is worth working on or if you think keeping everything in CStdString is a better solution. Thanks for the feedback.

@Memphiz
Member
Memphiz commented Sep 2, 2011

Vector of strings definitly sounds better and nicer then parsing a string. So IMHO your work on that is worth it :)

@davilla
Contributor
davilla commented Sep 2, 2011

+1e6 on this. CStdString is an xbox'ism and should really die in a fire :)

@jmarshallnz
Member

Code looks good. Note the reading of multiple strings from XML might need some doxy to note that it doesn't clear the vector first (I think this is what we want for chaining etc?)

@Montellese
Member

I have to admit that I didn't think about whether to clear the array first or not. But if we want to support chaining I'll add a doxy comment about it. Thanks for the hint.

@Montellese
Member

One thing I was wondering while I just looked through the changes I made:
Would it make sense to present these changed members as arrays in other places as well (e.g. in python's videoinfotag and listitem)? Until now I just changed the code so that it would work exactly as before (with the exception of jsonrpc) but it might make sense to use the new types in other places as well. But changing it in python would probably break every addon out there right?

@Montellese
Member

Finally came around to extend this work on CMusicInfoTag, CAlbum, CArtist and CSong.

Any feedback and hints to errors are welcome.

@jmarshallnz
Member

I say we merge this immediately following Eden.

@Montellese
Member

Looking through this again I noticed that it will break backwards-compatibility for JSON-RPC. In Eden properties like "genre" etc are returned as a string containing multiple genres seperated by a slash. With these changes the "genre" property will be an array of strings where each string represents one genre. While this is certainly better than the situation in Eden I don't like breaking backwards-compatibility (again and so soon after Eden). I could add some logic to continue returning those properties as single strings containing multiple values.

@jmarshallnz
Member

Sure - just make sure to mark the backward compatibility well so you can drop it when backcompat is broken elsewhere.

Montellese added some commits Jun 16, 2011
@Montellese Montellese Multiple improvements for handling std::vector<std::string>
add easier-to-use SplitString/Split and JoinString/Join methods to StringUtils
add handling of std::vector<std::string> to CVideoDatabase's auto-mapping
add overloaded << and >> to CArchive for std::vector<std::string>
add constructor to CVariant taking a std::vector<std::string>
add SetStringArray() and GetStringArray() to XMUtils
don't add an empty string to an array in StringUtils::SplitString
3c190a7
@Montellese Montellese replace m_strGenre (CStdString) with m_genre (std::vector<std::string…
…>) in CVideoInfoTag
a61cd77
@Montellese Montellese better integrate CVideoInfoTag::m_set and ::m_setId 77066b3
@Montellese Montellese replace CVideoInfoTag::m_strCountry (CStdString) with m_country (std:…
…:vector<std::string>)
d3689b1
@Montellese Montellese replace CVideoInfoTag::m_strDirector (CStdString) with m_director (st…
…d::vector<std::string>)
0ddafa9
@Montellese Montellese replace CVideoInfoTag::m_strWritingCredits (CStdString) with m_writin…
…gCredits (std::vector<std::string>)
301ac55
@Montellese Montellese replace CVideoInfoTag::m_strStudio (CStdString) with m_studio (std::v…
…ector<std::string>)
3acfaa3
@Montellese Montellese replace CVideoInfoTag::m_strShowLink (CStdString) with m_showLink (st…
…d::vector<std::string>)
8704e7e
@Montellese Montellese improvements for handling CDateTime
add Date and DateTime handling (via CDateTime) to CVideoDatabase's auto-mapping
add GetDate/GetDateTime and SetDate/SetDateTime to XMLUtils
cb78d7f
@Montellese Montellese replace CVideoInfoTag::m_strPremiered (CStdString) with m_premiered (…
…CDateTime)
4d6bc0b
@Montellese Montellese replace CVideoInfoTag::m_strFirstAired (CStdString) with m_firstAired…
… (CDateTime)
67b3046
@Montellese Montellese changed type of CVideoInfoTag::m_lastPlayed from CStdString to CDateTime 3a15e83
@Montellese Montellese replace CMusicInfoTag::m_strLastPlayed (CStdString) with m_lastPlayed…
… (CDateTime)
4c7fd8a
@Montellese Montellese replace CMusicInfoTag/CAlbum/CArtist::(m_)strGenre (CStdString) with …
…(m_)genre (std::vector<std::string>)
b110dcc
@Montellese Montellese replace CMusicInfoTag/CAlbum/CSong::(m_)strArtist (CStdString) with (…
…m_)artist (std::vector<std::string>)
aa87c44
@Montellese Montellese replace CMusicInfoTag/CSong::(m_)strAlbumArtist (CStdString) with (m_…
…)albumArtist (std::vector<std::string>)
6e367ad
@Montellese Montellese replace CAlbum/CArtist::strMoods/strStyles/strThemes (CStdString with…
… moods/styles/themes (std::vector<std::string>)
8ea0fa9
@Montellese Montellese replace CArtist::strYearsActive (CStdString) with yearsActive (std::v…
…ector<std::string>)
36c1d89
@Montellese Montellese changed type of CSong::lastPlayed from CStdString to CDateTime 28698a7
@Montellese Montellese replace CArtist::strIntruments (CStdString) with intruments (std::vec…
…tor<std::string>)
fe922be
@Montellese Montellese fix collision of album-/artist-specific properties in CFileItem used …
…by skins
ec886fe
@Montellese Montellese [linux] fix build 880b60d
@Montellese
Member

OK rebased again and added a commit that (hopefully) makes sure we don't break backwards compatibility to JSON-RPC v4 (Eden). Should I squash the other commits into one for all the video related fields and one for all the music related fields or should I merge it the way it is now?

@Montellese Montellese merged commit daf711d into xbmc:master Mar 30, 2012
@tru tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Jan 16, 2013
@tru tru Missing artist in now playing screen
Fixes #394
6e5d24c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment