Add fullscreen option to in-line videos #2747
Conversation
| public void setAnchorView(View view) { | ||
| super.setAnchorView(view); | ||
|
|
||
| CommCareVideoView videoView; |
There was a problem hiding this comment.
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);
avazirna
force-pushed
the
add-fullscreen-option-to-inline-videos
branch
from
5f336a2
to
7ba0f74
Compare
app/res/values/styles.xml
Outdated
| @@ -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"> | |||
There was a problem hiding this comment.
lets call it FullScreenVideoButton instead.
| <item name="android:layout_width">71dip</item> | ||
| <item name="android:layout_height">52dip</item> |
There was a problem hiding this comment.
can we keep the sizes in multiples of 8 here (reasoning)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sounds good in that case.
| 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 |
There was a problem hiding this comment.
pending todo, we should actually crash if no uri is available since that obviously means a code error.
There was a problem hiding this comment.
instead of crashing, I opted for canceling the activity and informing the user.
| } else { | ||
| Intent intent = new Intent(getContext(), FullscreenVideoViewActivity.class); | ||
| intent.setData(FileUtil.getUriForExternalFile(getContext(), | ||
| new File(videoView.getVideoPath()))); |
There was a problem hiding this comment.
we should explicitly validate videoView.getVideoPath() is not null and crash with an exception otherwise.
There was a problem hiding this comment.
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.
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
| @@ -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(); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
we should remove the validation here now right ?
There was a problem hiding this comment.
Yes, it has been removed already, the code there is outdated. Can you double check?
avazirna
force-pushed
the
add-fullscreen-option-to-inline-videos
branch
from
123590e
to
8588645
Compare
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
Safety story