[Art] Add the item/video path to the "browse for art" dialog #1740

Merged
merged 1 commit into from Dec 1, 2012

4 participants

@ace20022
Team Kodi member

This PR adds the path of an item to the "browse for art" dialog. It includes TV Shows (first path), seasons, episodes, movies, sets (multipath).

In the majority of cases art files are stored in the video/item path, e.g., if the "Artwork Downloader" is used. So this may save a lot of clicks.

@MartijnKaijser
Team Kodi member

Sorry but this is not needed any more.
Edit:
At least not artwork downloader specific. So you should at least drop the comments in the file

@Montellese
Team Kodi member

I have a branch with similar work but I went as far as setting the directory containing the media item as the starting directory. But that probably depends on the use case but I have all my additional artwork stored with the media item itself so I always have to browse to that directory anyway. I never finished my work so I'm welcoming this. Will look at the code later. IIRC you can get the path to the tvshow from CVideoInfoTag::m_strFile.

@Montellese Montellese commented on an outdated diff Nov 6, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.cpp
@@ -884,3 +887,18 @@ void CGUIDialogVideoInfo::SetLabel(int iControl, const CStdString &strLabel)
{
return m_movieItem->GetArt("thumb");
}
+
+void CGUIDialogVideoInfo::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item)
+{
+ CMediaSource* cur = new CMediaSource();
@Montellese
Team Kodi member

No need for this to be a pointer. A normal object will work just as well and you can't forget the call to delete in that case (which you did).
Same applies to curP and the changes in CGUIWindowVideoNav.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Montellese Montellese commented on an outdated diff Nov 6, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.cpp
@@ -884,3 +887,18 @@ void CGUIDialogVideoInfo::SetLabel(int iControl, const CStdString &strLabel)
{
return m_movieItem->GetArt("thumb");
}
+
+void CGUIDialogVideoInfo::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item)
+{
+ CMediaSource* cur = new CMediaSource();
+ std::vector<CStdString>* curP = new std::vector<CStdString>;
+ CStdString filePath = URIUtils::GetDirectory(item.GetVideoInfoTag()->GetPath());
+ if (item.IsStack())
+ {
+ filePath = URIUtils::GetDirectory(CStackDirectory::GetFirstStackedFile(
+ item.GetVideoInfoTag()->GetPath()));
+ }
+ curP->push_back(filePath);
+ cur->FromNameAndPaths("","* " + g_localizeStrings.Get(36041), *curP);
@Montellese
Team Kodi member

I'd put the "* " into the localized string as well (think of right-to-left languages etc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Montellese Montellese commented on an outdated diff Nov 6, 2012
xbmc/video/windows/GUIWindowVideoNav.cpp
@@ -1775,3 +1778,33 @@ CStdString CGUIWindowVideoNav::GetLocalizedType(const std::string &strType)
else
return "";
}
+
+void CGUIWindowVideoNav::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item)
+{
+ CMediaSource* cur = new CMediaSource();
+ std::vector<CStdString>* curP = new std::vector<CStdString>;
+ CStdString filePath = item.GetVideoInfoTag()->GetPath();
+ if (item.IsStack())
+ {
+ filePath = URIUtils::GetDirectory(CStackDirectory::GetFirstStackedFile(
+ item.GetVideoInfoTag()->GetPath()));
+ }
+ else if (item.GetVideoInfoTag()->m_type.Equals("set"))
@Montellese
Team Kodi member

This is a bit tricky because we can't assume that the filesystem structure has an extra directory for the movie set. Not sure what the best way to go is though.

EDIT: To make things easier we could change the logic in CVideoDatabase::GetSetsByWhere() to store the path of the first item im CVideoInfoTag::m_strPath for a set item. Then you could safe the additional call to GetDirectory() etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Montellese Montellese and 1 other commented on an outdated diff Nov 6, 2012
xbmc/video/windows/GUIWindowVideoNav.cpp
+
+void CGUIWindowVideoNav::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item)
+{
+ CMediaSource* cur = new CMediaSource();
+ std::vector<CStdString>* curP = new std::vector<CStdString>;
+ CStdString filePath = item.GetVideoInfoTag()->GetPath();
+ if (item.IsStack())
+ {
+ filePath = URIUtils::GetDirectory(CStackDirectory::GetFirstStackedFile(
+ item.GetVideoInfoTag()->GetPath()));
+ }
+ else if (item.GetVideoInfoTag()->m_type.Equals("set"))
+ {
+ CFileItemList items;
+ GetDirectory(item.GetPath(), items);
+ if(items[1])
@Montellese
Team Kodi member

Why index 1 and not 0 (same on the next line)? I'd just do

if (GetDirectory(item.GetPath(), items) && items.Size() > 0)

here because GetDirectory() might also fail.

@ace20022
Team Kodi member
ace20022 added a note Nov 6, 2012

item[0].m_strPath contains "videodb://1/2/", not sure why.
The movies are store from 1 on

@Montellese
Team Kodi member

Ah that's the "..." (parent) item you see in the GUI. Unfortunately that depends on GUI settings so doing it like this here is not safe as the user might have the parent item disabled and the set might only contain a single movie and then items[1] wouldn't exist and it would blow up.

Probably best to go with the way I mentioned in the comment further up i.e. setting the path of the first item in the set as the path of the set's CVideoInfoTag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Montellese Montellese commented on an outdated diff Nov 6, 2012
xbmc/video/windows/GUIWindowVideoNav.cpp
@@ -1775,3 +1778,33 @@ CStdString CGUIWindowVideoNav::GetLocalizedType(const std::string &strType)
else
return "";
}
+
+void CGUIWindowVideoNav::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item)
+{
+ CMediaSource* cur = new CMediaSource();
+ std::vector<CStdString>* curP = new std::vector<CStdString>;
+ CStdString filePath = item.GetVideoInfoTag()->GetPath();
@Montellese
Team Kodi member

This is most likely the full path to the media file and not the path to it's parent directory so you are missing a URIUtils::GetDirectory().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ace20022
Team Kodi member

@Montellese I made the changes you suggested, thanks for the really fast review!

Any ideas about distributed tv shows? Consider the following structure:
x:\shows\The X-Files\Season 1 and y:\shows\The X-Files\Season 2
If a user wants to change the poster of season 2, which path should be offered?

@ace20022
Team Kodi member

@MartijnKaijser

I changed the comment, even though it was a temp comment anyway.
Can you please explain why this is not needed any more?

@Montellese Montellese and 1 other commented on an outdated diff Nov 6, 2012
xbmc/video/VideoDatabase.cpp
@@ -4867,7 +4867,11 @@ bool CVideoDatabase::GetSetsByWhere(const CStdString& strBaseDir, const Filter &
pItem->SetPath(itemUrl.ToString());
pItem->m_bIsFolder = true;
- pItem->GetVideoInfoTag()->m_strPath = pItem->GetPath();
+ // set the path of the set as the path of its first movie
+ if (it->second.movies.size() > 0)
@Montellese
Team Kodi member

This check is not necessary as at the very start of the for-loop there's a check for it->second.movies.size() <= 0 which will result in skipping the set so this check will always be true.

@ace20022
Team Kodi member
ace20022 added a note Nov 6, 2012

Ah! Very nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartijnKaijser
Team Kodi member

@ace20022
it will directly set the URL in the database and will not download any file by default anymore. It works exactly the same like the scrapers atm.

I do agree that when opening the browse for art takes to many clicks and imo the ideal situation would be that the browser opens in the folder where the video files is located.
Movie sets is a bit of a difficult issue on where to start. Option would be to start in the root of the source where the movies belong to.

@Montellese
Team Kodi member

Starting in the root of where the movies belongs has the same problem as with sets and tvshows: A movie source can consist of several paths.
IMO in these cases we either decide to make an (educated) guess and use the first path available (for sets the path of the first movie) or we don't add any special handling for these items and open in the root of the filesystem.

@jmarshallnz
Team Kodi member

You should ideally open with GetVideoInfoTag()->m_basePath if it's available (non-empty). That's the item that XBMC has decided represents the actual "movie". This is either a directory that you can open directly, or is a filepath. I think checking to see if it's the same as GetVideoInfoTag()->m_strFileNameAndPath will do the trick (if it is, go up a directory, if not, stay with it).

For multipaths etc. just use the first path found. Same with sets and so on. There's nothing to be done with actors.

@ace20022
Team Kodi member

@MartijnKaijser
Now I understand, you referred to the downloader. The new behaviour will save a lot of bandwidth which is great!
But I for myself prefer local artwork, so it was just an example.

@ace20022
Team Kodi member

@jmarshallnz

Since you mentioned multipath, I wondered why the tv show in my test setup
x:\shows\The X-Files\Season 1 and y:\shows\The X-Files\Season 2
had no multipath. There's one episode in each directory.

I think I discovered a bug:

  • the tv shows windows shows the correct amount of episodes: 2
  • the tv shows info dialog shows 1 ep and path x:... set
  • after removing the show from the db only the one with path x:.. is removed, i.e. the show remains with 1 ep and path y:.. set
@ace20022
Team Kodi member

Regarding the path of an movie set: we could construct a multipath from the movies inside the set. Any thoughts?

@jmarshallnz
Team Kodi member

I wouldn't bother doing anything for sets, but if you must, the first movie is plenty - that's where we take the art from now anyway.

@ace20022
Team Kodi member

@jmarshallnz Sorry for spamming, but could you verify this #1740 (comment) ?

@jmarshallnz jmarshallnz commented on an outdated diff Nov 8, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.cpp
@@ -884,3 +887,18 @@ void CGUIDialogVideoInfo::SetLabel(int iControl, const CStdString &strLabel)
{
return m_movieItem->GetArt("thumb");
}
+
+void CGUIDialogVideoInfo::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item)
+{
+ CMediaSource cur;
+ std::vector<CStdString> curP;
+ CStdString filePath = URIUtils::GetDirectory(item.GetVideoInfoTag()->GetPath());
@jmarshallnz
Team Kodi member

IMO you want to use m_basePath here for most cases, and only if it's the same as item.GetVideoInfoTag()->m_strFileNameAndPath use URIUtils::GetDirectory()

Note that you'll also want to do the stack stuff before you go up a folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on an outdated diff Nov 8, 2012
xbmc/video/windows/GUIWindowVideoNav.cpp
+ {
+ filePath = URIUtils::GetDirectory(CStackDirectory::GetFirstStackedFile(
+ item.GetVideoInfoTag()->GetPath()));
+ }
+ else if (item.GetVideoInfoTag()->m_type.Equals("season"))
+ {
+ /*
+ How should we handle distributed shows?
+ Offer the first tv show path found in the db?
+ How can we get this path?
+ */
+ }
+ curP.push_back(filePath);
+ cur.FromNameAndPaths("", g_localizeStrings.Get(36041), curP);
+ sources.push_back(cur);
+}
@jmarshallnz
Team Kodi member

You should just call the routine in the info dialog class here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on an outdated diff Nov 10, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.cpp
+{
+ CMediaSource itemSource;
+ std::vector<CStdString> itemSourcePath;
+ CStdString itemDir = item.GetVideoInfoTag()->m_basePath;
+
+ if (! itemDir.IsEmpty())
+ {
+ if (item.IsStack())
+ itemDir = URIUtils::GetDirectory(CStackDirectory::GetFirstStackedFile(itemDir));
+
+ if (itemDir.Equals(item.GetVideoInfoTag()->m_strFileNameAndPath))
+ itemDir = URIUtils::GetDirectory(itemDir);
+ }
+ //seasons
+ else
+ itemDir = URIUtils::GetDirectory(item.GetVideoInfoTag()->GetPath());
@jmarshallnz
Team Kodi member

I think for seasons you just use item.GetVideoInfoTag()->GetPath() anyway (which points to the show folder) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on an outdated diff Nov 10, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.cpp
+ {
+ if (item.IsStack())
+ itemDir = URIUtils::GetDirectory(CStackDirectory::GetFirstStackedFile(itemDir));
+
+ if (itemDir.Equals(item.GetVideoInfoTag()->m_strFileNameAndPath))
+ itemDir = URIUtils::GetDirectory(itemDir);
+ }
+ //seasons
+ else
+ itemDir = URIUtils::GetDirectory(item.GetVideoInfoTag()->GetPath());
+
+ if (! itemDir.IsEmpty() && CDirectory::Exists(itemDir))
+ {
+ itemSourcePath.push_back(itemDir);
+ itemSource.FromNameAndPaths("", g_localizeStrings.Get(36041), itemSourcePath);
+ sources.push_back(itemSource);
@jmarshallnz
Team Kodi member

You can drop itemSourcePath and just set itemSource.strPath and itemSource.strName directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on an outdated diff Nov 10, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.cpp
@@ -884,3 +888,29 @@ void CGUIDialogVideoInfo::SetLabel(int iControl, const CStdString &strLabel)
{
return m_movieItem->GetArt("thumb");
}
+
+void CGUIDialogVideoInfo::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item)
+{
+ CMediaSource itemSource;
+ std::vector<CStdString> itemSourcePath;
@jmarshallnz
Team Kodi member

Move these to where they're used (reduce scope)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on an outdated diff Nov 10, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.h
@@ -42,6 +42,7 @@ class CGUIDialogVideoInfo :
virtual bool HasListItems() const { return true; };
static std::string ChooseArtType(const CFileItem &item, std::map<std::string, std::string> &currentArt);
+ static void AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem item);
@jmarshallnz
Team Kodi member

const CFileItem &

(i.e. use a reference)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ace20022
Team Kodi member

@jmarshallnz
made the changes you suggested, thanks.

@ace20022
Team Kodi member

@jmarshallnz your comment got lost, sorry. I changed positions, don't know why it was that way ;)

@jmarshallnz
Team Kodi member

Looks good. Over to you @cptspiff.

@ghost

url should be prettified and stripped for pwds etc.

@jmarshallnz
Team Kodi member

I don't believe the url is displayed in the UI - the name of the share will be, and anything displayed in the filebrowser (e.g. the path when you select an item) will be prettified anyway?

@ghost

right, it will be prettified by the dialog, forgot that. rebase it up..

@ghost

ace^^^

@ace20022
Team Kodi member

I'm working on it; having problems because #1418 got merged: the order of the members of a set is random now and therefore its path. Should I push it anyway and provide a solution later?

@ghost

didn't realize there was a reason for the automerge to fail ;) take your time.

@ace20022
Team Kodi member

In the first commit a set's path is randomly set, due to the changes mentioned above.
The second commit fixes this. The additional int variable could be saved if the year of a set is set to the minimum and not the maximum. This needs a review again ;)

@jmarshallnz
Team Kodi member

The two commits can be squashed together.

@Montellese your thoughts on minimum vs maximum year? (I think maximum makes most sense).

@Montellese
Team Kodi member

I don't have any strong feelings either way. IIRC we discussed these values once in a trac ticket and the consensus was to go with maximum year.

@jmarshallnz
Team Kodi member

Yeah - maximum makes more sense to me for most sorting stuff.

@ace20022
Team Kodi member

I just found out that the browser can handle mulitpaths, really nice. I haven't used them because I got confused with these "stacked shows" I mentioned before. Will do the rest of the work tomorrow, since beta 1 is closed now anyway.

@ace20022
Team Kodi member

I used a new test structure and rebuild the db. Now GetVideoInfoTag()->m_basePath always is a directory. Has this been changed since I opened the PR?
The only exception are episodes, but these can't be stacked afaik.
The path of a set is a multipath now because the browser can handle them nicely.

(btw I kept the other commit(s) only to have a history)

@ace20022
Team Kodi member

Ok, this is getting worse and more complex than I thought at the beginning.
The value of m_basePath depends on a scraper setting, namely "Movies are in seperate...".
If that setting is on, m_basePath stores a dir, otherwise it stores a file path, possibly stacked etc.; really confusing for a newbie.
Will shut up now till I (think I) covered all cases.

@ace20022
Team Kodi member

Will shut up now till I (think I) covered all cases.

Hopefully I have now;)

