Fix #1957: Optimise code in profile_chooser_profile_view.xml #2008

peculiaruc · 2020-10-13T17:20:43Z

Explanation

Fix Optimise code in profile_chooser_profile_view.xml #1957: Optimized code in profile_chooser_profile_view.xml by adding style to TextViews using defined styles in the style.xml file and removed redundant attributes.
Also, flatten the view hierarchy here into a single ConstraintLayout.
Reviewer: @BenHenning @MohamedMedhat1998 @rt4914

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

peculiaruc · 2020-10-13T17:57:13Z

@rt4914 PTAL

BenHenning · 2020-10-13T23:39:10Z

I'll take this.

peculiaruc · 2020-10-13T23:52:00Z

Please how do you mean?

…

On Wed, 14 Oct 2020 at 00:39, Ben Henning ***@***.***> wrote: I'll take this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2008 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIOVAVZHN5ZKW3DF5FISSJ3SKTQK3ANCNFSM4SPJXAKA> .

BenHenning

Thanks @peculiaruc! Took a first pass on the PR. I think it's on the right track, I just had a few nits & one more major suggestion in profile_chooser_profile_view.xml.

Beyond that, as a small nit please remove the space before : in your PR title for consistency with other PR titles. Also, please update your PR title & description to more accurately describe what this PR is doing. It's clearly optimizing more than just profile_chooser_profile_view.xml given the other style updates happening here.

BenHenning · 2020-10-13T23:39:39Z

.idea/codeStyles/Project.xml

@@ -161,4 +162,4 @@
      </indentOptions>
    </codeStyleSettings>
  </code_scheme>
-</component>
+</component>


Please add EOF newline here & in other files (in general, we always want all files to end with a newline).

Ok But this sort of error.

Sorry @peculiaruc I'm not sure I follow. What error are you referring to? In general, please try to include all the context in your comment since reviews can often take multiple hours or even a day to go back-and-forth on, and it's more efficient to over-communicate than under in these cases.

Also, I'm still seeing an EOF newline missing here.

@peculiaruc

EOF is missing, add one blank line at the end.

BenHenning · 2020-10-13T23:40:19Z

.idea/codeStyles/Project.xml

@@ -1,5 +1,6 @@
 <component name="ProjectCodeStyleConfiguration">
  <code_scheme name="Project" version="173">
+    <option name="FORMATTER_TAGS_ACCEPT_REGEXP" value="true" />


For this & the next file: please don't check in changes to the .idea project files in unrelated PRs (if you specifically want to change them, please submit a separate PR).

Noted. Thanks.

BenHenning · 2020-10-13T23:47:58Z

app/src/main/res/layout-land/profile_chooser_profile_view.xml

@@ -45,42 +45,33 @@
        android:layout_marginBottom="8dp"
        app:civ_border_color="@color/avatarBorder"
        app:civ_border_width="1dp"
-        profile:src="@{viewModel.profile.avatar}" />
+        profile:src="@{viewModel.profile.avatar}"/>


Prefer readding this space (ditto elsewhere) for consistency.

BenHenning · 2020-10-13T23:58:26Z

app/src/main/res/layout-land/profile_chooser_profile_view.xml

-        android:layout_width="wrap_content"
-        android:layout_height="wrap_content"
-        android:fontFamily="sans-serif-light"
+        style="@style/TextFieldLabel"


This doesn't seem like the correct replacement--the previous version was using sans-serif-light and TextFieldLabel uses standard sans-serif.

BenHenning · 2020-10-13T23:58:39Z

app/src/main/res/layout-land/profile_chooser_profile_view.xml

-        android:layout_width="wrap_content"
-        android:layout_height="wrap_content"
-        android:fontFamily="sans-serif-light"
+        style="@style/TextFieldLabel"


Ditto here.

BenHenning · 2020-10-14T00:00:39Z

app/src/main/res/layout-sw600dp-land/profile_chooser_profile_view.xml


        <TextView
          android:id="@+id/profile_last_visited"
-          android:layout_width="wrap_content"
-          android:layout_height="wrap_content"
-          android:fontFamily="sans-serif-light"


Similarly, we used light before so Subtitle1 doesn't seem like the correct fit.

