Add fullscreen option to in-line videos #2747

avazirna · 2024-03-18T10:57:54Z

Summary

This PR adds the option to view inline videos in fullscreen mode. The initial approach was to use a third-party video app, but in order to offer a seamless user experience, an internal activity preferred.

Ticket: https://dimagi.atlassian.net/browse/SAAS-15133

Product Description

With this change, there will be an Fullscreen mode button in the media controller panel, which when pressed it transitions to fullscreen mode. And once in fullscreen mode, the same button can be used to transition back:

Inline_video_in_fullscreen_demo.mp4

Safety Assurance

I have confidence that this PR will not introduce a regression for the reasons below
Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Safety story

OrangeAndGreen

One little nit but otherwise all looks good.

OrangeAndGreen · 2024-03-18T13:13:14Z

app/src/org/commcare/views/media/CommCareMediaController.java

+    public void setAnchorView(View view) {
+        super.setAnchorView(view);
+
+        CommCareVideoView videoView;


Nit: minimize the code in the case, i.e.:
int videoId = fullScreenMode ? R.id.fullscreen_video_view : R.id.inline_video_view;
CommCareVideoView videoView = view.findViewById(videoId);

shubham1g5

Looks good overall, the PR should have a QA note and release note. We should also include a video/gif of the transition back and forth to get to know how the transition UX feels like.

shubham1g5 · 2024-03-19T23:44:04Z

app/res/values/styles.xml

@@ -293,4 +293,16 @@
    <color name="login_edit_text_color">@color/cc_core_text</color>
    <color name="login_edit_text_color_error">@color/cc_attention_negative_color</color>
    <dimen name="map_button_min_width">120dp</dimen>
+
+    <style name="MediaButton">


lets call it FullScreenVideoButton instead.

shubham1g5 · 2024-03-19T23:45:49Z

app/res/values/styles.xml

+        <item name="android:layout_width">71dip</item>
+        <item name="android:layout_height">52dip</item>


can we keep the sizes in multiples of 8 here (reasoning)

This is the same style being used by the Android's internal Media controller widgets, I kept the same dimensions to ensure consistency. But I don't think changing them will have an impact.

sounds good in that case.

shubham1g5 · 2024-03-20T00:04:22Z

app/src/org/commcare/activities/FullscreenVideoViewActivity.kt

+        setContentView(viewBinding.root)
+
+        // Get video URI from intent, finish if no URI is available
+        // TODO: we should inform the user when we finish because there is no data


pending todo, we should actually crash if no uri is available since that obviously means a code error.

instead of crashing, I opted for canceling the activity and informing the user.

app/src/org/commcare/views/media/CommCareMediaController.java

shubham1g5 · 2024-03-20T17:53:19Z

app/src/org/commcare/views/media/CommCareMediaController.java

+                } else {
+                    Intent intent = new Intent(getContext(), FullscreenVideoViewActivity.class);
+                    intent.setData(FileUtil.getUriForExternalFile(getContext(),
+                            new File(videoView.getVideoPath())));


we should explicitly validate videoView.getVideoPath() is not null and crash with an exception otherwise.

The validation of the of the video path happens before the instantiation of the CommCareMediaController, so this branch would only be executed if the file exists.

avazirna · 2024-04-12T15:18:29Z

@damagatchi retest this please

avazirna · 2024-04-12T16:26:25Z

@damagatchi retest this please

shubham1g5 · 2024-04-13T04:21:13Z

app/src/org/commcare/activities/FormEntryActivity.java

@@ -332,6 +332,8 @@ public void onActivityResultSessionSafe(int requestCode, int resultCode, Intent
            } else if (requestCode == FormEntryConstants.INTENT_CALLOUT) {
                processIntentResponse(intent, true);
                Toast.makeText(this, Localization.get("intent.callout.cancelled"), Toast.LENGTH_SHORT).show();
+            } else if (requestCode == FormEntryConstants.VIEW_VIDEO_FULLSCREEN) {
+                Toast.makeText(this, Localization.get("media.invalid.video.path"), Toast.LENGTH_SHORT).show();


This doesn't seem like the ideal validation place to me, we can potentially have other code path ways that can cancel the full screen activity. I think we should not even start the FullscreenVideoViewActivity.kt if the video uri is null or invalid and crash hard if we encounter the video path to be null inside the FullscreenVideoViewActivity

Done. 8588645

we should remove the validation here now right ?

Yes, it has been removed already, the code there is outdated. Can you double check?

avazirna added 13 commits March 11, 2024 17:55

Add fullscreen button to media controller

2cd1b6c

Lint

75f9a6e

Style fullscreen button

b68126e

Return video path

5ac6177

Add activity to view inline videos in fullscreen mode

3aade54

Maintain time position when switching to fullscreen mode

b10af35

Save time position during activity recreation

9f4f5a7

Return time position to calling activity

cd82dc9

Refactor

d67ca37

Add fullscreen mode flag to media controller

d5f4d2b

Switch to CommCare media controller

b5d5dfb

Add exit fullscreen mode button

0df8eed

Add exit fullscreen event

5ff7cfa

avazirna self-assigned this Mar 18, 2024

avazirna requested a review from OrangeAndGreen March 18, 2024 10:59

OrangeAndGreen previously approved these changes Mar 18, 2024

View reviewed changes

Lint

7ba0f74

avazirna dismissed OrangeAndGreen’s stale review via 7ba0f74 March 18, 2024 15:36

avazirna force-pushed the add-fullscreen-option-to-inline-videos branch from 5f336a2 to 7ba0f74 Compare March 18, 2024 15:36

Refactor

dd9f9e9

avazirna added the QA Note label Mar 18, 2024

shubham1g5 self-requested a review March 19, 2024 14:13

shubham1g5 requested changes Mar 20, 2024

View reviewed changes

avazirna added the Release Note label Apr 3, 2024

Refactor

fa76bbc

avazirna requested a review from shubham1g5 April 12, 2024 15:18

shubham1g5 requested changes Apr 13, 2024

View reviewed changes

avazirna added this to the 2.54 milestone Apr 17, 2024

Crash CommCare when there is no video path

8588645

avazirna force-pushed the add-fullscreen-option-to-inline-videos branch from 123590e to 8588645 Compare April 26, 2024 12:32

avazirna requested a review from shubham1g5 April 29, 2024 09:18

shubham1g5 approved these changes Apr 29, 2024

View reviewed changes

avazirna merged commit 2df7444 into master Apr 29, 2024
5 of 9 checks passed

avazirna deleted the add-fullscreen-option-to-inline-videos branch April 29, 2024 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fullscreen option to in-line videos #2747

Add fullscreen option to in-line videos #2747

avazirna commented Mar 18, 2024 •

edited

Loading

OrangeAndGreen left a comment

OrangeAndGreen Mar 18, 2024

shubham1g5 left a comment

shubham1g5 Mar 19, 2024

shubham1g5 Mar 19, 2024

avazirna Apr 3, 2024

shubham1g5 Apr 3, 2024

shubham1g5 Mar 20, 2024

avazirna Apr 8, 2024

shubham1g5 Mar 20, 2024

avazirna Apr 8, 2024

avazirna commented Apr 12, 2024

avazirna commented Apr 12, 2024

shubham1g5 Apr 13, 2024

avazirna Apr 29, 2024

shubham1g5 Apr 29, 2024

avazirna Apr 29, 2024

		<item name="android:layout_width">71dip</item>
		<item name="android:layout_height">52dip</item>

Add fullscreen option to in-line videos #2747

Add fullscreen option to in-line videos #2747

Conversation

avazirna commented Mar 18, 2024 • edited Loading

Summary

Product Description

Safety Assurance

Safety story

OrangeAndGreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1g5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Apr 12, 2024

avazirna commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Mar 18, 2024 •

edited

Loading