Rafostar / clapper

Level up your video experience with a modern and user-friendly media player.
https://rafostar.github.io/clapper/
GNU General Public License v3.0
772 stars 36 forks source link

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

Open sp1ritCS opened 5 months ago

sp1ritCS commented 5 months ago

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

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

Some suggestions:

Rafostar commented 5 months ago

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 commented 5 months ago

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 :cry:

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 commented 5 months ago

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

sp1ritCS commented 5 months ago

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.