@rt4914 looking at https://github.com/oppia/oppia-android/blob/a44791096a3e797653c3d463158db6e7662ab5cb/app/src/main/res/values/styles.xml we don't seem to have any styles with sans-serif-light. Are the current layouts wrong, or are we missing some styles?

Thanks. Holding off on resolving this until @rt4914 can weigh in since this particular situation probably should have a new style given you found a bunch of places where the same properties are being used.

BenHenning · 2020-10-14T00:00:55Z

app/src/main/res/layout-sw600dp-land/profile_chooser_profile_view.xml

-          android:layout_width="wrap_content"
-          android:layout_height="wrap_content"
-          android:fontFamily="sans-serif-light"
+          style="@style/Subtitle1"


Ditto for this one (the font family is unexpectedly changing).

BenHenning · 2020-10-14T00:01:53Z

app/src/main/res/layout-sw600dp-port/profile_chooser_profile_view.xml

-          android:layout_width="wrap_content"
-          android:layout_height="wrap_content"
-          android:fontFamily="sans-serif-light"
+          style="@style/Subtitle1"


Ditto here.

BenHenning · 2020-10-14T00:01:57Z

app/src/main/res/layout-sw600dp-port/profile_chooser_profile_view.xml

-          android:layout_width="wrap_content"
-          android:layout_height="wrap_content"
-          android:fontFamily="sans-serif-light"
+          style="@style/Subtitle1"


@BenHenning Please has this been resolved?

BenHenning · 2020-10-14T00:03:52Z

app/src/main/res/layout/profile_chooser_profile_view.xml

@@ -20,7 +20,7 @@
      type="org.oppia.android.app.model.ProfileChooserUiModel" />
  </data>

-  <LinearLayout
+  <androidx.constraintlayout.widget.ConstraintLayout


We should either stay with LinearLayout in this case, or remove the inner LinearLayouts by leveraging a single outer ConstraintLayout.

In this case, I suggest the latter: we should flatten the view hierarchy here into a single ConstraintLayout.

@peculiaruc I don't think you addressed this comment. To be clear: we should be optimizing this layout by flattening the hierarchy. That involves removing the nested LinearLayouts here & bringing up their elements to the new top-level ConstraintLayout, using constraints to make sure they're still presented correctly.

Note that there are also 2 other versions of this layout that need to be updated under layout-land and the sw600dp layout folders.

peculiaruc · 2020-10-14T16:15:15Z

@BenHenning PTAL

BenHenning

Thanks @peculiaruc. There's still some work left to do on this PR. Please make sure to fully address all comments before requesting another follow-up review. If there's something that you don't understand, please mention in a reply comment:

What you tried
What didn't work
Where you're stuck/confused
What you think are the next steps

This will help reviewers specifically understand where the issue started, and give them a foundation to provide specific feedback for you to get unstuck and make progress.

Please let me know if you have any questions!

BenHenning · 2020-10-15T05:13:24Z

.idea/codeStyles/Project.xml

@@ -161,4 +162,4 @@
      </indentOptions>
    </codeStyleSettings>
  </code_scheme>
-</component>
+</component>


Sorry @peculiaruc I'm not sure I follow. What error are you referring to? In general, please try to include all the context in your comment since reviews can often take multiple hours or even a day to go back-and-forth on, and it's more efficient to over-communicate than under in these cases.

Also, I'm still seeing an EOF newline missing here.

BenHenning · 2020-10-15T05:15:41Z

app/src/main/res/layout-sw600dp-land/profile_chooser_profile_view.xml


        <TextView
          android:id="@+id/profile_last_visited"
-          android:layout_width="wrap_content"
-          android:layout_height="wrap_content"
-          android:fontFamily="sans-serif-light"


Thanks. Holding off on resolving this until @rt4914 can weigh in since this particular situation probably should have a new style given you found a bunch of places where the same properties are being used.

BenHenning · 2020-10-15T05:18:06Z

app/src/main/res/layout/profile_chooser_profile_view.xml

@@ -20,7 +20,7 @@
      type="org.oppia.android.app.model.ProfileChooserUiModel" />
  </data>

-  <LinearLayout
+  <androidx.constraintlayout.widget.ConstraintLayout


