Skip to content

fixed: Properly handle stacked files for subtitles #4274

Merged
merged 1 commit into from Mar 3, 2014

6 participants

@arnova
Team Kodi member
arnova commented Feb 24, 2014

This is a followup for #4196 & #3819. Note that #4196 multi-sub implementation is pretty flawed and this should fix that.

Note that I'm not sure our current subtitle API is flexible/capable enough to properly handle future-support for stacked/multiple subtitles. For what we have, this seems to be the best approach, but of course any suggestions/discussions are welcome. And since we're still pre-Gotham-beta, this would be the time if we want to change our API after all.

@arnova
Team Kodi member
arnova commented Feb 24, 2014

@jmarshallnz : Please comment/review. jenkins build this please

@amet
amet commented Feb 24, 2014

what would the API change be? as it is subtitle services can(if they support it) return more than one subtitle file, is that not enough?

see here how we can get more than one subtitle -> https://github.com/amet/service.subtitles.opensubtitles/blob/Gotham/service.subtitles.opensubtitles/service.py#L61

we need to send stack=True to it and it should try to get all.

note that I have zero stacked files and in all these years have successfully stayed away from it, so I could be a wrong person to comment :)

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 24, 2014
xbmc/video/dialogs/GUIDialogSubtitles.cpp
{
CStdString subPath = CSpecialProtocol::TranslatePath("special://subtitles");
if (!subPath.empty())
strDownloadPath = subPath;
- strFileName = URIUtils::GetFileName(strCurrentFile);
+ // Handle stacks, if any
+ if (g_application.CurrentFileItem().IsStack() && items->Size() > 1)
+ {
+ CStackDirectory::GetPaths(g_application.CurrentFileItem().GetPath(), vecFiles);
+ }
+
+ // Make sure (stack) size is the same is the size of the items handed to us (this includes non-stack items!)
+ if (vecFiles.size() != (unsigned int) items->Size())
@jmarshallnz
Team Kodi member
jmarshallnz added a note Feb 24, 2014

I'd push this into the if (Stack) bracket to make it clear (and append the current file in an else)

@arnova
Team Kodi member
arnova added a note Feb 24, 2014

Good point. Updated PR together with your comment below.

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 Feb 24, 2014
xbmc/video/dialogs/GUIDialogSubtitles.cpp
// Iterate over all items to transfer
- for (int i = 0; i < items->Size(); i++)
+ for (unsigned int i = 0; i < vecFiles.size(); i++)
{
CStdString strUrl = items->Get(i)->GetPath();
@jmarshallnz
Team Kodi member
jmarshallnz added a note Feb 24, 2014

It's a bit icky to be indexing two vectors with the same index. If you're going to do it, I'd add a && i < (unsigned int)items->Size() to be on the safe side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova
Team Kodi member
arnova commented Feb 24, 2014

@amet, I vaguely recall looking at that code before and at least back then there was some stuff missing to make it work properly (I think the zip unpacker + correctly naming/ordering the individual files was the problem). We also need the possibility to fallback to single items, meaning the user should have the option to download a stacked (multi-file) sub but also the ability to select single subs (like how it is now) as the service may not offer a correct one for one or the other.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 25, 2014
xbmc/video/dialogs/GUIDialogSubtitles.cpp
{
CStdString subPath = CSpecialProtocol::TranslatePath("special://subtitles");
if (!subPath.empty())
strDownloadPath = subPath;
- strFileName = URIUtils::GetFileName(strCurrentFile);
+ // Handle stacks: Make sure (stack) size is the same as the size of the items handed to us
+ if (g_application.CurrentFileItem().IsStack() && items->Size() > 1 && items->Size() == (int) vecFiles.size())
@jmarshallnz
Team Kodi member
jmarshallnz added a note Feb 25, 2014

You haven't put anything in vecFiles yet...

@arnova
Team Kodi member
arnova added a note Feb 25, 2014

Ah crap, that's probably why I wrote it the way I did initially. But what did you mean then? My idea was that you make sure the number of files in the vector is the same as the number of items.

@jmarshallnz
Team Kodi member
jmarshallnz added a note Feb 25, 2014
if (IsStack())
{
  Stack::GetPaths(vecFiles);
  if (vecFiles.size() != items->Size())
  {
    vecFiles.clear();
    vecFiles.push_back(strCurrentFile);
  }
}
@arnova
Team Kodi member
arnova added a note Feb 25, 2014

Just to be clear:

if (IsStack())
{
Stack::GetPaths(vecFiles);
if (vecFiles.size() != items->Size())
{
vecFiles.clear();
vecFiles.push_back(strCurrentFile);
}
}
else
{
vecFiles.push_back(strCurrentFile);
}

Right? Not necessarily prettier than it was but it's probably clearer for the other readers out there ;-)

@jmarshallnz
Team Kodi member
jmarshallnz added a note Feb 25, 2014

yes, you need the else block for the usual case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@amet
amet commented Feb 25, 2014

@arnova we will be limited to whatever the service provider gives us. If you find me on IRC(or e-mail me) we can discuss whats needed and see if we have that in any of the services that offer API(opensubtitles, podnapisi) as all others are just scraping the websites and chances are that there will be nothing there.

@arnova
Team Kodi member
arnova commented Feb 25, 2014

@jmarshallnz : Updated
@amet / @jmarshallnz : Note the stack=true url option I've added, which the addons can use to determine whether they also need to fetch multi-file subs. @amet: Please have a look, I'll try to catch you on IRC one of these days to discuss the Python implementation.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 27, 2014
@arnova
Team Kodi member
arnova commented Feb 28, 2014

@jmarshallnz : Any (additional) ideas about this? I think we (also) need some way to flag stacked subs (= zip archive with multiple subtitles) in the subtitle dialog.

@amet
amet commented Mar 2, 2014

@arnova stack=true helps, thats a good idea as it saves checking for it in script all the time

@jmarshallnz
Team Kodi member

ok from my perspective. jenkins build this please

@arnova
Team Kodi member
arnova commented Mar 3, 2014

@t-nelson / @jmarshallnz : So this can be merged before we branch?

@t-nelson t-nelson commented on the diff Mar 3, 2014
xbmc/video/dialogs/GUIDialogSubtitles.cpp
@@ -325,6 +328,10 @@ void CGUIDialogSubtitles::Search(const std::string &search/*=""*/)
if (setting)
url.SetOption("languages", setting->ToString());
+ // Check for stacking
+ if (g_application.CurrentFileItem().IsStack())
@t-nelson
t-nelson added a note Mar 3, 2014

Is this safe to call all over the show? What playback is stopped mid-operation?

@jmarshallnz
Team Kodi member
jmarshallnz added a note Mar 3, 2014

We're on appthread here, so yes, it's safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz merged commit 20fcb22 into xbmc:master Mar 3, 2014

1 check passed

Details default Merged build #307 succeeded in 1 hr 6 min
@amet amet commented on the diff Apr 10, 2014
xbmc/video/dialogs/GUIDialogSubtitles.cpp
@@ -72,6 +73,8 @@ class CSubtitlesJob: public CJob
virtual bool DoWork()
{
CDirectory::GetDirectory(m_url.Get(), *m_items);
+ // Sort items by path so they properly order for eg. stacks
+ m_items->Sort(SortByPath, SortOrderAscending);
@amet
amet added a note Apr 10, 2014

@arnova this is not really useful in any other case except stacks.

what we have done in services is to sort them by "sync" status if it exists, but with this its all thrown out of wack :)

can you think of a different way we can handle it?

@arnova
Team Kodi member
arnova added a note Apr 12, 2014

@amet Good point but not sure about how to fix it, you have any idea whether stacked items are already in order from eg. OpenSubtitles?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@phate89
phate89 commented Apr 13, 2014

Is this patch going to allow multiple subtitles even if they are not stacked files? Because there are subtitle services (like itasa that i develope) that sometimes have more than one version in a single zip file and right now i can pass every subtitle but only the first one is considered and there's no way to pass also the 2nd one..

@amet
amet commented Apr 13, 2014

@arnova

We make sure service sorts it if it's stacked , or let service dictate who should sort. Let service return sorted=true or some such.

Also, we can pass more than one subtitle back to xbmc to handle if movie is stack if it's not stack we only use first one.

@arnova arnova deleted the arnova:sub_stack_handling branch Nov 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.