Handle cases where subtitle fails to save to destination directory #4196

Merged
merged 1 commit into from Feb 22, 2014

Conversation

Projects
None yet
5 participants
Member

arnova commented Feb 12, 2014

This takes some of the logic of #3869 and adds some improvements. Basically it will always first download the remote subtitle to special://temp (or special://subtitles, if available) and try copy it from there to the movie's directory (if enabled). This is desirable as one can't always know upfront whether a certain directory will be writable.

Note that this also includes a fix for storing URL-encoded subtitle-files on the local filesystem.

Member

arnova commented Feb 12, 2014

@da-anda / @jmarshallnz : Please comment/review

Contributor

t-nelson commented Feb 12, 2014

@elupus @ace20022 I know you guys are invested in subs to some extent. Mind reviewing also?

Member

arnova commented Feb 13, 2014

jenkins build this please

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 13, 2014

xbmc/video/dialogs/GUIDialogSubtitles.cpp
@@ -420,29 +418,30 @@ void CGUIDialogSubtitles::OnDownloadComplete(const CFileItemList *items, const s
// Get (unstacked) path
const CStdString &strCurrentFile = g_application.CurrentUnstackedItem().GetPath();
+ const CStdString &strCurrentFilePath = URIUtils::GetDirectory(strCurrentFile);
+ const CStdString &strCurrentFileName = URIUtils::GetFileName(strCurrentFile);
@jmarshallnz

jmarshallnz Feb 13, 2014

Member

I don't think these should be const references. They can be const if you like.

@arnova

arnova Feb 14, 2014

Member

Hmmm, yes didn't think that through properly (or is it just brain-dead replicating what was already on the line above?)...

@jmarshallnz jmarshallnz commented on an outdated diff Feb 13, 2014

xbmc/video/dialogs/GUIDialogSubtitles.cpp
}
+
+ // for ".sub" subtitles we check if ".idx" counterpart exists and copy that as well
+ if (strSubExt.Equals(".sub"))
+ {
+ strUrl = URIUtils::ReplaceExtension(strUrl, ".idx");
+ if(CFile::Exists(strUrl))
+ {
+ CStdString strSubNameIdx = StringUtils::Format("%s.%s.idx", strFileName.c_str(), strSubLang.c_str());
+ // Handle URL decoding/slash fixing for local storage, if required
+ strDestFile = URIUtils::ChangeBasePath(strCurrentFilePath, strSubNameIdx, strDestPath);
@jmarshallnz

jmarshallnz Feb 13, 2014

Member

I think this may not give what you want. Consider a share that requires URL-encoded paths, while local requires URL-decoded paths. The local file will be URL-decoded, so the share path above will also be URL-decoded, while this one will remain URL-encoded.

It might be better to use FileUtils here to copy them both at once (create a CFileItemList and copy across).

Member

jmarshallnz commented Feb 13, 2014

I'd tend to do it a bit more like this (pseudo-code)

vector<string> paths;
if (http)
  paths.push_back(AddFileToFolder("special://temp/", filename));
else
{
  if (savetomoviefolder)
    paths.push_back(moviefolder);
  if (subtitlesfolder)
    paths.push_back("special://subtitles/");
  else
    paths.push_back("special://temp/");
}

vector<string> files;
files.push_back(subsUrl);
if (.sub and has .idx)
{ // find .idx file
  files.push_back(idxUrl);
}

for_each(paths)
{
  for_each(files)
  {
     // construct path
     if (!CFile::Cache())
       break; // no point continuing
  }
}
Member

arnova commented Feb 14, 2014

@jmarshallnz : Your suggestion looks a lot nicer, I also thought the code (as it already was) looked a bit messy. I'll have a look at it this weekend.

Member

arnova commented Feb 14, 2014

@jmarshallnz : Only drawback of implementing the fallback this way is that we may try to download the subtitle several times from the "other" side (eg. OpenSubtitles). I'm not sure whether this is desirable especially since downloading from eg. OpenSubtitles can already be quite slow every now and then. This is probably why I chose to always download to temp first....

Member

jmarshallnz commented Feb 14, 2014

I think you'll find all the services download the subs to local anyway, before you get this far.

Member

arnova commented Feb 16, 2014

@jmarshallnz : Yeah probably, a debug log should confirm that. But that illustrates that the current implementation is rather user-unfriendly.

Member

jmarshallnz commented Feb 16, 2014

No, I meant the services (python scripts) download locally to addon_data already anyway, so all these are local copies. A service later on might give us a direct http:// link, however, so it may be better to copy it locally first anyway as you have it here.

I think then with a little bit of cleanup this can go in as-is. Specifically, add the .idx/.sub thing so you have a vector of paths to transfer over so that the caching can be done in a loop (complete with correct URL en/decoding)

Member

arnova commented Feb 17, 2014

@jmarshallnz : Only problem with the vector of paths is that it probably requires a vector<map<Cstdstring,CStdstring>> (for the source / target of the sub) else you'll have to create the changed named for idx/sub inside the loop. Or do you see some other nicer way to accomplish this?

Member

jmarshallnz commented Feb 17, 2014

Wouldn't you want to create the change name for idx/sub inside the loop anyway, as it would depend on the destination path (url decoding etc.)

Member

arnova commented Feb 17, 2014

Yeah but you can determine the destination path upfront, right? And if I understand correctly you meant a vector of files to transfer, which normally is only a single file (eg. srt) and sometimes 2 (.sub/.idx). Or do you mean a vector still holding eg. movie-dir and special://subtitles (since special://temp is the default folder anyway) ?

Member

jmarshallnz commented Feb 17, 2014

vector of files to transfer (so it can easily be extended later as needed). The destination could be a vector if you want, but I suspect it's just as easy to download to one of special://subtitles or temp and then (optionally) move to the moviedir.

MartijnKaijser added this to the Pending for inclusion milestone Feb 17, 2014

Member

arnova commented Feb 19, 2014

@jmarshallnz : Updated, please review. Note that instead of using a vector I just iterate over items[] now which should also make our code properly handle eg. stacks as soon as the addons provide those.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 20, 2014

xbmc/video/dialogs/GUIDialogSubtitles.cpp
+ {
+ CLog::Log(LOGWARNING, "%s - Saving of subtitle %s to %s failed using fallback %s", __FUNCTION__, strDownloadFile.c_str(), strDestPath.c_str(), strDownloadPath.c_str());
+ strDestPath = strDownloadPath; // Copy failed, use fallback for the rest of the items
+ }
+ }
+
+ // for ".sub" subtitles we check if ".idx" counterpart exists and copy that as well
+ if (strSubExt.Equals(".sub"))
+ {
+ strUrl = URIUtils::ReplaceExtension(strUrl, ".idx");
+ if(CFile::Exists(strUrl))
+ {
+ CStdString strSubNameIdx = StringUtils::Format("%s.%s.idx", strFileName.c_str(), strSubLang.c_str());
+ // Handle URL decoding/slash correction:
+ strDestFile = URIUtils::ChangeBasePath(strCurrentFilePath, strSubNameIdx, strDownloadPath);
+ CFile::Cache(strUrl, strDestFile);
@jmarshallnz

jmarshallnz Feb 20, 2014

Member

I think you want strDestPath here instead of strDownloadPath (and that assumes you want to copy it directly rather than via special://subtitles/ or whatever)

Alternatively, add it to the CFileItemList.

@arnova

arnova Feb 20, 2014

Member

At this point strDestPath should always be equal to strDownloadPath (look at the code above), so I think it's fine.

@jmarshallnz

jmarshallnz Feb 20, 2014

Member

You're right, but this is really non-obvious code. I guess it'll be clearer if the comment regarding copying from temp was updated to make it clear what is going on.

Member

arnova commented Feb 20, 2014

@t-nelson: You mean to have a copy function which takes a destination path without a filename, you mean?

ps. I think this should be a single commit (although the commit message may suggest otherwise), it just fixes some other stuff as a bonus. So I'd rather squash this than split it up.

Contributor

t-nelson commented Feb 20, 2014

@t-nelson: You mean to have a copy function which takes a destination path without a filename, you mean?

Right

ps. I think this should be a single commit (although the commit message may suggest otherwise), it just fixes some other stuff as a bonus. So I'd rather squash this than split it up.

For commit, sure. For review, way better to keep that stuff broken up.

Member

arnova commented Feb 21, 2014

@jmarshallnz : Squashed and updated (improved comment + added toast in case initial download fails).

@t-nelson : Had a look at creating a CopyTo()-function but that doesn't improve things much imo so I'd rather skip that. The problem is you still need to provide the destination filename so you get a function like:

bool CopyTo(const CStdString &sourceFile, const CStdString &targetFile, const CStdString &targetPath)
{
// Handle URL decoding/slash correction:
CStdString strTargetFile = URIUtils::ChangeBasePath(sourceFile, targetFile, targetPath);

return CFile::Cache(sourceFile, strTargetFile);
}

Member

jmarshallnz commented Feb 22, 2014

jenkins build this please

@jmarshallnz jmarshallnz added a commit that referenced this pull request Feb 22, 2014

@jmarshallnz jmarshallnz Merge pull request #4196 from arnova/sub_fallback
Handle cases where subtitle fails to save to destination directory
3915ac4

@jmarshallnz jmarshallnz merged commit 3915ac4 into xbmc:master Feb 22, 2014

1 check passed

default Merged build #260 succeeded in 1 hr 1 min
Details
Contributor

amet commented Mar 7, 2014

@arnova please check http://forum.xbmc.org/showthread.php?tid=188250&pid=1645765#pid1645765 , looks something is broken with one of the recent sub commits. only streamed content fails

Member

arnova commented Mar 7, 2014

Strange, don't see anything obvious that could cause this. Will have a look this weekend.

arnova deleted the arnova:sub_fallback branch Nov 7, 2014

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