Move subtitle addon core handling to core. #3552

jmarshallnz · 2013-11-03T00:47:21Z

This is primarily aimed at taking away some of the load from @amet at having to handle 1249591 subtitle services.

It moves the UI portion of the xbmc subtitles script into core, and has the services run as plugins from there. This will allow (in future) for better integration of downloading of subtitles. For now it's still a little clunky in that downloading of subtitles is still somewhat separate from embedded or external subs already on disk. Nonetheless, it's a starting point and IMO important to get that starting point in so that the subtitle service guys can get things done (it's a minor change for them) and so that skinners can get their changes done.

If anyone has any ideas on how it could be better integrated in the future, then that's cool - please discuss on forums to keep this focused on code discussion.

It does add a new window for skinners to deal with, but most already have it in place as most support the subtitles script. Their existing window can be transferred over just by renaming the file in most cases.

@Montellese: It relies on multi-select list settings, so a solution to that needs to happen first. Note that all it needs is a comma separated list of languages, so we don't actually need any special retrieving of a multi-select setting pre-broken down (as we'll just stuff it back together again).

jmarshallnz · 2013-11-03T00:53:33Z

jenkins build this please

language/English/strings.po

-#empty strings from id 24104 to 24999
+msgctxt "#24104"
+msgid "Save subtitles to movie folder"
+msgstr ""


ronie · 2013-11-05T21:56:58Z

a thing i noticed is, xbmc is not settings focus to the list.
(can't do it in the skin as the list is still empty on windowopen)

i've updated DialogSubtitles.xml to use the new positioning goodies:
ronie@ab987f6

arnova · 2013-11-06T15:52:19Z

Overall looking good. I assume the stack handling is still on the todo-list?

jmarshallnz · 2013-11-06T19:31:15Z

not on my list short-term, so if you could adapt what you have that might be useful.

arnova · 2013-11-07T06:25:59Z

Well I don't have anything yet beside "ideas" but this is certainly something I'd be willing to work on. When will this hit mainline?

jmarshallnz · 2013-11-07T06:42:47Z

It's reliant on the list settings stuff, so towards the end of the merge window.

jmarshallnz · 2013-11-11T08:02:27Z

@amet rebased up with @Montellese' list[string] implementation. Please test.

system/settings/settings.xml

+            <delimiter>,</delimiter>
+            <minimum>1</minimum>
+          </constraints>
+          <control type="list" format="string" />


jmarshallnz · 2013-11-11T08:24:38Z

jenkins build this please

system/settings/settings.xml

+          <constraints>
+            <options>languages</options>
+            <delimiter>,</delimiter>
+            <minimum>1</minimum>


amet · 2013-11-11T15:16:05Z

@jmarshallnz I will try to test tonight to make sure all is ok with the service. are we ok to clean it up in the upcoming merge window before Gotham goes into freeze window or is what we have here final?

we should really take care of default services and stack movies, as much as I hate them :)

EDIT: just realized we missed merge window, didn't see it was 11th.. so we have some time to polish it and shove in in December.

jmarshallnz · 2013-11-11T19:00:36Z

Fixes are fine after the fact. And this is ready to go in - it was reviewed during merge window.

jmarshallnz · 2013-11-11T22:58:48Z

@amet: rebased from recent flurry of commits. Ready for your testing.

jenkins build this please

Move subtitle addon core handling to core.

xhaggi · 2013-11-12T09:07:05Z

@jmarshallnz what do you think about replacing the country flag icons with these?
http://www.customicondesign.com/free-icons/flag-icon-set/all-in-one-country-flag-icon-set/

amet · 2013-11-12T09:08:31Z

where were you 3 minutes ago??

EDIT: anyway its a skin thing, Jezz_X/ronie/you or whoever wants to change it is more than welcome to it

xhaggi · 2013-11-12T09:11:36Z

i can add a PR with the new ones, if you want?

jmarshallnz · 2013-11-12T09:15:20Z

They don't appear to be freely distributable?

xhaggi · 2013-11-12T09:34:28Z

yeah you are right .. will look for some other ones.

xhaggi · 2013-11-12T09:39:26Z

https://www.gosquared.com/resources/flag-icons/

Usage:
  You're free to use this set for both personal and commercial projects
  (MIT License, see LICENSE.txt), and a back-link is not required
  (but certainly appreciated).

are they okay?

As discussed in xbmc#3552, this replace the subtitle flag icons with new 32x32 pixel icons from https://www.gosquared.com/resources/flag-icons/ This also fixes some wrong flags like uk. It's Ukrainian not United Kingdom.

MartijnKaijser · 2013-11-18T17:47:31Z

