Fixes #1956: Optimise code in profile_chooser_add_view.xml

[Akshatkamboj14]

Explanation

Fixes #1956

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).

For UI-specific PRs only

If your PR includes UI-related changes, then:

• Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
• For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
• Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
• Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

[Akshatkamboj14]

@rt4914 @BenHenning @seanlip I am Blocked in this, I have tried comparing to the previous PR of this issue but the code has changed a lot and I have tried to compare it with other Merged PR that has Optimised in its description but I find none helpful,
I have a problem with changing orientation with boolean, so can anyone help me with it?

[BenHenning]

@rt4914 @BenHenning @seanlip I am Blocked in this, I have tried comparing to the previous PR of this issue but the code has changed a lot and I have tried to compare it with other Merged PR that has Optimised in its description but I find none helpful, I have a problem with changing orientation with boolean, so can anyone help me with it?

@Akshatkamboj14 it would help to have more context on the specific issue that you're running into so that we can effectively help you.

Based on the code, I'm assuming the challenge is that some of the margin/padding properties are difficult to combine due to there being a boolean used via databinding--is that correct? If so, could we maybe set it up so that we have one layout with all margins/padding attributes present that are needed across all versions of the layout, then create two dimensions for each based on the boolean value? Then, these can be specialized for each configuration type (e.g, portrait, landscape, tablet portrait, tablet landscape). Could that work?

[Akshatkamboj14]

Hey @BenHenning, That works, but I have a query in this section, https://github.com/oppia/oppia-android/blob/139056bb0d10aacf8f91f7755368312c51b3ccbf/app/src/main/res/layout/profile_chooser_add_view.xml#L47
The Linear layout and its Textviews are changing their configuration based upon the value of the boolean that we are getting from data-binding so my query is that how we can manage that by Constraint layout and I tried it by circle positioning in constraint layout but that is not working as well.

[BenHenning]

Hey @BenHenning, That works, but I have a query in this section, https://github.com/oppia/oppia-android/blob/139056bb0d10aacf8f91f7755368312c51b3ccbf/app/src/main/res/layout/profile_chooser_add_view.xml#L47 The Linear layout and its Textviews are changing their configuration based upon the value of the boolean that we are getting from data-binding so my query is that how we can manage that by Constraint layout and I tried it by circle positioning in constraint layout but that is not working as well.

@Akshatkamboj14 I'm not sure off-hand without fully solving it myself, but I think the easiest way to do this is to create one version of each of the layouts (for each display density) with ConstraintLayout to get that part working first, then work toward combining them together. I think that'll be much easier than trying to do it all in one shot.

[Akshatkamboj14]

Hey @BenHenning, That works, but I have a query in this section, https://github.com/oppia/oppia-android/blob/139056bb0d10aacf8f91f7755368312c51b3ccbf/app/src/main/res/layout/profile_chooser_add_view.xml#L47 The Linear layout and its Textviews are changing their configuration based upon the value of the boolean that we are getting from data-binding so my query is that how we can manage that by Constraint layout and I tried it by circle positioning in constraint layout but that is not working as well.

@Akshatkamboj14 I'm not sure off-hand without fully solving it myself, but I think the easiest way to do this is to create one version of each of the layouts (for each display density) with ConstraintLayout to get that part working first, then work toward combining them together. I think that'll be much easier than trying to do it all in one shot.

@BenHenning in today's meet @vrajdesai78 has suggested taking a look at profile_chooser_profile_view.xml and it's using the parent layout as constraint layout and in the constraint layout this file is using linear layout, so by taking a look at that i think I have already pushed the same, so what do you suggest?

[BenHenning]

Hey @BenHenning, That works, but I have a query in this section, https://github.com/oppia/oppia-android/blob/139056bb0d10aacf8f91f7755368312c51b3ccbf/app/src/main/res/layout/profile_chooser_add_view.xml#L47 The Linear layout and its Textviews are changing their configuration based upon the value of the boolean that we are getting from data-binding so my query is that how we can manage that by Constraint layout and I tried it by circle positioning in constraint layout but that is not working as well.

@Akshatkamboj14 I'm not sure off-hand without fully solving it myself, but I think the easiest way to do this is to create one version of each of the layouts (for each display density) with ConstraintLayout to get that part working first, then work toward combining them together. I think that'll be much easier than trying to do it all in one shot.

@BenHenning in today's meet @vrajdesai78 has suggested taking a look at profile_chooser_profile_view.xml and it's using the parent layout as constraint layout and in the constraint layout this file is using linear layout, so by taking a look at that i think I have already pushed the same, so what do you suggest?

@Akshatkamboj14 I think you need to first start with consolidating the layouts into one, then you can change the single one into a constraint layout. In my earlier comment, what I meant by 'display density' was each qualifier corresponding to types of the layout (e.g. '-land', '-sw600dp-port', and '-sw600dp-land').

I created a Gist to show the deltas between the different files to provide some context on how you can merge them together, even with the variable. See this similarity for reference: https://gist.github.com/BenHenning/f6795224360742c0bea5da9169b9ea0f/revisions#diff-f32d27c80e2849ced0f04be57483c80bf07bd6b06090cc3738c5b7052554afafL31. Here, we can see two lines and how they differ between values/profile_chooser_add_view.xml and values-land/profile_chooser_add_view.xml:

      app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/profile_chooser_add_view_margin_start_profile_already_added : @dimen/profile_view_not_added_margin}"	

(standard layout)

      android:layout_marginStart="16dp"

Here, we only use the boolean in the standard layout, but the common denominator is to always use the boolean (it's just that in the landscape case we don't care). Thus, we could do something like:

• Rename profile_chooser_add_view_margin_start_profile_already_added to add_profile_item_later_profile_start_margin
• Rename profile_view_not_added_margin to add_profile_item_first_profile_start_margin
  (landscape layout)
• Create values for both in values-land/dimens.xml to be 16dp
• Use the following for the consolidated layout:

      app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/add_profile_item_first_profile_start_margin : @dimen/profile_view_not_added_margin}"	

In the case of landscape, it will always be 16dp regardless of the boolean value so it will have the correct margin as the current setup.

Does this help clarify?

Regarding the top-level layout, from what I can see it's a LinearLayout for all four versions of the layout. As far as I can tell at first glance, it seems like all four layouts have the same structure which means all that needs to be consolidated is the view attributes.

Once that's done, then you can look into converting to ConstraintLayout.

[Akshatkamboj14]

Hey @BenHenning, That works, but I have a query in this section, https://github.com/oppia/oppia-android/blob/139056bb0d10aacf8f91f7755368312c51b3ccbf/app/src/main/res/layout/profile_chooser_add_view.xml#L47 The Linear layout and its Textviews are changing their configuration based upon the value of the boolean that we are getting from data-binding so my query is that how we can manage that by Constraint layout and I tried it by circle positioning in constraint layout but that is not working as well.

@Akshatkamboj14 I'm not sure off-hand without fully solving it myself, but I think the easiest way to do this is to create one version of each of the layouts (for each display density) with ConstraintLayout to get that part working first, then work toward combining them together. I think that'll be much easier than trying to do it all in one shot.

@BenHenning in today's meet @vrajdesai78 has suggested taking a look at profile_chooser_profile_view.xml and it's using the parent layout as constraint layout and in the constraint layout this file is using linear layout, so by taking a look at that i think I have already pushed the same, so what do you suggest?

@Akshatkamboj14 I think you need to first start with consolidating the layouts into one, then you can change the single one into a constraint layout. In my earlier comment, what I meant by 'display density' was each qualifier corresponding to types of the layout (e.g. '-land', '-sw600dp-port', and '-sw600dp-land').

I created a Gist to show the deltas between the different files to provide some context on how you can merge them together, even with the variable. See this similarity for reference: https://gist.github.com/BenHenning/f6795224360742c0bea5da9169b9ea0f/revisions#diff-f32d27c80e2849ced0f04be57483c80bf07bd6b06090cc3738c5b7052554afafL31. Here, we can see two lines and how they differ between values/profile_chooser_add_view.xml and values-land/profile_chooser_add_view.xml:

      app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/profile_chooser_add_view_margin_start_profile_already_added : @dimen/profile_view_not_added_margin}"	

(standard layout)

      android:layout_marginStart="16dp"

Here, we only use the boolean in the standard layout, but the common denominator is to always use the boolean (it's just that in the landscape case we don't care). Thus, we could do something like:

• Rename profile_chooser_add_view_margin_start_profile_already_added to add_profile_item_later_profile_start_margin
• Rename profile_view_not_added_margin to add_profile_item_first_profile_start_margin
  (landscape layout)
• Create values for both in values-land/dimens.xml to be 16dp
• Use the following for the consolidated layout:

      app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/add_profile_item_first_profile_start_margin : @dimen/profile_view_not_added_margin}"	

In the case of landscape, it will always be 16dp regardless of the boolean value so it will have the correct margin as the current setup.

Does this help clarify?

Regarding the top-level layout, from what I can see it's a LinearLayout for all four versions of the layout. As far as I can tell at first glance, it seems like all four layouts have the same structure which means all that needs to be consolidated is the view attributes.

Once that's done, then you can look into converting to ConstraintLayout.

Hey @BenHenning I am now fully understood what I am supposed to do but I have a question in what context am I supposed to rename the dimension values like in this