@peculiaruc I don't think you addressed this comment. To be clear: we should be optimizing this layout by flattening the hierarchy. That involves removing the nested LinearLayouts here & bringing up their elements to the new top-level ConstraintLayout, using constraints to make sure they're still presented correctly.

Note that there are also 2 other versions of this layout that need to be updated under layout-land and the sw600dp layout folders.

peculiaruc · 2020-10-16T13:00:57Z

@BenHenning PTAL

BenHenning · 2020-10-17T01:15:26Z

I'm a bit braindead today. :) @MohamedMedhat1998 or @aggarwalpulkit596 could one of you do a pass on the ConstraintLayout bit to see if it looks good to you?

rt4914 · 2020-10-17T05:11:28Z

.idea/codeStyles/codeStyleConfig.xml

@@ -3,4 +3,4 @@
    <option name="USE_PER_PROJECT_SETTINGS" value="true" />
    <option name="PREFERRED_PROJECT_CODE_STYLE" value="Project copy" />
  </state>
-</component>
+</component>


EOF is missing, add one blank line at the end.

peculiaruc · 2020-10-23T22:46:02Z

Yes @rt4914 It does help thanks a lot

peculiaruc · 2020-10-26T07:52:33Z

@MohamedMedhat1998 Please confirm if the screens are good now. @BenHenning PTAL

MohamedMedhat1998 · 2020-10-26T13:30:56Z

@MohamedMedhat1998 Please confirm if the screens are good now. @BenHenning PTAL

Nice work @peculiaruc!
There are only few changes that need to be done, please refer to the following table.

Original	After PR	Regression
		The profiles aren't centered The margin between `Admin` and `Administrator` text views were increased
		The profiles aren't centered The margin between `Admin` and `Administrator` text views were increased
		The profiles top margin is decreased The margin between `Admin` and `Administrator` text views were increased
		The margin between `Admin` and `Administrator` text views were increased
		The margin between `Admin` and `Administrator` text views were increased

…profile_view

rt4914 · 2020-10-28T20:17:50Z

@peculiaruc Please update this PR and also fix the issues mentioned by @MohamedMedhat1998

Thanks.

peculiaruc · 2020-10-28T20:33:28Z

I have done that but couldn't test the app before my final push as a result of an error for @dimen/space_8dp. which is used to bind profile_name_text in the ProfileChooserProfileViewBindingImpl. I tried changing the margin of the textView to the recent change @dimens but it didn't work, i have also asked for help in Glitter, waiting for a possible reply. Thanks

…

On Wed, 28 Oct 2020 at 21:18, Rajat Talesra ***@***.***> wrote: @peculiaruc <https://github.com/peculiaruc> Please update this PR and also fix the issues mentioned by @MohamedMedhat1998 <https://github.com/MohamedMedhat1998> Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2008 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIOVAV2A66QP4ACXOQKAFXLSNB373ANCNFSM4SPJXAKA> .

…profile_view

rt4914 · 2020-10-29T11:08:34Z

I have done that but couldn't test the app before my final push as a result of an error for @dimen/space_8dp. which is used to bind profile_name_text in the ProfileChooserProfileViewBindingImpl. I tried changing the margin of the textView to the recent change @dimens but it didn't work, i have also asked for help in Glitter, waiting for a possible reply. Thanks
…
On Wed, 28 Oct 2020 at 21:18, Rajat Talesra @.***> wrote: @peculiaruc https://github.com/peculiaruc Please update this PR and also fix the issues mentioned by @MohamedMedhat1998 https://github.com/MohamedMedhat1998 Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2008 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIOVAV2A66QP4ACXOQKAFXLSNB373ANCNFSM4SPJXAKA .

@peculiaruc You are facing this issue because you are getting merge-conflicts while pulling the latest changes. You will need to resolve those merge conflicts manually to fix this. Reference PR because of which you are getting that error: #2034

peculiaruc · 2020-10-29T11:35:56Z

This is part of my challenge here, I try to update my branch but is demanding me to commit files I did not work on.

peculiaruc · 2020-10-29T11:36:09Z

rt4914 · 2020-10-29T11:47:42Z

This is part of my challenge here, I try to update my branch but is demanding me to commit files I did not work on.

That's how merge conflicts work. You will need to commit those changes and at the same time also make sure that you have done correct changes.

peculiaruc · 2020-10-29T12:49:29Z