@jmarshallnz
it seems that the skin uses the old rectangular logos from the service addons which are now the icon.png
Rectangular and transparent icon.png are not allowed according to our repo guidelines (and i'm not going to make exceptions). So the service provider logo should get another .png filename.

Edit: fixed typos.

jmarshallnz · 2013-11-18T19:01:13Z

They'll need icon.png for the add-ons manager I guess? If so, mayaswell make 'em square in the UI as well I should think - no point having two logos?

MartijnKaijser · 2013-11-18T19:05:48Z

indeed for addon manager.
well rectangle ones look a bit better in the subtitle dialog than square ones i think.

jmarshallnz · 2013-11-18T19:06:44Z

That might just be a skin thing though?

@amet: any opinion?

MartijnKaijser · 2013-11-18T21:04:08Z

For reference Wunderground addon has it like in the resource folder
https://github.com/XBMC-Addons/weather.wunderground/blob/master/resources/logo/logo.jpg

taxigps · 2013-12-03T14:06:41Z

@jmarshallnz no way to downlaod sub/idx subtitles now?

jmarshallnz · 2013-12-03T19:08:49Z

That's a question for @amet

xbmc/video/dialogs/GUIDialogSubtitles.cpp

+  CStdString strSubPath = URIUtils::AddFileToFolder(strDestPath, strSubName);
+
+  // and copy the file across
+  CFile::Cache(strUrl, strSubPath);


amet · 2013-12-04T18:02:21Z

@taxigps I have no way of testing that... if you have a service that downloads sub/idx let me have it and we can just check for idx if ext is sub. should not be too hard

amet · 2013-12-04T18:20:28Z

@jmarshallnz @taxigps something like this ? http://paste.ubuntu.com/6521191/

but as said earlier, I have nothing to test with... all done blind. it compiles though :)

taxigps · 2013-12-04T23:35:40Z

@amet yes, the patch is ok. my service here https://github.com/taxigps/service.subtitles.shooter. you can test it. but not all movie have sub/idx subtitle.

taxigps · 2013-12-05T03:07:19Z

@amet two problem of your patch.

URIUtils::ReplaceExtension(strUrl,".idx"); not work, need change to strUrl = URIUtils::ReplaceExtension(strUrl,".idx");
idx/sub subtitle can't load after cache. and it can load when restart the movie play.

taxigps · 2013-12-05T03:18:00Z

@amet I have fixed the problem of the patch, and it works well in my test.

new patch here: http://paste.ubuntu.com/6523036/

amet · 2013-12-05T07:41:16Z

@taxigps, feel free to make PR or push in as fix if you tested it... I won't be able test for few days

MartijnKaijser reviewed Nov 3, 2013
View reviewed changes

language/English/strings.po

#empty strings from id 24104 to 24999

msgctxt "#24104"

msgid "Save subtitles to movie folder"

msgstr ""

This comment was marked as spam.

Sign in to view

jmarshallnz mentioned this pull request Nov 6, 2013

changed: Extend Python/CApp GetCurrentFile to obtain Player's unstacked file #3440

Closed

Montellese reviewed Nov 11, 2013
View reviewed changes

system/settings/settings.xml

<delimiter>,</delimiter>

<minimum>1</minimum>

</constraints>

<control type="list" format="string" />

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

amet reviewed Nov 11, 2013
View reviewed changes

Jonathan Marshall and others added 5 commits November 12, 2013 21:49

CAddonsDirectory::FileItemFromAddon should take a const AddonPtr &

a520c65

replace xbmc.python.subtitles extension with xbmc.subtitle.module one

1a27525

[subtitles] adds new dialog for subtitles to core.

bc7fb1f

[confluence] changes for new subtitles dialog

4baf2c0

[subtitles] set a maximum of 3 languages.

ef87af6

amet added a commit that referenced this pull request Nov 12, 2013

Merge pull request #3552 from jmarshallnz/SubsCore

8dc1825

Move subtitle addon core handling to core.

amet merged commit 8dc1825 into xbmc:master Nov 12, 2013

jmarshallnz deleted the SubsCore branch November 12, 2013 09:04

xhaggi mentioned this pull request Nov 12, 2013

[confluence] replace subtitle flag icons with new ones #3629

Merged

taxigps reviewed Dec 3, 2013
View reviewed changes

xbmc/video/dialogs/GUIDialogSubtitles.cpp

CStdString strSubPath = URIUtils::AddFileToFolder(strDestPath, strSubName);

// and copy the file across

CFile::Cache(strUrl, strSubPath);

This comment was marked as spam.

Sign in to view

taxigps mentioned this pull request Dec 5, 2013

[subtitles] check and get idx/sub pair when download. #3768

Merged

Move subtitle addon core handling to core. #3552

Move subtitle addon core handling to core. #3552

Conversation

jmarshallnz commented Nov 3, 2013

jmarshallnz commented Nov 3, 2013

This comment was marked as spam.

ronie commented Nov 5, 2013

arnova commented Nov 6, 2013

jmarshallnz commented Nov 6, 2013

arnova commented Nov 7, 2013

jmarshallnz commented Nov 7, 2013

jmarshallnz commented Nov 11, 2013

This comment was marked as spam.

This comment was marked as spam.

jmarshallnz commented Nov 11, 2013

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

amet commented Nov 11, 2013

jmarshallnz commented Nov 11, 2013

jmarshallnz commented Nov 11, 2013

xhaggi commented Nov 12, 2013

amet commented Nov 12, 2013

xhaggi commented Nov 12, 2013

jmarshallnz commented Nov 12, 2013

xhaggi commented Nov 12, 2013

xhaggi commented Nov 12, 2013

MartijnKaijser commented Nov 18, 2013

jmarshallnz commented Nov 18, 2013

MartijnKaijser commented Nov 18, 2013

jmarshallnz commented Nov 18, 2013

MartijnKaijser commented Nov 18, 2013

taxigps commented Dec 3, 2013

jmarshallnz commented Dec 3, 2013

This comment was marked as spam.

amet commented Dec 4, 2013

amet commented Dec 4, 2013

taxigps commented Dec 4, 2013

taxigps commented Dec 5, 2013

taxigps commented Dec 5, 2013

amet commented Dec 5, 2013