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

Investigate the possibility of better "Enable Subtitle" UX #410

Open
1 task
sp1ritCS opened this issue Apr 10, 2024 · 4 comments
Open
1 task

Investigate the possibility of better "Enable Subtitle" UX #410

sp1ritCS opened this issue Apr 10, 2024 · 4 comments

Comments

@sp1ritCS
Copy link
Contributor

As I've said in #408

Note: I'm unhappy with the UX of the enable subtitle button / checkbox

Currently the extra menu contains the "Subtitle" child menu that has a "Enabled" checkbox:
Bildschirmfoto vom 2024-04-10 12-48-28 Bildschirmfoto vom 2024-04-10 12-50-10

As a

  • Enabled

checkbox is bad UX, we should investigate if there is a better alternative.

Some suggestions:

  • Instead of a checkbox, have a "Enable" button that changes it's label to "Disable" after it has been pressed
  • Drop the checkbox and instead provide a "No subtitles" entry inside the subtitle stream selector
@Rafostar
Copy link
Owner

Rafostar commented Apr 10, 2024

checkbox is bad UX, we should investigate if there is a better alternative.

I agree on that, but this was done for now cause at the same time:

Instead of a checkbox, have a "Enable" button that changes it's label to "Disable" after it has been pressed

GTK does not give such functionality for popover menu items AFAIK. Depending how GActionEntry is set up it will either have just signal when clicked or a checkmark when it toggles some state (this is used here).

Drop the checkbox and instead provide a "No subtitles" entry inside the subtitle stream selector

GStreamer by design requires us to select a stream if there are any of such type. Whenever it will be processed is instead set by a flag on playbin. So selecting a "no subtitles" option would have to actually not change the selected stream but set a flag. Given this whole thing works on GListModel this would be a pain to implement and seems like a big hack already (since stream list would have to include something that is not a stream - it simply makes no sense).

The best idea I have for this currently, is simply to get rid of it and instead allow another click on a selected stream to unselect it and thus have nothing selected in the list. Note that as mentioned above, GTK implementation part would still have to remember and keep last selected stream index stored (since we cannot actually not select anything) and set a flag instead.

Not sure how this would turn out in code when implemented, but at least initially seems much less hacky then "no subtitles" selector. Well... also unsure if people would be aware that it can be "unclicked".

@Rafostar
Copy link
Owner

Rafostar commented Apr 10, 2024

The best idea I have for this currently, is simply to get rid of it and instead allow another click on a selected stream to unselect it and thus have nothing selected in the list. Note that as mentioned above, GTK implementation part would still have to remember and keep last selected stream index stored (since we cannot actually not select anything) and set a flag instead.

Well, seems like grouped radio buttons do not allow that 😢

And! One more thing to note here. Currently the "Enabled" button can be toggled on/off even if there are no subtitles in video and state is preserved when app is closed. This acts like a feature of "I do not want subtitles in any of my videos". Additionally, this helps working around a bug in playbin3 (not used by default yet) where it refuses to play a video with subtitles. It can be worked around by disabling that and restarting app, then you can play video although without subtitles (still might be better then nothing for some).

@Rafostar
Copy link
Owner

Would simply changing this text to "Show Subtitles" suffice perhaps? Nautilus (and probably other apps too) does menu like this and I think its better then "Enabled" which is even more mistaking as its past-tense.

image

Rafostar added a commit that referenced this issue Apr 12, 2024
As pointed in #410, the word "Enabled" and a checkmark next to it is a bad UX.
Change this text to "Show Subtitles" instead.
@sp1ritCS
Copy link
Contributor Author

Putting the discussion from matrix here for posterity:

@Rafostar:

sp1rit: What do we do with your "Enabled" issue? Change title/description? Close it? 🙃 I did not close it myself, since while its "certainly better than it is now" its probably not perfect to you. Although, doing anything else would be hard and probably very hacky too, since menu items are "immutable" objects.

@sp1ritCS:

I'm against closing it, as I think I still like my second proposed solution the best, even if that means not exposing the whole featureset of gstreamer to the user (as in the separate don't show subtitles toggle that gets saved across sessions).

I also don't think this has to be hacky. I've done a similar thing recently where I've wanted to always have one static element and a set of additional elements in a Gtk.DropDown by implementing a ListModel that just returned n+1 for the length and the k-1th element of the dynamic elements / the static element if k = 0. (But I've had the benefit of having the static element be of the same type as the dynamic ones, so they probably need to get wrapped them in some sort of container object)

@Rafostar:

OK. I will leave it open then. [...] One thing to note, is since I already have a tracks list model it would need to create yet another model that wraps the streams one and returns number of streams + 1 items. And having this as a list option would mean no ability to disable subtitles always option anymore.

@sp1ritCS:

And having this as a list option would mean no ability to disable subtitles always option anymore.

Yes, I believe this to be more confusing than helpful. Perhaps keeping track of the previously selected subtitle language is the better alternative.
I suspect this isn't that easy tho, as the actual language ids aren't actually available for subtitle streams, are they?

@Rafostar:

The option to globally disable subtitles is helpful (at least to me) and putting it to preferences would be inconvenient. The languages IDs might be or not available depending on video (it is nullable): https://rafostar.github.io/clapper/doc/clapper/method.SubtitleStream.get_lang_code.html

The hacks begin where we always have to return some stream when GStreamer asks us which is selected.

So the list model would have to store and remember last non-disabled item ID.

And tell GStreamer that this ID is selected even when it actually doesn't in this particular list.

It is doable but I call this to be hacky already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants