Inflate MediaLayout using xml #2336

ShivamPokhriyal · 2020-09-03T07:24:43Z

Breaking Change.

An attempt to fix the UI change caused by #2243 probably this (Inline Video View is really shrinked with a lot of white spaces See screenshot)

I've moved the layout logic to it's own xml which also avoids the redundant nested view groups.

PS: Follow up later with #2339

TODO:

~~Allow multiple missing media views to be present when the question contains multiple missing media.~~
- Seems like we only show inline-video view when both inline-video view and image are present in the question.
Fix showImageAboveText.
Fix video view visiiblilty issue once the video is downloaded(using missing media UI)
Add a background color to the missing media view.
Show missing media view for invalid image reference.
Check ResizingImageView, it was showing some error in the layout editor.
Validate this new change doesn't break any existing UI. Possibly through screenshot-tests Setup Screenshot tests #2339

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

ShivamPokhriyal · 2020-09-03T14:59:29Z

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

Same, do we need to show something here as well just like we do for videoView?

we can follow the same pattern as videoView.

shubham1g5 · 2020-09-07T14:24:25Z

@ShivamPokhriyal Wondering if you are seeing a different behaviour on the currently released Playstore version. To me both the released and master video playback behaves exactly same with same amount of whitespace.

ShivamPokhriyal · 2020-09-07T14:37:52Z

@ShivamPokhriyal Wondering if you are seeing a different behaviour on the currently released Playstore version. To me both the released and master video playback behaves exactly same with same amount of whitespace.

Yeah. the master build shows a lot of space to me, while the released version doesn't. You can try this app.

shubham1g5 · 2020-09-07T15:40:42Z

Got it. I can repro using your app. I was using a portrait video and possibly the bug doesn't surface with those. Would be good for you to test your solution with this app maybe so that we make sure we are not breaking anything.

Also the diff link in the PR description doesn't point me to anything specific but the PR and it seems like you were trying to point to something specific which broke this. Can you maybe try relinking the code point where it breaks.

ShivamPokhriyal · 2020-09-08T05:22:50Z

Can you maybe try relinking the code point where it breaks.

Sure, I think this https://github.com/dimagi/commcare-android/blob/master/app/src/org/commcare/views/media/MediaLayout.java#L160-L162 code adds the unwanted space and shrinks the videoview.

shubham1g5 · 2020-09-09T11:51:27Z

@ShivamPokhriyal Sounds good. Let me know when it's ready to review again as we would want to get this in soon given the code freeze on Friday.

ShivamPokhriyal · 2020-09-09T12:19:29Z

@shubham1g5 While carrying out the todo for showImageAboveText I realised that this layout hierarchy doesn't consider multiple missing medias.
Also, I'm really afraid that this change might break a lot of UI(paddings/margins). So I was thinking of setting up the screenshot tests to validate that this change doesn't break anything. So I guess it's gonna take a lot more time.

I've also edited the description of the PR with a list of TODOs.

shubham1g5 · 2020-09-09T18:21:39Z

@ShivamPokhriyal moving this to 2.51 in that case.

shubham1g5 · 2020-09-11T11:04:37Z

@ShivamPokhriyal I just noticed that the showImageAboveText is also breaking in master which makes it critical for us to fix this in 2.50. Wondering how do you feel about completing it for 2.50 without the screenshot tests (QA should be able to take care of that). If it's not likely I can attempt to fix these issues separately without xml layout in 2.50

ShivamPokhriyal · 2020-09-11T11:24:08Z

@ShivamPokhriyal I just noticed that the showImageAboveText is also breaking in master which makes it critical for us to fix this in 2.50. Wondering how do you feel about completing it for 2.50 without the screenshot tests (QA should be able to take care of that). If it's not likely I can attempt to fix these issues separately without xml layout in 2.50

TBH I'm not really confident with this change😅. Though the only thing remaining in this PR is the video view visibility issue which is really tricky and I haven't been able to figure it out yet. If you think QA can take care of testing this change then I can definitely try fixing the video view issue and get it merged before QA starts.

ShivamPokhriyal · 2020-09-11T13:46:06Z

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

-        params.width = maxBounds[0];
-        params.height = maxBounds[1];
+        params.width = metrics.widthPixels;
+        params.height = metrics.heightPixels;


the reason why videoView doesn't appear after download completion was this maxBounds[1]. In my case, the textView had a really long text and I had to scroll down to see the videoView.

The getMaxCenterViewBounds method subtracts the text height from the total height
maxHeight = maxHeight - textViewContainer.getChildAt(0).getHeight(); making this maxBounds[1] to be 0.

Since, videoView will automatically configure itself with the min height possible, we're good to give it max height here.

ShivamPokhriyal · 2020-09-11T14:25:22Z

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

            Log.e(TAG, "invalid video reference exception");
            ire.printStackTrace();
-            return getMissingMediaView(inlineVideoURI, "Invalid reference: " + ire.getReferenceString(), false);
+            showMissingMediaView(inlineVideoURI, "Invalid reference: " + ire.getReferenceString(), false, null);


@shubham1g5 Do we need to localize this text?