@ace20022
Team Kodi member

Next try, and thanks for all the patience!

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Nov 16, 2012
xbmc/video/dialogs/GUIDialogVideoInfo.cpp
@@ -876,3 +880,31 @@ void CGUIDialogVideoInfo::SetLabel(int iControl, const CStdString &strLabel)
{
return m_movieItem->GetArt("thumb");
}
+
+void CGUIDialogVideoInfo::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem &item)
+{
+ if (!item.HasVideoInfoTag())
+ return;
+
+ CStdString itemDir = item.GetVideoInfoTag()->m_basePath;
+
+ if (!itemDir.IsEmpty())
+ {
+ if (URIUtils::IsStack(itemDir))
+ itemDir = URIUtils::GetDirectory(CStackDirectory::GetFirstStackedFile(itemDir));
+
+ if (itemDir.Equals(item.GetVideoInfoTag()->m_strFileNameAndPath))
+ itemDir = URIUtils::GetDirectory(itemDir);
@jmarshallnz
Team Kodi member

Sorry to be a PITA, but I think this block can be replaced by

CFileItem item(itemDir, false);
if (item.IsVideo())
  itemDir = URIUtils::GetParentPath(itemDir);

thus avoiding the inclusion of StackDirectory.h

@ace20022
Team Kodi member

Ok, I changed the whole thing to:

void CGUIDialogVideoInfo::AddItemPathToFileBrowserSources(VECSOURCES &sources, const CFileItem &item)
{
  if (!item.HasVideoInfoTag())
    return;

  CStdString itemDir = item.GetVideoInfoTag()->m_basePath;

  //season
  if (itemDir.IsEmpty())
    itemDir = item.GetVideoInfoTag()->GetPath();

  CFileItem itemTmp(itemDir, false);
  if (itemTmp.IsVideo())
    itemDir = URIUtils::GetParentPath(itemDir);

  if (!itemDir.IsEmpty() && CDirectory::Exists(itemDir))
  {
    CMediaSource itemSource;
    itemSource.strName = g_localizeStrings.Get(36041);
    itemSource.strPath = itemDir;
    sources.push_back(itemSource);
  }
}

I noticed one more thing: yesterday we decided to take a single path for multipath shows, but the path of a season of such a show is a multi one. I don't see an easy solution for getting the correct path out of it; besides the way to use the, probably wrong, first one, which would mean another include. So maybe we should revise the decision from yesterday or just leave it as it is now, which of course is inconsistent.
Now I'm a PITA ;)

@jmarshallnz
Team Kodi member

Just leave it as it is. The whole multipath thing will be sorted after Frodo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz
Team Kodi member

Other than that change, I'm happy with this now.

@Montellese, @davilla it's your call as to whether this goes into Frodo. It requires a string addition.

@ace20022
Team Kodi member

Done.
@jmarshallnz I thought about adding something similar to the music section, what do you think?

@MartijnKaijser
Team Kodi member

It could indeed be useful for music too. Make that a separate PR
Can we pull this in for Frodo?

@jmarshallnz
Team Kodi member

@davilla up to you. As @Montellese suggested above, this isn't strictly a bugfix, though is a UI improvement that would be nice to have.

@ace20022 ace20022 Add the item/video path to the "browse for art" dialog.
This includes TV Shows (first path), seasons, episodes, movies, sets (multipath).
5735ae2
@ace20022
Team Kodi member

Rebased, just in case...

@MartijnKaijser MartijnKaijser merged commit 3760407 into xbmc:master Dec 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment