Fix #3586: Merge topic_practice_subtopic.xml into a single xml file

[kritigupta45]

Explanation

Fixes #3586: merges the 4 versions of the topic_practice_subtopic.xml file into a single XML file.

Essential Checklist

• The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)

• Any changes to scripts/assets files have their rationale included in the PR explanation.

• The PR follows the style guide.

• The PR does not contain any unnecessary code changes from Android Studio (reference).

• The PR is made from a branch that's not called "develop" and is up-to-date with "develop".

• The PR is assigned to the appropriate reviewers (reference).

• Before ss of mobile-portrait mode

• After ss of mobile-portrait mode

• Before ss of mobile-landscape mode

• After ss of mobile-landscape mode

• Before ss of tablet-portrait mode

• After ss of tablet-portrait mode

• Before ss of tablet-landscape mode

• After ss of tablet-landscape mode

[kritigupta45]

@Arjupta PTAL. Thanks.

[rt4914]

The before/after screenshot does look different for mobile-landscape and therefore the code should be merged such that there is no difference in before/after UI.

Also once you remove tablet related files, make sure you remove the app from tablet device and then rebuild the entire project and then run the app on emulator.

[oppiabot]

Hi @kritigupta45, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

[kritigupta45]

Apologies for the discrepancy between the before and after changes (ss) in mob-landscape mode UI.
@rt4914 PTAL. Thanks.

[oppiabot]

Unassigning @kritigupta45 since a re-review was requested. @kritigupta45, please make sure you have addressed all review comments. Thanks!

[Arjupta]

@kritigupta45 you don't have to commit the file changes that are under .idea/ directory. You can revert these files in the next commit.

[kritigupta45]

@rt4914 I am facing an issue with the tablet mode where the layout_width is set to 360dp and match_parent for the mobile UI.
Unable to get a proper dimension for the dimens.xml file which should work well with both mobile and tablet mode UI.
PTAL. Thanks.

[rt4914]

@kritigupta45 @Arjupta PTAL Thanks.

[rt4914]

To merge all files correctly you can use ConstraintLayout (CL) inside ConstraintLayout such that the outer CL is match_parent and inner CL width can be either 0dp or 360dp. When its 0dp it will automatically be treated as match_parent.

[Arjupta]

Nice Idea. But doesn't this looks like a patch work. I didn't knew that Android doesn't support match_parent as parameter in viewbinding. I thought just like while handling with importantForAccessibilty stuff we got to change it with the help of few dimens constants, that actually corresponded to the values of enums.

[rt4914]

We can use databinding too and it might be clear, I am just doubtful about the UI rendering performance. Can you check if this works without any UI glitch?

How is it related to accessibility?

[Arjupta]

I have tried that databinding solution but no luck. Also the accessiblity issue I am talking about is implemented here also importantForAccessiblity. Normally the importantForAccessiblity will take yes or no as a parameter. But we know that there enums values corresponds to true and false hence we used that logic here. I was aksing can something similar be done

[rt4914]

@Arjupta We cannot use databinding because for doing that we need some condition based on which we can identify if we want to keep match_parent or 360dp width. And here we don't have any such condition. Therefore using CL inside CL is the easiest working solution here.

[rt4914]

I have tried the nested ConstraintLayout solution and it is working correctly. Below is the sample code.
Note that you will need to introduce the correct values for width, margin_start and margin_end dimens in all 4 dimens files and also make sure that the names are also correct.

<androidx.constraintlayout.widget.ConstraintLayout
    android:layout_width="match_parent"
    android:layout_height="wrap_content">

    <androidx.constraintlayout.widget.ConstraintLayout
      android:layout_width="@dimen/width"
      android:layout_height="wrap_content"
      android:layout_marginStart="@dimen/margin_start"
      android:layout_marginEnd="@dimen/margin_end"
      app:barrierDirection="end"
      app:layout_constraintBottom_toBottomOf="parent"
      app:layout_constraintEnd_toEndOf="parent"
      app:layout_constraintStart_toStartOf="parent"
      app:layout_constraintTop_toTopOf="parent">

      <androidx.constraintlayout.widget.Barrier

[Arjupta]

@Arjupta We cannot use databinding because for doing that we need some condition based on which we can identify if we want to keep match_parent or 360dp width. And here we don't have any such condition. Therefore using CL inside CL is the easiest working solution here.

@rt4914 I agree with your findings and hence @kritigupta45 can go with making a CL under a Cl for this PR. Although I would like to know that, if we assume there is an observable boolean in the viewmodel which changes its value based on the device orientation (true for portrait & false for landscape). So can we use this boolean to change the constraint between match_parent and some other value (defined in dimens.xml). And if the answer is Yes, then please do share a code snippet.

[kritigupta45]

Done.

[rt4914]

@Arjupta Yes we can do that but that would make code more complex as compared to what we have done right now.

[Arjupta]

The changes I suggested are done hence LGTM. @rt4914 PTAL

[oppiabot]

Assigning @BenHenning for code owner reviews. Thanks!

[rt4914]

LGTM, thanks.