[coreSubs] set default services for Tv Shows and Movies #3746

Merged
merged 1 commit into from Dec 3, 2013

Projects

None yet

2 participants

@amet
Contributor
amet commented Dec 1, 2013

this allows us to specify which subtitle service to use for tv Shows and which for movies. its handy to have as some services are TV only and will return no subs for movies.

@jmarshallnz please check

@jmarshallnz jmarshallnz commented on an outdated diff Dec 2, 2013
xbmc/video/dialogs/GUIDialogSubtitles.cpp
@@ -221,8 +222,31 @@ void CGUIDialogSubtitles::FillServices()
CGUIMessage msg(GUI_MSG_LABEL_BIND, GetID(), CONTROL_SERVICELIST, 0, 0, m_serviceItems);
OnMessage(msg);
- // TODO: Default service support will need to check through the items to find the CFileItem in the loop above.
- SetService(m_serviceItems->Get(0)->GetProperty("Addon.ID").asString());
+ CFileItemPtr fileItem;
+ fileItem = CFileItemPtr(new CFileItem(g_application.CurrentFileItem()));
@jmarshallnz
jmarshallnz Dec 2, 2013 Member

Use const CFileItem &item = g_application.CurrentFileItem();

@jmarshallnz jmarshallnz commented on an outdated diff Dec 2, 2013
xbmc/video/dialogs/GUIDialogSubtitles.cpp
@@ -221,8 +222,31 @@ void CGUIDialogSubtitles::FillServices()
CGUIMessage msg(GUI_MSG_LABEL_BIND, GetID(), CONTROL_SERVICELIST, 0, 0, m_serviceItems);
OnMessage(msg);
- // TODO: Default service support will need to check through the items to find the CFileItem in the loop above.
- SetService(m_serviceItems->Get(0)->GetProperty("Addon.ID").asString());
+ CFileItemPtr fileItem;
+ fileItem = CFileItemPtr(new CFileItem(g_application.CurrentFileItem()));
+
+ if (fileItem->GetVideoContentType() == VIDEODB_CONTENT_TVSHOWS ||
+ fileItem->GetVideoContentType() == VIDEODB_CONTENT_EPISODES)
+ // Set default service for tv shows
+ m_defaultService = CSettings::Get().GetString("subtitles.tv").c_str();
+ else
+ // Set default service for filemode and movies
+ m_defaultService = CSettings::Get().GetString("subtitles.movie").c_str();
@jmarshallnz
jmarshallnz Dec 2, 2013 Member

Don't use a member here. Also, don't use .c_str()?

@jmarshallnz jmarshallnz commented on an outdated diff Dec 2, 2013
xbmc/video/dialogs/GUIDialogSubtitles.cpp
+ m_defaultService = CSettings::Get().GetString("subtitles.movie").c_str();
+
+ // This ensures default service is activated, otherwise we fall back to first item in m_serviceItems
+ if (!ADDON::CAddonMgr::Get().IsAddonDisabled(m_defaultService))
+ {
+ for (int i = 0; i < m_serviceItems->Size(); i++)
+ {
+ if (m_serviceItems->Get(i)->GetProperty("Addon.ID") == m_defaultService)
+ {
+ SetService(m_serviceItems->Get(i)->GetProperty("Addon.ID").asString());
+ break;
+ }
+ }
+ }
+ else
+ SetService(m_serviceItems->Get(0)->GetProperty("Addon.ID").asString());
@jmarshallnz
jmarshallnz Dec 2, 2013 Member

I would do:

std::string addons.front()->ID();
for loop at ln 215...
{
  // create item as in for loop at ln 215
  if (addonIt->ID() == defaultService)
    service = (*addonIt)->ID();
}

...

SetService(service);

i.e. you needn't have the two loops.

@amet
Contributor
amet commented Dec 2, 2013

@jmarshallnz all done as discussed

@jmarshallnz jmarshallnz commented on the diff Dec 2, 2013
xbmc/video/dialogs/GUIDialogSubtitles.cpp
@@ -211,18 +212,31 @@ void CGUIDialogSubtitles::FillServices()
return;
}
+ std::string defaultService;
+ std::string service = addons.front()->ID();
+ const CFileItem &item = g_application.CurrentFileItem();
+
+ if (item.GetVideoContentType() == VIDEODB_CONTENT_TVSHOWS ||
+ item.GetVideoContentType() == VIDEODB_CONTENT_EPISODES)
+ // Set default service for tv shows
+ defaultService = CSettings::Get().GetString("subtitles.tv");
+ else
+ // Set default service for filemode and movies
+ defaultService = CSettings::Get().GetString("subtitles.movie");
+
@jmarshallnz
jmarshallnz Dec 2, 2013 Member

Purely cosmetic, but I'd move std::string service down to just above the for loop (reduce scope), and get rid of the empty line between const CFileItem &item line and the if().

Looks great though.

@amet
Contributor
amet commented Dec 2, 2013

@jmarshallnz see cf85880 please, not sure why that is not reflecting on this page

@jmarshallnz
Member

Looks good. Check it builds then knock it in.

@amet
Contributor
amet commented Dec 3, 2013

jenkins build this please

@amet amet merged commit d2e1b69 into xbmc:master Dec 3, 2013

1 check passed

default Merged build #665 succeeded in 1 hr 6 min
Details
@amet amet deleted the unknown repository branch Jan 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment