Fix for downloading subtitles for http videos #4367

Merged
merged 2 commits into from Mar 13, 2014

Conversation

Projects
None yet
6 participants
@arnova
Member

arnova commented Mar 7, 2014

Should fix problems reported here:

Also improved logging a bit to make spotting issues a little easier and removed the gui setting to configure the custom subtitle path.

@jmarshallnz

View changes

xbmc/video/dialogs/GUIDialogSubtitles.cpp
CStdString strDownloadPath = "special://temp";
CStdString strDestPath;
std::vector<CStdString> vecFiles;
CStdString strCurrentFilePath = URIUtils::GetDirectory(strCurrentFile);
- if (StringUtils::StartsWith(strCurrentFilePath, "http://"))
+ if (StringUtils::StartsWith(strCurrentFilePath, "http://") && !URIUtils::HasExtension(strCurrentFile, g_advancedSettings.m_videoExtensions))

This comment has been minimized.

@jmarshallnz

jmarshallnz Mar 7, 2014

Member

Why?? We can't store these on the http server.

@jmarshallnz

jmarshallnz Mar 7, 2014

Member

Why?? We can't store these on the http server.

This comment has been minimized.

@arnova

arnova Mar 8, 2014

Member

@jmarshallnz : Because afaik this block is only here for streams without an actual filename (/extension). http will be checked once again down below by CUtil::SupportsWriteFileOperations(strCurrentFilePath)). By changing this at least items like http://server/movie.mp4 get their subtitle stored in the custom subtitle folder as eg. movie.srt so it can be reused the next time a user plays the video.

Btw. I think we should drop the http check altogether here and only check the extension.

@arnova

arnova Mar 8, 2014

Member

@jmarshallnz : Because afaik this block is only here for streams without an actual filename (/extension). http will be checked once again down below by CUtil::SupportsWriteFileOperations(strCurrentFilePath)). By changing this at least items like http://server/movie.mp4 get their subtitle stored in the custom subtitle folder as eg. movie.srt so it can be reused the next time a user plays the video.

Btw. I think we should drop the http check altogether here and only check the extension.

This comment has been minimized.

@jmarshallnz

jmarshallnz Mar 8, 2014

Member

Your change would make sense if you dropped to only check extension, yes.

@jmarshallnz

jmarshallnz Mar 8, 2014

Member

Your change would make sense if you dropped to only check extension, yes.

@amet

This comment has been minimized.

Show comment
Hide comment
@amet

amet Mar 8, 2014

Contributor

Icefilms won't let us

Contributor

amet commented Mar 8, 2014

Icefilms won't let us

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 8, 2014

Member

@amet: Not sure what you mean

Member

arnova commented Mar 8, 2014

@amet: Not sure what you mean

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 8, 2014

Member

@jmarshallnz : Updated

Member

arnova commented Mar 8, 2014

@jmarshallnz : Updated

@amet

This comment has been minimized.

Show comment
Hide comment
@amet

amet Mar 8, 2014

Contributor

@jmarshallnz said "we can't upload to http:" ... It's a joke without a smiley at the end :)

Contributor

amet commented Mar 8, 2014

@jmarshallnz said "we can't upload to http:" ... It's a joke without a smiley at the end :)

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 8, 2014

Member

@amet: Ah yes, you meant: won't let us yet ;-)

Member

arnova commented Mar 8, 2014

@amet: Ah yes, you meant: won't let us yet ;-)

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 9, 2014

Member

jenkins build this please

Member

arnova commented Mar 9, 2014

jenkins build this please

@arnova arnova added this to the Gotham13.0-beta2 milestone Mar 10, 2014

@mkorganashvili

This comment has been minimized.

Show comment
Hide comment
@mkorganashvili

mkorganashvili Mar 11, 2014

When it's going to be merged?

When it's going to be merged?

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Mar 11, 2014

Member

as soon as the settings change is resolved

Member

da-anda commented Mar 11, 2014

as soon as the settings change is resolved

@mkorganashvili

This comment has been minimized.

Show comment
Hide comment
@mkorganashvili

mkorganashvili Mar 11, 2014

is not it possible to merge only subtitle download fix?!

is not it possible to merge only subtitle download fix?!

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Mar 11, 2014

Member