@rt4914 PTAL.

This @dimen/space_8dp is the cause of this fail. The ProfileChooserProfileViewBindingImpl, kept throwing an error because of the modification done by #2034. Please can this change be made in the ProfileChooserProfileViewBindingImpl, so that this issue will be resolve
Thank

rt4914 · 2020-10-29T13:54:44Z

@rt4914 PTAL.

This @dimen/space_8dp is the cause of this fail. The ProfileChooserProfileViewBindingImpl, kept throwing an error because of the modification done by #2034. Please can this change be made in the ProfileChooserProfileViewBindingImpl, so that this issue will be resolve
Thank

@peculiaruc You might need to rebuild the project after doing the merge. ProfileChooserProfileViewBindingImpl is auto-generated and therefore we cannot make any change to this file.

peculiaruc · 2020-10-29T14:22:14Z

Yes, I know but in this case, is not working for me locally. I am checking to find a way around it. Thanks

…

On Thu, 29 Oct 2020 at 14:55, Rajat Talesra ***@***.***> wrote: @rt4914 <https://github.com/rt4914> PTAL. [image: image] <https://user-images.githubusercontent.com/35475543/97575789-8db00a00-19ed-11eb-86f3-0f196029c424.png> This @dimen/space_8dp is the cause of this fail. The ProfileChooserProfileViewBindingImpl, kept throwing an error because of the modification done by #2034 <#2034>. Please can this change be made in the ProfileChooserProfileViewBindingImpl, so that this issue will be resolve Thank @peculiaruc <https://github.com/peculiaruc> You might need to rebuild the project after doing the merge. ProfileChooserProfileViewBindingImpl is auto-generated and therefore we cannot make any change to this file. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2008 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIOVAV4KHVDGJ7E6U35QGCLSNFX3JANCNFSM4SPJXAKA> .

…profile_view

rt4914

@peculiaruc Great. There are few issues but its nice to see that you solved the issue that you were facing.

rt4914 · 2020-10-29T20:17:13Z

.idea/render.experimental.xml

+  <component name="RenderSettings">
+    <option name="showDecorations" value="true" />
+  </component>
+</project>


Please remove this file.

rt4914 · 2020-10-29T20:18:22Z

domain/src/test/java/org/oppia/android/domain/classify/InteractionObjectTestBuilder.kt

