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

chore: show subtitles for videos in full-screen mode #97

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

shafqat-muneer
Copy link

@shafqat-muneer shafqat-muneer commented Nov 19, 2024

LEARNER-10236: iOS - videos do not show captions when viewed full-screen

In the current implementation, subtitles are displayed only in full-screen mode, whether in landscape or portrait orientation. Subtitles are not shown in non-full-screen mode for two reasons. First, we cannot control the default caption on/off option provided by AVPlayer. Second, in normal mode, users can already view captions in any language, making it unnecessary to display additional subtitles. However, in full-screen mode, the option to view captions is unavailable, which is why we ensure subtitles are shown in that mode.

Note: Currently, there is no option to hide subtitles in full-screen mode. To enable this functionality, we would need to design a solution for it or opt for a fully custom implementation by hiding the default AVPlayer control bar.

Demo Downloaded Video Demo Streaming Video
Demo.Downloaded.Video.mov
Demo.Streaming.Video.mov

@saeedbashir
Copy link

@forgotvas It would be nice if you could also look as you did most of the work with videos.

@rnr
Copy link
Collaborator

rnr commented Nov 19, 2024

Hello @shafqat-muneer
On both videos you show in the PR description, the previously added subtitles (below the video) stop working (are not highlighted and do not auto-scroll) after exiting full screen mode. Could you please fix it. Thank you

@shafqat-muneer
Copy link
Author

Hello @shafqat-muneer On both videos you show in the PR description, the previously added subtitles (below the video) stop working (are not highlighted and do not auto-scroll) after exiting full screen mode. Could you please fix it. Thank you

Hi @rnr,
I was considering that auto-scrolling to the highlighted part might be intentionally disabled, as users may scroll up or down on their own to view previous or upcoming subtitles. If auto-scrolling is enabled, it could interrupt their navigation. I wanted to confirm if this is an intentional implementation. @saeedbashir or @forgotvas can confirm on it.

Once clarified, and if we decide to implement auto-scrolling to the highlighted content, I will handle it in a separate PR since it falls outside the scope of this one.

@rnr
Copy link
Collaborator

rnr commented Nov 19, 2024

Hello @shafqat-muneer On both videos you show in the PR description, the previously added subtitles (below the video) stop working (are not highlighted and do not auto-scroll) after exiting full screen mode. Could you please fix it. Thank you

Hi @rnr, I was considering that auto-scrolling to the highlighted part might be intentionally disabled, as users may scroll up or down on their own to view previous or upcoming subtitles. If auto-scrolling is enabled, it could interrupt their navigation. I wanted to confirm if this is an intentional implementation. @saeedbashir or @forgotvas can confirm on it.

Once clarified, and if we decide to implement auto-scrolling to the highlighted content, I will handle it in a separate PR since it falls outside the scope of this one.

@shafqat-muneer I'd like to point out that auto-scrolling now works in the 2U/develop branch (even after exiting fullscreen mode). Why did we decide to disable it? So other words it was working before this PR.

@shafqat-muneer
Copy link
Author

@shafqat-muneer I'd like to point out that auto-scrolling now works in the 2U/develop branch (even after exiting fullscreen mode). Why did we decide to disable it? So other words it was working before this PR.

@rnr It seems that the functionality may occasionally work, but not consistently. On the 2U/develop branch, it’s currently not working on my end, whether in portrait or landscape mode. 😕

Landscape Portrait
Landscape.mp4
Portrait.mp4

@rnr
Copy link
Collaborator

rnr commented Nov 19, 2024

@shafqat-muneer I'd like to point out that auto-scrolling now works in the 2U/develop branch (even after exiting fullscreen mode). Why did we decide to disable it? So other words it was working before this PR.

@rnr It seems that the functionality may occasionally work, but not consistently. On the 2U/develop branch, it’s currently not working on my end, whether in portrait or landscape mode. 😕

Landscape Portrait
Landscape.mp4
Portrait.mp4

I see what you mean - yes, it is broken when the user rewinds in fullscreen mode even in 2U/develop - I think we need to create a separate issue for this. Thank you.

Copy link

@saeedbashir saeedbashir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@shafqat-muneer shafqat-muneer merged commit dc2eab5 into 2U/develop Nov 22, 2024
2 checks passed
@shafqat-muneer shafqat-muneer deleted the Shafqat/LEARNER-10236 branch November 22, 2024 08:32
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

Successfully merging this pull request may close these issues.

4 participants