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

Question: About handling media view. #11977

Closed
parneet-guraya opened this issue Sep 18, 2023 · 9 comments · Fixed by #11936
Closed

Question: About handling media view. #11977

parneet-guraya opened this issue Sep 18, 2023 · 9 comments · Fixed by #11936

Comments

@parneet-guraya
Copy link
Contributor

Little Context First:

I recently pushed a PR (not merged yet) including migration from Exoplayer to Media3. Media UI has functionality to go full screen (type video) by launching a Dialog. It involved using internal resources (which turned out to be breaking, since marked private now in Media3) and also UI looked a little outdated too. So removed that functionality and I used the full screen button from the player view itself.

It handled going in and out of immersive mode.

But there's a catch --> The UI used to jumps when going in /out immersive mode. Turns out it conflicts with display cutout mode (DEFAULT). So, it is recommended -->

Use shortEdges or never cutout modes if your app needs to transition into and out of immersive mode. Default cutout behavior can cause content in your app to render in the cutout area while the system bars are present, but not while in immersive mode. This results in the content moving up and down during transitions, as demonstrated in the following image.

First Question -->

Since, media is handled inside PreviewMediaFragment (both audio and video). Setting cut-out mode does not take effect since activities have window ( which gets rendered) and by setting cut-out mode we're saying whether to render the window inside display cutout or not. So, in order take effect have to set cutout mode inside activity.

I did set the mode inside the main view FileDisplayActivity to NEVER, but then it would take effect for the whole main view which would be affect other previews etc.

So, should I move the media functionality to activity from fragment?? To handle the window differently for the media purposes?

Second Question -->

I have this question about which cutout mode to choose so do let me know about it.

Let me explain both of the cases:

  • NEVER --> It would show a black status bar (since not rendering in cutout area) but when landscape the status bar is not in the cutout area so it would show it's themed color.
    Is this looking okay (I'm asking here the opinion/suggestion because the cutout mode itself doing what it needs to be) if it shows the themed status bar when in landscape but not in portrait?
    See below 👇

NEVER

  • SHORT_EDGES --> Window will render into the cutout area. Also since it is rendering the window, in portrait mode the themed status bar color would show.
    But, it would be a little weird(seeing different colors due to different paddings to the views, see the circled area in landscape image) to see (in landscape mode). Since padding would be applied to the window (due to default setting of fitSystemWindows to fit the application window inside system window such as systembars, displaycutout etc). See below 👇

SHORT_EDGES

However there's a solution: So, we can set the decor view to not fit the system window. Hence it would be drawn below it and to avoid it being obscured we can use Insets (It contains value of system windows dimens (bars , cutout etc) ), which will give us the dimens of system windows those are getting overlapped with our application's view. That way we can use those values to add padding so that our content does not get cutout. Here's how google drive is using that thing 👇

G DRIVE

As you can see from the back arrow in google drive there's padding

But, I'm not able to achieve this solution. I do apply padding,I do also get correct insets value but for some reason applying padding to PlayerView and Toolbar does not take effect (however values are correct)

So, I think for now I'm stuck with option 1 (cutout mode: NEVER).Also, I thiink it is not looking bad but let me know what you think?

@AndyScherzinger
Copy link
Member

looping in @tobiasKaminsky and @ZetaTom

@parneet-guraya
Copy link
Contributor Author

Hi 👋 I just wanted to bring your attention to this, that I'm waiting for your answer, so that I can finish the PR #11936 😄.

@ZetaTom
Copy link
Collaborator

ZetaTom commented Oct 10, 2023

@parneet-guraya, thank you for providing such a comprehensive overview of the viable solutions.

To be honest, as a user, I'd expect the content to be displayed within the cutout area regardless of screen orientation. If I remember correctly, that's the way most of Googles apps handle media playback.
Regarding the screenshots for SHORT_EDGES, I completely agree with you, it looks a bit out of place. The (system) UI should appear in a consistent colour. The same goes for NEVER. Either way, it would be cleaner than the current implementation.
In my opinion, and if it's not too much trouble, I would recommend extending the PreviewMediaFragment into a separate activity, as it has its own unique UI. As far as I know that would be the way to go.

@tobiasKaminsky, is there anything I've missed?

@parneet-guraya
Copy link
Contributor Author

@parneet-guraya, thank you for providing such a comprehensive overview of the viable solutions.

To be honest, as a user, I'd expect the content to be displayed within the cutout area regardless of screen orientation. If I remember correctly, that's the way most of Googles apps handle media playback. Regarding the screenshots for SHORT_EDGES, I completely agree with you, it looks a bit out of place. The (system) UI should appear in a consistent colour. The same goes for NEVER. Either way, it would be cleaner than the current implementation.

Hi 👋 , actually I'm finally able to achieve the GDrive look. So, I guess we can go with SHORT_EDGES and have the UI rendered in the cutout area. Have a look below 👇

Screenshot_2023_10_15_17_44_47_21_06ea656a9ba653ff96746f477f32d0f9
Screenshot_2023_10_15_17_44_57_34_06ea656a9ba653ff96746f477f32d0f9

Record_2023-10-15-17-44-24.mp4

In my opinion, and if it's not too much trouble, I would recommend extending the PreviewMediaFragment into a separate activity, as it has its own unique UI. As far as I know that would be the way to go.

Yes, for the above thing to work, I need to have the Media logic in Activity as in PreviewMediaActivity (which is currently in fragment PreviewMediaFragment) tell me what you say. Also, I need to ask that should I include that migration inside the media3 migration PR #11936 or should I push the separate PR (with migration to activity from fragment) first?

@ZetaTom
Copy link
Collaborator

ZetaTom commented Oct 30, 2023

@parneet-guraya, this looks great! As this migration already involves substantial code changes, I would go ahead and push all changes into a single pull request.

@AndyScherzinger, would you recommend the same, or is a separate pull request preferable?

@AndyScherzinger
Copy link
Member

@ZetaTom anything is fine with me. Whatever helps with getting improvements merged fast(er) 😬👍

@parneet-guraya
Copy link
Contributor Author

Okay @AndyScherzinger So single PR for both migration to media3 and to Activity 👍

@AndyScherzinger
Copy link
Member

Thanks a lot @parneet-guraya 🙏 for your contributions and ongoing efforts to improve Nextcloud 💙

@parneet-guraya
Copy link
Contributor Author

Thanks a lot @parneet-guraya 🙏 for your contributions and ongoing efforts to improve Nextcloud 💙

Glad to contribute 💙

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

Successfully merging a pull request may close this issue.

4 participants