Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Estuary] Replace subtitle menu shortcut by a Media tracks selection menu #22557

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mjibe
Copy link

@mjibe mjibe commented Jan 22, 2023

Description

Replaces the "Subtitles settings" shortcut on OSD Video Bar with a new menu called "Mediatrack selection".
This is a new redux menu directly on OSDBar which shows these options :

  • Enable/Disable subtitle [switch]
  • Change sub track
  • Change audio track
  • Change video track (designed only for specific movies which have Color or B/W tracks)

It also add a new "case" in xbmc\guilib\guiinfo\VideoGUIInfo.cpp to be able to retrieve the video stream count on theme's xml.

All of these options are disabled if each related track type hasn't at least one occurrence (two for audio and video).
E.g. : No subtitle track will disable the first two entries as usual.

It also comes with a new OSD icon for the menu, that may or may not be suitable (I didn't come with a better idea).

Motivation and context

As both Windows user and Android TV user, it seemed to me quite easy on Windows to change subs, audiotrack or even videotrack (with key shortcuts), but very click-consuming on androidTV.

As there was a huge Subtitle menu (allowing delaying, disabling, etc.), then aside audio and subtitles selection in OSD settings menu, but no video selection, it looked more practical to have access to a redux menu directly on OSDBar which shows :

  • Enable/Disable subtitle [switch]
  • Toggle subtitle
  • Toggle audio stream
  • Toggle video stream (designed only for specific movies which have, for example, Color and B/W tracks like screenshots below)

Then leaves the delay/download settings, and so on, on the associated "Settings menu", as nowadays subtitles are quite often already embedded and synced.

How has this been tested?

I forked estuary skin from kodi repo, and developed it from another custom Dialog basis.
Aside from deactivating Video stream toggle instruction, all of this has been tested since 2 years ago, on an almost daily basis.
Original post on kodi forum

What is the effect on users?

This change will replace the existing "subtitle menu" shortcut on OSDBar with a new AiO redux menu allowing to switch all streams (subs/audio/video) in one menu, showing these options :

  • Enable/Disable subtitle [switch]
  • Change sub track
  • Change audio track
  • Change video track (designed only for specific movies which have Color or B/W tracks)

Consequently, it centralizes all streams toggling, moving hence the audio and subs selection (from estuary "OSD settings menu") to this one.
It also adds ability to switch video tracks (helpful for particular movies releases which have Colored and B/W video streams).

[Non visible to users] It also exposes the StreamVideoCount property outside the core.

Screenshots (if appropriate):

Track selection 1
Track selection 2

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 **<== No test touching CVideoGUIInfo::GetInt method or the VideoPlayer infomap **
  • All new and existing tests passed

@CastagnaIT
Copy link
Collaborator

CastagnaIT commented Jan 23, 2023

thanks for your proposal
i think is a good thing decouple the setup menu

i also think that if possible we could try remove the dialog window, and shown a popup menu? that automatically shown when the new "configuration" icon is selected, like other video players do, this will remove another GUI step and also imo will looks more user friendly, ofc questionable,
my concept example:
immagine

for this point:

Change video track (designed only for specific movies which have Color or B/W tracks)

having multiple video tracks have different purposes, like multi-angle videos blueray,
but i think most of time this could be used with adaptive video streams, so media streams that have multiple bitrates,
could be more appreciated to know what bitrate is currently being played instead of nothing,
something like:
immagine
maybe if bitrate is not found show the track name? I am not fully sure need more feedbacks

another problem to take in account, that especially with VOD services,
services provide many languages of audio / subtitles tracks (also more than 5/10 languages which can double depending on the type of features) this lead to become crazy to click on "Toggle subtitle" to find the right language and type (e.g. sub forced or other), therefore this new dialog menu becomes useless,
so in this new menu there is no way to have an easy access to select the language as a list
maybe (questionable) we should start remove these "toggle" (for next track) and show only menu for language lists

@enen92
Copy link
Member

enen92 commented Jan 23, 2023

iirc this was done at the time to reuse already existing dialogs and save skinners the effort to implement yet another dialog. However having a skin defined custom dialog only in the scope of estuary should not hurt though. Feedback of skinners is appreciated.

@mjibe
Copy link
Author

mjibe commented Jan 26, 2023

@CastagnaIT Hi, thanks for your feedback!

The reason why I kept it as a menu instead of popup was to keep Estuary skin's overall consistency. I didn't see any "popup like" display on it (at least on OSD).

Regarding functionalities (Toggles), for the same reasons, I kept them as they already are (aside from adding video Toggle, with the same behaviour). Then I simply regrouped them in a dedicated (and simple) window. Leaving advanced features in the dedicated settings menus.

Of course, your suggestions can be implemented, but I would require more feedbacks before doing it.

Thanks again !

@jjd-uk
Copy link
Member

jjd-uk commented Jan 26, 2023

I like it but not sure when I'll have time to test.

The reason why I kept it as a menu instead of popup was to keep Estuary skin's overall consistency. I didn't see any popup like display on it (at least on OSD).

I agree with this, a full dialog fits better with what we have already for consistency.

another problem to take in account, that especially with VOD services,
services provide many languages of audio / subtitles tracks (also more than 5/10 languages which can double depending on the type of features) this lead to become crazy to click on "Toggle subtitle" to find the right language and type (e.g. sub forced or other), therefore this new dialog menu becomes useless,

I don't see why. There's the Subtitles section in Settings if you want the full list, besides there will still be an activity saving for the user.

Counting actions required with this new Track settings dialog:

  1. Open tracks menu - 1 select action
  2. Select Toggle subtitle language - 1 down action
  3. Select subtitle 10th/10 - 9 select actions
  4. Close dialog - 1 back action
    Total = 12 actions

Counting using existing Subtitle settings:

  1. Open Settings menu - 1 select action
  2. Open Subtitle settings - 1 select action
  3. Highlight subtitle language selection - 2 down actions
  4. Open selection list - 1 select action
  5. Highlight wanted lanuage 10th/10 - 9 down actions
  6. Select language - 1 select action
  7. Exit list - 1 back action
  8. Exit Settings - 2 back actions
    Total = 18 actions

So potentially a signifcant saving for the majority of users.

@CastagnaIT
Copy link
Collaborator

CastagnaIT commented Jan 26, 2023

I do not understand its usefulness of toggles have more cons that pro

the user cannot always know which languages are included on a stream or file
so having a toggle, means that the user is forced to toggle through all languages tracks in all formats (forced/caption/description/channels 2.0/channels 5.1/ etc...) if there are many tracks this is very inconvenient

So, if you do not find the right track because it is not available, you should not be forced to toggle tracks all the time

moreover, if you makes a mistake in stopping and presses button more time (can happens with a remote)
you must again make a complete loop of all tracks to find the right one

IMO toggle button cannot be called as simplified user experience,
otherwise both methods must be provided in some way in this new menu
so also allow select track by using track list

if we are to change a little the menus, we should better improve this menu section

@jjd-uk
Copy link
Member

jjd-uk commented Jan 26, 2023

This is meant as a shortcut route for quick selection, using a list in my view just adds more unnecessary actions back into the flow. Doesn't really matter to me however, as I'm using my own skin with toggle buttons directly on OSD.

[edit]
Looking the list of Action ID's and Window ID's I'm not sure opening the subtitle selection dialog from a button is even possible from anywhere other than Subtitle Settings.

@mjibe
Copy link
Author

mjibe commented Jan 29, 2023

[edit] Looking the list of Action ID's and Window ID's I'm not sure opening the subtitle selection dialog from a button is even possible from anywhere other than Subtitle Settings.

Indeed, this list seems to be dynamically generated, building the "user's window" by filling an empty "DialogSettings" view with what was retrieved on C++ side (same behaviour for OSDAudio/OSDVideo/OSDSubtitles).

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Nov 22, 2023
@jenkins4kodi
Copy link
Contributor

@mjibe this needs a rebase

@hugbug
Copy link
Contributor

hugbug commented Dec 9, 2023

Please do not remove the existing button for subtitles dialog. This dialog is extremely useful. As @CastagnaIT pointed out the toggles to choose subtitles are bad UI experience. The abilities to sync or download subtitles are also the functions I use quite often. Being forced to go into subtitles dialog through settings dialog is a step backwards in usability.

If anyone needs the new dialog proposed in this PR please make a separate button for it. I personally don't see this PR as an improvement. I'm sure the PR author doesn't use subtitles but I can assure him of the importance of the existing dialog.

@mjibe
Copy link
Author

mjibe commented Dec 23, 2023

Please do not remove the existing button for subtitles dialog. This dialog is extremely useful. As @CastagnaIT pointed out the toggles to choose subtitles are bad UI experience. The abilities to sync or download subtitles are also the functions I use quite often. Being forced to go into subtitles dialog through settings dialog is a step backwards in usability.

If anyone needs the new dialog proposed in this PR please make a separate button for it. I personally don't see this PR as an improvement. I'm sure the PR author doesn't use subtitles but I can assure him of the importance of the existing dialog.

You're wrong, I live by subtitles (or not) but the amount of times I needed to resync or download subs can be counted on one hand's fingers.

On the other hand, the amount of times I needed to switch fast between languages (both audios and subs) because someone in my house didn't understand foreign languages to share something I watch... I don't have enough fingers.

Please don't assume my usage...

And for the time being, you could see in other comments that your POV is not absolute.

@hugbug
Copy link
Contributor

hugbug commented Dec 23, 2023

I agree with you that subtitle sync is rarely needed and a quick switch between subtitles is important. However your PR does not make that easier but rather more difficult. The essential thing when switching between subtitles is to see all available languages to choose from. A simple toggle-to-next button doesn't make it easier with 10 or 20 different subtitles which is an often case. CastagnaIT explained in details why this is the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rebase needed PR that does not apply/merge cleanly to current base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants