Added Dialogs for direct Audiostream and Subtitle switching #16825

lobermann · 2019-10-24T19:42:11Z

Description

Mentioned this in one of the last PRs I made. This Adds two new dialogs that allow direct switching of audiostream and subtitle.

Motivation and Context

In order not to have to always navigate through the menu if you want to switch the audiostream or subtitle, this two dialogs can be called directly from the OSD and allow quick and easy change of them. (Similar to how it is on netflix for example).
Looking forward for feedback. Might not find it's way into Estuary (as we will not add any new buttons there) but there might be other skins interested in using it.

How Has This Been Tested?

Tested on MacOS and LibreElec.

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

lrusak · 2019-10-24T20:20:33Z

Would you mind adding some screenshots?

ksooo

This needs a bit more love and I'm not sure the implementation is correct at all. Implementing a dialog which does nothing than to open another dialog from its OnInitWindowoverride looks strange.

ksooo · 2019-10-24T20:57:11Z

xbmc/guilib/GUIWindowManager.cpp

@@ -241,6 +243,8 @@ void CGUIWindowManager::CreateWindows()

  Add(new CGUIDialogMediaFilter);
  Add(new CGUIDialogSubtitles);
+  Add(new CGUIDialogSubtitleSelect);
+  Add(new CGUIDialogAudioSelect);


Destruction of the new dialog instances inCGUIWindowManager::DestroyWindows() is missing.

ksooo · 2019-10-24T21:02:05Z

xbmc/video/dialogs/GUIDialogAudioSelect.cpp

+#include "utils/URIUtils.h"
+#include "utils/Variant.h"
+#include "utils/log.h"
+#include "video/VideoDatabase.h"


I doubt that all these headers are actually needed. Example: utils/log.h included, but I cannot see any use of CLog

ksooo · 2019-10-24T21:03:25Z

xbmc/video/dialogs/GUIDialogAudioSelect.cpp

+  if (dialog == NULL)
+    return false;
+
+  CFileItemList options;


I would define vars where they are actually first used.

ksooo · 2019-10-24T21:04:09Z

xbmc/video/dialogs/GUIDialogAudioSelect.cpp

+  CGUIDialogSelect* dialog =
+      CServiceBroker::GetGUI()->GetWindowManager().GetWindow<CGUIDialogSelect>(
+          WINDOW_DIALOG_SELECT);
+  if (dialog == NULL)


if (dialog == nullptr) or if (!dialog)

ksooo · 2019-10-24T21:06:40Z

xbmc/video/dialogs/GUIDialogAudioSelect.cpp