@@ -87,4 +88,10 @@ object InteractionObjectTestBuilder {
  fun createInt(value: Int): InteractionObject {
    return InteractionObject.newBuilder().setReal(value.toDouble()).build()
  }
+
+  fun createRatio(value: List<Int>): InteractionObject {


There are changes in these non xml files too. Thats because your branch is still 3 commits behind current develop.

If you can just merge with latest develop this should work and make these changes not visible.

Done, It is still yet to be fix

rt4914 · 2020-10-30T07:10:33Z

@peculiaruc As per our 1:1 discussion we should remove un-intentionally commits to PR and also keep on eye on Files Changed tabs in PR to make sure that you have only those files which you have changed intentionally. Thanks.

rt4914 · 2020-10-30T19:52:08Z

@peculiaruc Please close this PR, if not needed.

peculiaruc · 2020-10-31T01:48:10Z

This error persisted so i created a new branch and worked on it

peculiaruc · 2020-10-31T01:50:32Z

Alight, done

…

On Fri, 30 Oct 2020 at 20:52, Rajat Talesra ***@***.***> wrote: @peculiaruc <https://github.com/peculiaruc> Please close this PR, if not needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2008 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIOVAV35VTJW2OGYCQ5R6FDSNMKPNANCNFSM4SPJXAKA> .

pecpaker added 3 commits October 13, 2020 16:15

{{Fixes #1997:Optimise code in profile chooser fragment xml}}

a520170

{{Fix #1997:Optimise code in profile_chooser_profile_view.xml}}

5367b77

{{Fix #1957:Optimise code in profile_chooser_profile_view.xml}}

b83140c

BenHenning self-requested a review October 13, 2020 23:39

BenHenning self-assigned this Oct 13, 2020

BenHenning requested changes Oct 14, 2020

View reviewed changes

BenHenning assigned peculiaruc and unassigned BenHenning Oct 14, 2020

peculiaruc changed the title ~~Fix #1957 : Optimise code in profile_chooser_profile_view.xml~~ Fix #1957: Optimise code in profile_chooser_profile_view.xml Oct 14, 2020

{{Fixes #2008: Optimise code in profile_chooser_profile_view.xml }}

a0a8ee9

rt4914 assigned BenHenning and unassigned peculiaruc Oct 14, 2020

BenHenning previously requested changes Oct 15, 2020

View reviewed changes

BenHenning assigned peculiaruc and unassigned BenHenning Oct 15, 2020

pecpaker added 3 commits October 16, 2020 11:04

{{Fixes #2008: Optimise code in profile_chooser_profile_view}}

0548713

{{Fixes #2008: Optimise code in profile_choose_profile

18c6e70

{{Fixes #2008: Optimise code in profile_chooser_profile_view}}

92b7dd9

BenHenning assigned BenHenning and unassigned peculiaruc Oct 16, 2020

{{Fixes #2008: Optimise code in profile_chooser_profile_view}}

a55a249

BenHenning assigned MohamedMedhat1998 and aggarwalpulkit596 Oct 17, 2020

rt4914 reviewed Oct 17, 2020

View reviewed changes

{{Fix #1957: Optimise code in profile_chooser_profile_view.xml}}

693bb8e

MohamedMedhat1998 removed their assignment Oct 26, 2020

pecpaker added 2 commits October 27, 2020 15:28

Margins updated

dd1627f

Merge remote-tracking branch 'upstream/develop' into profile_chooser_…

9066029

…profile_view

Merge remote-tracking branch 'upstream/develop' into profile_chooser_…

1111184

…profile_view

pecpaker and others added 2 commits October 29, 2020 13:08

Other changes

ceb8501

Other changes

1d9d7eb

peculiaruc and others added 2 commits October 29, 2020 16:48

Fixed Margins

4b4376c

Merge remote-tracking branch 'upstream/develop' into profile_chooser_…

aa723ef

…profile_view

rt4914 suggested changes Oct 29, 2020

View reviewed changes

peculiaruc added 2 commits October 30, 2020 09:06

Fixed Margins

a7878d6

Fixed Margins

7f86822

peculiaruc mentioned this pull request Oct 30, 2020

Fixes #2008: Optimise code in profile_chooser_profile_view.xml #2065

Closed

8 tasks

peculiaruc closed this Oct 31, 2020

Fix #1957: Optimise code in profile_chooser_profile_view.xml #2008

Fix #1957: Optimise code in profile_chooser_profile_view.xml #2008

Conversation

peculiaruc commented Oct 13, 2020 • edited Loading

Explanation

Checklist

peculiaruc commented Oct 13, 2020

BenHenning commented Oct 13, 2020

peculiaruc commented Oct 13, 2020 via email

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peculiaruc Oct 14, 2020 • edited Loading

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

peculiaruc Oct 14, 2020 • edited Loading

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

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

peculiaruc Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peculiaruc commented Oct 14, 2020

BenHenning 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

peculiaruc commented Oct 16, 2020

BenHenning commented Oct 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peculiaruc commented Oct 23, 2020 • edited Loading

peculiaruc commented Oct 26, 2020

MohamedMedhat1998 commented Oct 26, 2020

rt4914 commented Oct 28, 2020

peculiaruc commented Oct 28, 2020 via email

rt4914 commented Oct 29, 2020

peculiaruc commented Oct 29, 2020

peculiaruc commented Oct 29, 2020

rt4914 commented Oct 29, 2020

peculiaruc commented Oct 29, 2020 • edited Loading

rt4914 commented Oct 29, 2020

peculiaruc commented Oct 29, 2020 via email

rt4914 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

rt4914 commented Oct 30, 2020

rt4914 commented Oct 30, 2020

peculiaruc commented Oct 31, 2020

peculiaruc commented Oct 31, 2020 via email

peculiaruc commented Oct 13, 2020 •

edited

Loading

peculiaruc Oct 14, 2020 •

edited

Loading

peculiaruc Oct 14, 2020 •

edited

Loading

peculiaruc Oct 14, 2020 •

edited

Loading

peculiaruc commented Oct 23, 2020 •

edited

Loading

peculiaruc commented Oct 29, 2020 •

edited

Loading