Also the information here isn't really informative imo. Would it be better if we say: Can't show media due to: ?

yes, we should localize anything we display on UI. I think the prefix you mentioned is redundant in Invalid ... but would not really mind us adding that.

shubham1g5

Looks quite good to me. Left some very minor points for you to consider. On testing it's regressions, we should check the 2.50 test plan and see if our current tests have testcases for 1. QR view setup in the image 2. Missing Media (these would definitely need to get added) 3. showImageAboveText usecases.

shubham1g5 · 2020-09-13T17:26:45Z

app/res/layout/media_layout.xml

+        app:layout_constraintTop_toBottomOf="@id/question_barrier" />
+
+    <!--
+    Missing media view


nit: comments formatting seems off, should be single lined

shubham1g5 · 2020-09-13T17:56:05Z

app/res/layout/media_layout.xml

@@ -0,0 +1,178 @@
+<?xml version="1.0" encoding="utf-8"?>
+<merge xmlns:android="http://schemas.android.com/apk/res/android"


Good use of merge to avoid layout hierarchy.

shubham1g5 · 2020-09-13T18:53:29Z

app/res/layout/media_layout.xml

+        app:layout_constraintRight_toRightOf="parent"
+        app:layout_constraintTop_toBottomOf="@id/audio_button" />
+
+    <androidx.constraintlayout.widget.Barrier


Read about Barrier a bit and seems like a real useful addition to the set of Android views. Thanks for finding about these.

shubham1g5 · 2020-09-13T18:54:13Z

app/res/layout/media_layout.xml

+        android:id="@+id/buttons_barrier"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"
+        app:barrierDirection="left"


should be start to not break RTL languages.

shubham1g5 · 2020-09-13T19:45:37Z

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

            Log.e(TAG, "invalid video reference exception");
            ire.printStackTrace();
-            return getMissingMediaView(inlineVideoURI, "Invalid reference: " + ire.getReferenceString(), false);
+            showMissingMediaView(inlineVideoURI, "Invalid reference: " + ire.getReferenceString(), false, null);


yes, we should localize anything we display on UI. I think the prefix you mentioned is redundant in Invalid ... but would not really mind us adding that.

shubham1g5 · 2020-09-13T19:47:47Z

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

-        params.width = maxBounds[0];
-        params.height = maxBounds[1];
+        params.width = metrics.widthPixels;
+        params.height = metrics.heightPixels;


shubham1g5 · 2020-09-13T19:58:15Z

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