+  for (int i = 0; i < audioStreamCount; ++i)
+  {
+    std::string strItem;
+    std::string strLanguage;


I would define vars where they are actually first used.

ksooo · 2019-10-24T21:18:43Z

xbmc/video/dialogs/GUIDialogSubtitleSelect.cpp

+    if (!g_LangCodeExpander.Lookup(info.language, strLanguage))
+      strLanguage = g_localizeStrings.Get(13205); // Unknown
+
+    if (info.name.length() == 0)


if (info.name.empty())

ksooo · 2019-10-24T21:19:57Z

xbmc/video/dialogs/GUIDialogSubtitleSelect.cpp

+  CGUIDialogSelect* dialog =
+      CServiceBroker::GetGUI()->GetWindowManager().GetWindow<CGUIDialogSelect>(
+          WINDOW_DIALOG_SELECT);
+  if (dialog == NULL)


if (dialog == nullptr) or if (!dialog)

ksooo · 2019-10-24T21:20:35Z

xbmc/video/dialogs/GUIDialogSubtitleSelect.cpp

+    else
+      strItem = StringUtils::Format("%s - %s", strLanguage.c_str(), info.name.c_str());
+
+    CFileItemPtr item(new CFileItem(strItem.c_str()));


std::make_shared<CFileItem>(strItem);

ksooo · 2019-10-24T21:23:06Z

xbmc/video/dialogs/GUIDialogAudioSelect.cpp

+
+void CGUIDialogAudioSelect::OnInitWindow()
+{
+  ShowAudioSelect();


Are you sure it is correct to show the dialog in OnInitWindow - while the dialog is not yet fully initialized? Did you copy this pattern from an existing dialog implementation?

ksooo · 2019-10-24T21:23:28Z

xbmc/video/dialogs/GUIDialogSubtitleSelect.cpp

+
+void CGUIDialogSubtitleSelect::OnInitWindow()
+{
+  ShowSubtitleSelect();


Are you sure it is correct to show the dialog in OnInitWindow - while the dialog is not yet fully initialized? Did you copy this pattern from an existing dialog implementation?

da-anda · 2019-10-27T09:36:59Z

not against such a dialog in general, but aren't skins already able to add such a dialog/feature using currently available infolabels etc? I remember the balloon OSD popup in Confluence that had a similar feature.

And since you mentined Netflix. Netflix uses a combined view to switch audio and subtitle language which I actually like quite a lot. What was your underlying idea for going with two separate dialogs instead of having something like a LanguageSelectDialog (or however we might call that) that has both?

Rechi · 2019-10-28T18:35:49Z

xbmc/guilib/GUIWindowManager.cpp

@@ -127,6 +127,8 @@
 #include "dialogs/GUIDialogPlayEject.h"
 #include "dialogs/GUIDialogMediaFilter.h"
 #include "video/dialogs/GUIDialogSubtitles.h"
+#include "video/dialogs/GUIDialogSubtitleSelect.h"
+#include "video/dialogs/GUIDialogAudioSelect.h"


wrong include order

jenkins4kodi · 2020-05-30T09:13:42Z

@lobermann this needs a rebase

lobermann · 2022-04-03T13:48:49Z

@da-anda @ksooo to follow up on my open PR's, I guess we are not going to merge this one as it is and also it seems that this functionality is also achievable just within the skin. So I guess we can close this?

da-anda · 2022-04-04T16:03:25Z

strictly by looking at the code, I can't really say if it makes sense to add these dialogs or not. I know for certain that you can cycle through subs and audio streams via builtins that can be triggered by the skin in any way it likes. I however am not sure if skins are currently able to get a list of all available streams/languages for subs, audio tracks and video (in case of multi video stream containers) and then directly select a specific one. This in fact might be a nice addition to have (in case it's currently not yet possible), but I'm not sure if the best/only way to do this will be a new dialog that skins MUST add since it will be new core UI element/dialog that way, or if it would somehow be possible achieve this just by using infolabels and builtins, so that could integrate it in whatever form they like (new dialog or just via a menu that is tightly integrated into the OSD, like in Netflix).

@ronie do you know out of the top of your head if a feature like this would be possible via just infolabels and builtins?

luzpaz · 2024-03-02T22:42:47Z

any traction here ?

Hitcher · 2024-03-03T10:08:16Z

Having the ability to open these dialogs directly is a positive imo.

jjd-uk · 2024-03-03T10:54:44Z

Agreed it would be nice for the skin to be able to directly call the audio and subtitle selection dialogs. For this skins need window id's to which they can map buttons to open the selection dialog which I presume this PR adds.

This would allow an improved version of #22557 where the toggle buttons are replaced by buttons opening the selection dialogs.

Added Dialogs for direct Audiostream and Subtitle switch

8362038

lobermann added the Type: Feature non-breaking change which adds functionality label Oct 24, 2019

ksooo requested changes Oct 24, 2019

View reviewed changes

Rechi reviewed Oct 28, 2019

View reviewed changes

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label May 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Dialogs for direct Audiostream and Subtitle switching #16825

Added Dialogs for direct Audiostream and Subtitle switching #16825

lobermann commented Oct 24, 2019

lrusak commented Oct 24, 2019

ksooo left a comment •

edited

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

ksooo Oct 24, 2019

da-anda commented Oct 27, 2019 •

edited

Rechi Oct 28, 2019

jenkins4kodi commented May 30, 2020

lobermann commented Apr 3, 2022

da-anda commented Apr 4, 2022

luzpaz commented Mar 2, 2024

Hitcher commented Mar 3, 2024

jjd-uk commented Mar 3, 2024 •

edited

Added Dialogs for direct Audiostream and Subtitle switching #16825

Are you sure you want to change the base?

Added Dialogs for direct Audiostream and Subtitle switching #16825

Conversation

lobermann commented Oct 24, 2019

Description

Motivation and Context

How Has This Been Tested?

Types of change

Checklist:

lrusak commented Oct 24, 2019

ksooo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-anda commented Oct 27, 2019 • edited

Choose a reason for hiding this comment

jenkins4kodi commented May 30, 2020

lobermann commented Apr 3, 2022

da-anda commented Apr 4, 2022

luzpaz commented Mar 2, 2024

Hitcher commented Mar 3, 2024

jjd-uk commented Mar 3, 2024 • edited

ksooo left a comment •

edited

da-anda commented Oct 27, 2019 •

edited

jjd-uk commented Mar 3, 2024 •

edited