Rename profile_chooser_add_view_margin_start_profile_already_added to add_profile_item_later_profile_start_margin
Rename profile_view_not_added_margin to add_profile_item_first_profile_start_margin
(landscape layout)

In what context you are saying to rename the values?

[Akshatkamboj14]

And @BenHenning IDK where to declare these Attributes because if I am declaring it in dimes on building the project it's showing an error invalid dimen.

[BenHenning]

@Akshatkamboj14 I mean to rename the dimensions in all dimens.xml and layout files that reference them (so a project-wide rename for those dimensions).

Regarding the failure, what issue are you facing? A debugging doc might be useful here. As far as I can tell, your screenshot isn't referencing any dimensions so I'm not sure how it connects back to them per your question.

[Akshatkamboj14]

@BenHenning I understand what you are saying but I can rename the dimensions in all dimens.xml and layout files that reference them but there is an issue in this line

oppia-android/app/src/main/res/layout/profile_chooser_add_view.xml

Line 29 in efc98ca

android:orientation="@{hasProfileEverBeenAddedValue ? LinearLayout.VERTICAL : LinearLayout.HORIZONTAL}"

In this line, the orientation is changing as per the value of boolean and I cannot declare it the dimes.xml as well so I think merging into one layout is not possible in this layout.

[BenHenning]

@Akshatkamboj14 per chat I think you can try moving the LinearLayout orientations to integer properties which would then let you redefine them based on display density. Please let me know if that doesn't work.

[Akshatkamboj14]

Hey @BenHenning I am getting error upon building the project ig these lines(31 - 33) are the reason behind the error and if I am removing the lines(31 - 33) it's not showing any error, I think its important to add these lines because the land\profile_chooser_add_view has the same reference

oppia-android/app/src/main/res/layout-land/profile_chooser_add_view.xml

Line 25 in 7653898

android:layout_marginBottom="24dp"

oppia-android/app/src/main/res/layout-land/profile_chooser_add_view.xml

Line 28 in 7653898

app:layoutMarginBottom="@{hasProfileEverBeenAddedValue ? @dimen/profile_view_already_added_margin : @dimen/profile_chooser_add_view_margin_bottom_profile_not_added}"

and this is the error

C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\build\generated\source\kapt\debug\org\oppia\android\DataBinderMapperImpl.java:135: error: cannot find symbol
import org.oppia.android.databinding.ProfileChooserAddViewBindingImpl;
^
symbol: class ProfileChooserAddViewBindingImpl
location: package org.oppia.android.databindingC:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\ConstraintLayoutAdapters.java:14: warning: Application namespace for attribute app:layout_constraintEnd_toEndOf will be ignored.
public static void setConstraintEndToEndOf(@nonnull View view, int constraintToId) {
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\ConstraintLayoutAdapters.java:24: warning: Application namespace for attribute app:layout_constraintHorizontal_bias will be ignored.
public static void setHorizontalBias(@nonnull View view, float value) {
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\DrawableBindingAdapters.java:24: warning: Application namespace for attribute app:roundedRectDrawableWithColor will be ignored.
public static void setBackgroundDrawable(@nonnull View view, @ColorInt int colorRgb) {
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\DrawableBindingAdapters.java:33: warning: Application namespace for attribute app:topRoundedRectDrawableWithColor will be ignored.
public static void setTopBackgroundDrawable(@nonnull View view, @ColorInt int colorRgb) {
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\EditTextBindingAdapters.java:13: warning: Application namespace for attribute app:textChangedListener will be ignored.
public static void bindTextWatcher(@nonnull EditText editText, TextWatcher textWatcher) {
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\GuidelineBindingAdapters.java:13: warning: Application namespace for attribute app:layout_constraintGuide_percent will be ignored.
public static void setGuidelinePercentage(@nonnull Guideline guideline, float percentage) {
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\GuidelineBindingAdapters.java:22: warning: Application namespace for attribute app:layout_constraintGuide_end will be ignored.
public static void setConstraintGuidelineEnd(@nonnull Guideline guideline, float guideEndPx) {
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\ImageViewBindingAdapters.java:21: warning: Application namespace for attribute app:srcCompat will be ignored.
public static void setImageDrawableCompat(
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\ImageViewBindingAdapters.java:29: warning: Application namespace for attribute app:srcCompat will be ignored.
public static void setImageDrawableCompat(
^C:\Users\akuka\OneDrive\Desktop\opensource\oppia-android\app\src\main\java\org\oppia\android\app\databinding\ImageViewBindingAdapters.java:44: warning: Application namespace for attribute profile:src wi

[BenHenning]

@Akshatkamboj14 usually a binding reference error comes up when the binding is failed to be generated. This is caused by an error in the layout that is stopping the databinding compiler from succeeding, and the error is usually later in the build logs. Could you upload the entire build log to a Gist and link to it?

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!