it is if @arnova is splitting up the change to another PR. But this is not really needed, because both fixes have to end up in Gotham anyways - so have patience. And now please stop further non technical/development related questions here on github, because every comment in here will trigger an email notification to ALL XBMC devs - and there already is way to much github spam in our inboxes.

Member

da-anda commented Mar 11, 2014

it is if @arnova is splitting up the change to another PR. But this is not really needed, because both fixes have to end up in Gotham anyways - so have patience. And now please stop further non technical/development related questions here on github, because every comment in here will trigger an email notification to ALL XBMC devs - and there already is way to much github spam in our inboxes.

@t-nelson

This comment has been minimized.

Show comment
Hide comment
@t-nelson

t-nelson Mar 11, 2014

Contributor

is not it possible to merge only subtitle download fix?!

Please be patient and trust our judgment. We only use github for development to reduce inbox flooding. Please keep non-technical discussion to the forums in the future.

Contributor

t-nelson commented Mar 11, 2014

is not it possible to merge only subtitle download fix?!

Please be patient and trust our judgment. We only use github for development to reduce inbox flooding. Please keep non-technical discussion to the forums in the future.

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Mar 11, 2014

Member

@arnova: Can you please split out the first two commits (improved logging, fix for http:// subs) so we can get those in. I have a feeling the setting thing might take a bit longer than we may want to wait for B2.

Member

jmarshallnz commented Mar 11, 2014

@arnova: Can you please split out the first two commits (improved logging, fix for http:// subs) so we can get those in. I have a feeling the setting thing might take a bit longer than we may want to wait for B2.

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 12, 2014

Member

@jmarshallnz : You mean a seperate PR for the rest of the commits?

Member

arnova commented Mar 12, 2014

@jmarshallnz : You mean a seperate PR for the rest of the commits?

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 13, 2014

Member

@jmarshallnz : Can't we just merge this as a whole? The only remaining issue are the spinner-texts right?

Member

arnova commented Mar 13, 2014

@jmarshallnz : Can't we just merge this as a whole? The only remaining issue are the spinner-texts right?

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Mar 13, 2014

Member

A separate PR for the 2 commits I mentioned. The settings removal touches a bunch more code so will take longer to review. It would be nice to get the fixes we know about in so we can push B2 out.

Member

jmarshallnz commented Mar 13, 2014

A separate PR for the 2 commits I mentioned. The settings removal touches a bunch more code so will take longer to review. It would be nice to get the fixes we know about in so we can push B2 out.

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 13, 2014

Member

@jmarshallnz : Will do ASAP. Shouldn't it include arnova/xbmc@e9e909d as well?

Member

arnova commented Mar 13, 2014

@jmarshallnz : Will do ASAP. Shouldn't it include arnova/xbmc@e9e909d as well?

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 13, 2014

Member

@jmarshallnz Removed settings commit and moved to seperate PR #4411 . Let me know if you need anything else done.

Member

arnova commented Mar 13, 2014

@jmarshallnz Removed settings commit and moved to seperate PR #4411 . Let me know if you need anything else done.

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Mar 13, 2014

Member

No, it shouldn't include the change from http:// -> unknown extension. That again is something that needs more thought than the simple fix.

Member

jmarshallnz commented Mar 13, 2014

No, it shouldn't include the change from http:// -> unknown extension. That again is something that needs more thought than the simple fix.

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Mar 13, 2014

Member

@jmarshallnz : Done, moved the commit to the other PR as well.

Member

arnova commented Mar 13, 2014

@jmarshallnz : Done, moved the commit to the other PR as well.

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Mar 13, 2014

Member

Thanks! jenkins build this please

Member

jmarshallnz commented Mar 13, 2014

Thanks! jenkins build this please

@jmarshallnz jmarshallnz added the Gotham label Mar 13, 2014

jmarshallnz added a commit that referenced this pull request Mar 13, 2014

Merge pull request #4367 from arnova/more_sub_fixes
Fix for downloading subtitles for http videos

@jmarshallnz jmarshallnz merged commit 52d498e into xbmc:master Mar 13, 2014

1 check passed

default Merged build #369 succeeded in 1 hr 15 min
Details

jmarshallnz added a commit that referenced this pull request Mar 14, 2014

Merge pull request #4367 from arnova/more_sub_fixes
Fix for downloading subtitles for http videos

@jmarshallnz jmarshallnz removed the Gotham label Mar 14, 2014

@arnova arnova deleted the arnova:more_sub_fixes branch Nov 7, 2014

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