-        View downloadIcon = missingMediaView.findViewById(R.id.download_media_icon);
+    private void showMissingMediaView(String mediaUri, String errorMessage, boolean allowDownload, @Nullable Runnable completion) {
+        missingMediaBackground.setVisibility(VISIBLE);
+        missingMediaText.setText(errorMessage);


nit: missingMediaText -> missingMediaStatus

shubham1g5 · 2020-09-13T20:05:07Z

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

-                        true);
+                        true,
+                        () -> {
+                            hideMissingMediaView();


stylistic point that you can ignore: hideMissingMediaView can be called from showMissingMediaView to DRY the completion logic further.

shubham1g5 · 2020-09-13T20:06:33Z

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

-            return;
-        }
-        addView(v, dividerParams);
+    private void resetView() {


this method can be avoided by setting the initial visibility of all views as gone in xml layout itself.

Fix for android.view.ViewGroup cannot be cast to androidx.constraintlayout.widget.ConstraintLayout

shubham1g5

left Couple questions, though build seems to be failing on this one.

shubham1g5 · 2020-09-14T06:21:28Z

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

-        // Next align the text, audiobutton and videoButton below mediaView.
+        // Next align the text, audiobutton below mediaView.
        alignTextContainerBelowMediaView(audioButton);
-        alignTextContainerBelowMediaView(videoButton);


confused why did we remove videoButton from here

videoButton has a constraint which places it below audioButton. So we only need to align audioButton top constraint while shifting it below media view.

shubham1g5 · 2020-09-14T06:21:56Z

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

    }

    private void initView(Context context) {
+        setId(AndroidUtil.generateViewId());


why is this required ?

If not set, this.getId() call above returns -1 which is the same as LayoutParams.UNSET.

Ahh Makes sense.

ShivamPokhriyal · 2020-09-14T06:34:30Z

@damagatchi retest this please

shubham1g5 · 2020-09-14T07:33:13Z

app/res/layout/media_layout.xml

        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
-        android:visibility="gone"
+        tools:visibility="gone"


tools:visibility should be visible while keeping the android:visibility as gone

Ahh Damn, silly me! I thought it behaves the same as android:visibility

shubham1g5 · 2020-09-14T07:39:11Z

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

+        initView(context);
    }

    public static MediaLayout buildAudioImageLayout(Context context, TextView text, String audioURI, String imageURI) {


this seems ununsed and can be removed.

shubham1g5 · 2020-09-14T07:43:56Z

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


-        addAudioVideoButtonsToView(questionTextPane);
+        // Next align the text, audiobutton below mediaView.
+        alignTextContainerBelowMediaView(audioButton);


Wondering if it's possible to avoid multiple calls of alignTextContainerBelowMediaView as well and instead use barrier/guideline mechanism to shift these views ?

shubham1g5

Looks great! Kudos 🎆

ShivamPokhriyal commented Sep 3, 2020

View reviewed changes

app/src/org/commcare/views/media/MediaLayout.java Outdated Show resolved Hide resolved

ShivamPokhriyal changed the title ~~Inflate MediaLayout using xml~~ Inflate MediaLayout using xml [WIP] Sep 3, 2020

ShivamPokhriyal force-pushed the revamp-media-layout branch from 1fff6e2 to 931b63e Compare September 3, 2020 14:49

ShivamPokhriyal marked this pull request as ready for review September 3, 2020 14:57

ShivamPokhriyal added the product/all-users label Sep 3, 2020

ShivamPokhriyal added this to the 2.50 milestone Sep 3, 2020

ShivamPokhriyal commented Sep 3, 2020

View reviewed changes

shubham1g5 modified the milestones: 2.50, 2.51 Sep 9, 2020

ShivamPokhriyal force-pushed the revamp-media-layout branch from 931b63e to 6cf8668 Compare September 10, 2020 12:58

ShivamPokhriyal added 3 commits September 11, 2020 18:50

Inflate MediaLayout using xml

47ed9d2

Re-Add support to show media above text

122c75b

Fix videoView visibility issue after download completion

53f6ac6

ShivamPokhriyal force-pushed the revamp-media-layout branch from 6cf8668 to 53f6ac6 Compare September 11, 2020 13:42

ShivamPokhriyal commented Sep 11, 2020

View reviewed changes

Add background color to missing media view

417698d

ShivamPokhriyal requested review from ctsims and shubham1g5 September 11, 2020 14:14

ShivamPokhriyal added ready for review and removed ready for review labels Sep 11, 2020

Show missing media for invalid image reference

d6f0688

ShivamPokhriyal added the ready for review label Sep 11, 2020

ShivamPokhriyal commented Sep 11, 2020

View reviewed changes

shubham1g5 requested changes Sep 13, 2020

View reviewed changes

shubham1g5 modified the milestones: 2.51, 2.50 Sep 13, 2020

ShivamPokhriyal added 4 commits September 14, 2020 10:19

Fix videobutton visibility issue when media is above text

b59bd7b

Fix missing media background when media is shown above text

51b4df6

Fix ResizingImageView

c7ecd9f

Fix for android.view.ViewGroup cannot be cast to androidx.constraintlayout.widget.ConstraintLayout

PR suggestions

6a3b33c

ShivamPokhriyal requested a review from shubham1g5 September 14, 2020 05:49

shubham1g5 reviewed Sep 14, 2020

View reviewed changes

Use guideline for shifting mediaview and textcontainer

fdf4d38

ShivamPokhriyal requested a review from shubham1g5 September 14, 2020 07:31

shubham1g5 reviewed Sep 14, 2020

View reviewed changes

Use tools visibility along-with android visibility

e25e598

shubham1g5 reviewed Sep 14, 2020

View reviewed changes

Add suggested changes

a439529

ShivamPokhriyal requested a review from shubham1g5 September 14, 2020 08:00

Merge branch 'master' into revamp-media-layout

43a5b82

shubham1g5 approved these changes Sep 14, 2020

View reviewed changes

ShivamPokhriyal merged commit a4f075a into master Sep 14, 2020

ShivamPokhriyal deleted the revamp-media-layout branch September 14, 2020 08:50

ShivamPokhriyal changed the title ~~Inflate MediaLayout using xml [WIP]~~ Inflate MediaLayout using xml Sep 14, 2020

ShivamPokhriyal mentioned this pull request Sep 23, 2020

Revert "Inflate MediaLayout using xml" #2355

Closed

		@@ -0,0 +1,178 @@
		<?xml version="1.0" encoding="utf-8"?>
		<merge xmlns:android="http://schemas.android.com/apk/res/android"

Uh oh!

Inflate MediaLayout using xml #2336

Inflate MediaLayout using xml #2336

Uh oh!

Conversation

ShivamPokhriyal commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Change.

PS: Follow up later with #2339

TODO:

Uh oh!

Uh oh!

ShivamPokhriyal Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 commented Sep 7, 2020

Uh oh!

ShivamPokhriyal commented Sep 7, 2020

Uh oh!

shubham1g5 commented Sep 7, 2020

Uh oh!

ShivamPokhriyal commented Sep 8, 2020

Uh oh!

shubham1g5 commented Sep 9, 2020

Uh oh!

ShivamPokhriyal commented Sep 9, 2020

Uh oh!

shubham1g5 commented Sep 9, 2020

Uh oh!

shubham1g5 commented Sep 11, 2020

Uh oh!

ShivamPokhriyal commented Sep 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShivamPokhriyal Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShivamPokhriyal commented Sep 3, 2020 •

edited

Loading

ShivamPokhriyal Sep 3, 2020 •

edited

Loading

ShivamPokhriyal Sep 14, 2020 •

edited

Loading