Fix part of #1633: Removed view dependency from ProfileEditViewModel #1925

alokbharti · 2020-10-02T20:20:53Z

Explanation

This PR fixes part of #1633
Fixes part of #1870

This PR contains following changes:

ProfileEditViewModel had a reference to activity variable to set its title but now an existing livedata variable is used to expose the title value and then ProfileEditActivityPresenter observe that variable to set the title for ProfileEditActivity.
Checked state of switch is now shifted to ProfileEditActivityPresenter
Instead of using ViewModelProvider, now ProfileEditViewModel is directly injected in ProfileEditActivityPresenter
Removed onRestoreInstanceState from ProfileEditActivity as viewmodel can survives configuration changes

Following tests are done to ensure that everything is working as it was before

Created multiple users and verified that the profile name is visible as the title of the activity
Verified the checked state of switch button in ProfileEditActivity
Verified above after rotating screen multiple times
Repeated above steps while Talkback is on
Ran existing test file related to ProfileEditActivity

Update

Fixed all the tests for ProfileEditActivity. (tested on pixel 2 emulator and Xioami Mi A3)

Roboelectric

Espresso

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.

- Used MutableLiveData to store the value of ProfileEditActivity Title and then expose it using a livedata variable in profleEditViewModel class - Observe the livedata variable to set ProfileEditActivity Title in ProfileEditActivityPresenter class

Arjupta · 2020-10-03T03:40:12Z

@alokbharti you must ask for a review from any of the code maintainers else your PR will be noticed late. Like
@rt4914 sir were viewing the original issue so you can ask him for review here.

alokbharti · 2020-10-03T04:37:45Z

@alokbharti you must ask for a review from any of the code maintainers else your PR will be noticed late. Like
@rt4914 sir were viewing the original issue so you can ask him for review here.

@Arjupta I already know this but I was waiting for the checks to be finished.

alokbharti · 2020-10-03T04:37:57Z

@rt4914 PTAL

anandwana001 · 2020-10-05T11:15:43Z

app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditViewModel.kt

@@ -27,6 +25,9 @@ class ProfileEditViewModel @Inject constructor(

  lateinit var profileName: String

+  private val _activityTitle = MutableLiveData<String>()


As per I understand from the issue and this committed file, we have to work on Switch.
@miaboloix Could you please correct me here?

@anandwana001 you might need to approach @miaboloix directly w.r.t. this.

@anandwana001 is correct, though there are actually two issues with this view model:

It relies on a view directly (the Switch--view models should not depend on views)

It relies on Activity, but its lifecycle is managed by AndroidX's ViewModelProvider which means it will leak references to the previous activity during configuration changes

#1633 is meant to fix (1), but if possible we should also fix (2) while here. The way we've been fixing (2) everywhere else is just by injecting the view model directly into the presenter rather than using ViewModelProvider.

@alokbharti you'll need to figure out a way to make (1) work--we don't want to depend on Switch which may require introducing a callback interface or a live data from this view model.

Hi @BenHenning , thanks for the suggestions. I'll fix those issues one by one for this viewmodel. For now, as suggested I'm doing the changes for the switch part.

Thanks. While here, can you also do (2)? All that's needed is to switch the presenter from using ModelViewProvider to injecting the view model directly. There aren't straightforward tests that we can add for this, so we'll rely on existing tests to pass.

rt4914 · 2020-10-06T01:59:20Z

Also, I have assigned this PR to @BenHenning too as I am not completely sure about this solution yet.

BenHenning

Thanks @alokbharti! Left some thoughts.

BenHenning · 2020-10-06T04:23:23Z

app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditViewModel.kt

@@ -27,6 +25,9 @@ class ProfileEditViewModel @Inject constructor(

  lateinit var profileName: String

+  private val _activityTitle = MutableLiveData<String>()


@anandwana001 is correct, though there are actually two issues with this view model:

It relies on a view directly (the Switch--view models should not depend on views)

It relies on Activity, but its lifecycle is managed by AndroidX's ViewModelProvider which means it will leak references to the previous activity during configuration changes

#1633 is meant to fix (1), but if possible we should also fix (2) while here. The way we've been fixing (2) everywhere else is just by injecting the view model directly into the presenter rather than using ViewModelProvider.

@alokbharti you'll need to figure out a way to make (1) work--we don't want to depend on Switch which may require introducing a callback interface or a live data from this view model.

- Added a livedata variable to expose the checked state of switch in ProfileViewModel through which ProfileEditActivityPresenter can observe and update the UI accordingly - Removed switch variable initialization from viewmodel - Added activity dependeny as it was before

BenHenning

Thanks @alokbharti! Left two comments. I noticed that your PR description is out-of-date--could you update that to reflect the changes in this PR?

Also to confirm, have you manually tested the app with these changes to make sure that the profile switch still works correctly?

Further, can you please also verify or add tests to make sure the switch functionality is working correctly after the changes from this PR?

BenHenning · 2020-10-06T20:10:29Z

app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditViewModel.kt

-  private lateinit var switch: Switch
+
+  private val _isAllowedDownloadAccess = MutableLiveData<Boolean>()
+  val isAllowedDownloadAccess: LiveData<Boolean> = _isAllowedDownloadAccess


Public members of a class should have documentation. See https://developer.android.com/kotlin/style-guide#usage and https://developer.android.com/kotlin/style-guide#documentation.

BenHenning · 2020-10-06T20:11:41Z

app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditViewModel.kt

@@ -27,6 +25,9 @@ class ProfileEditViewModel @Inject constructor(

  lateinit var profileName: String

+  private val _activityTitle = MutableLiveData<String>()


Thanks. While here, can you also do (2)? All that's needed is to switch the presenter from using ModelViewProvider to injecting the view model directly. There aren't straightforward tests that we can add for this, so we'll rely on existing tests to pass.

BenHenning

Apologies, I meant to select 'request changes' in the last review.

alokbharti · 2020-10-07T04:26:47Z

Thanks @alokbharti! Left two comments. I noticed that your PR description is out-of-date--could you update that to reflect the changes in this PR?

Also to confirm, have you manually tested the app with these changes to make sure that the profile switch still works correctly?

Further, can you please also verify or add tests to make sure the switch functionality is working correctly after the changes from this PR?

Thanks @BenHenning , I've tested the app manually & using talkback and It's working correctly. I'll add the second part too in the next commit i.e. changes for activity title and will update the PR.

- Removed activity variable from ProfileViewModel, as we can simply use existing profile livedata variable to update the title of the ProfileEditActivity in the ProfileEditActivityPresenter - Instead of using viewModelProvider in ProfileEditActivityPresenter to get viewmodel, now it is directly injected - Since viewmodel can survive configuration changes, there is no need to create an extra variable to store profile name to use it in onRestoreInstanceState - Added comments for public variable in ProfileEditViewModel

anandwana001

nit change.

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt

anandwana001

LGTM, Thanks @alokbharti

rt4914

LGTM, thanks.

rt4914 · 2020-11-06T07:57:04Z

@anandwana001 Please resolve your comment threads. Also if you find any comment which I have not resolved feel free to add me again.

@alokbharti We will have to wait for @BenHenning review on this as the merging is blocked. Mostly by End of this month.

BenHenning

Thanks @alokbharti! Really nice improvements to the tests.

Left a couple of comments, but the PR overall LGTM.

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

…earlayout

rt4914 · 2020-11-23T15:11:40Z

@alokbharti If you are looking for another approval from Ben. Please assign him this PR. Thanks.

BenHenning

Thanks @alokbharti, LGTM!

BenHenning · 2020-11-24T03:22:19Z

@Sarthak2601 I'm not seeing your comments for some reason, so going ahead and merging this. Please follow-up with an issue if you have any additional requests for this implementation.

anandwana001 requested review from miaboloix and rt4914 October 5, 2020 10:45

anandwana001 assigned rt4914 and miaboloix Oct 5, 2020

anandwana001 reviewed Oct 5, 2020

View reviewed changes

rt4914 requested a review from BenHenning October 6, 2020 01:58

rt4914 assigned BenHenning Oct 6, 2020

BenHenning requested changes Oct 6, 2020

View reviewed changes

BenHenning assigned alokbharti, rt4914 and miaboloix and unassigned rt4914, BenHenning and miaboloix Oct 6, 2020

alokbharti added 2 commits October 6, 2020 11:06

Added new lines as required

8ce8c0e

alokbharti requested review from BenHenning and anandwana001 October 6, 2020 05:47

BenHenning approved these changes Oct 6, 2020

View reviewed changes

BenHenning requested changes Oct 6, 2020

View reviewed changes

anandwana001 assigned alokbharti Nov 5, 2020

anandwana001 suggested changes Nov 5, 2020

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt Outdated Show resolved Hide resolved

Used launch instead of ActivityScenario.launch

14b3cae

alokbharti requested a review from anandwana001 November 5, 2020 02:48

alokbharti assigned anandwana001 and unassigned alokbharti Nov 5, 2020

anandwana001 assigned alokbharti and unassigned anandwana001 Nov 5, 2020

Added named argument

e873990

alokbharti assigned anandwana001 and unassigned alokbharti Nov 5, 2020

anandwana001 approved these changes Nov 6, 2020

View reviewed changes

anandwana001 removed their assignment Nov 6, 2020

rt4914 approved these changes Nov 6, 2020

View reviewed changes

rt4914 assigned BenHenning and anandwana001 and unassigned rt4914 Nov 6, 2020

anandwana001 removed their assignment Nov 9, 2020

BenHenning approved these changes Nov 17, 2020

View reviewed changes

app/src/main/res/layout/profile_edit_activity.xml Outdated Show resolved Hide resolved

app/src/main/res/layout/profile_edit_activity.xml Outdated Show resolved Hide resolved

app/src/main/res/layout/profile_edit_activity.xml Outdated Show resolved Hide resolved

BenHenning assigned alokbharti and unassigned BenHenning Nov 17, 2020

Removed 0dp margin and padding from scrollview and renamed id for lin…

76bb7ad

…earlayout

alokbharti removed their assignment Nov 20, 2020

alokbharti assigned BenHenning Nov 23, 2020

alokbharti requested a review from BenHenning November 23, 2020 15:16

BenHenning approved these changes Nov 24, 2020

View reviewed changes

BenHenning merged commit ff74da4 into oppia:develop Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #1633: Removed view dependency from ProfileEditViewModel #1925

Fix part of #1633: Removed view dependency from ProfileEditViewModel #1925

alokbharti commented Oct 2, 2020 •

edited

Loading

Arjupta commented Oct 3, 2020

alokbharti commented Oct 3, 2020

alokbharti commented Oct 3, 2020

anandwana001 Oct 5, 2020

rt4914 Oct 6, 2020

BenHenning Oct 6, 2020

alokbharti Oct 6, 2020

BenHenning Oct 6, 2020

rt4914 commented Oct 6, 2020

BenHenning left a comment

BenHenning Oct 6, 2020

BenHenning left a comment •

edited

Loading

BenHenning Oct 6, 2020

BenHenning Oct 6, 2020

BenHenning left a comment

alokbharti commented Oct 7, 2020

anandwana001 left a comment

anandwana001 left a comment

rt4914 left a comment

rt4914 commented Nov 6, 2020

BenHenning left a comment

rt4914 commented Nov 23, 2020

BenHenning left a comment

BenHenning commented Nov 24, 2020 •

edited

Loading

		@@ -27,6 +25,9 @@ class ProfileEditViewModel @Inject constructor(

		lateinit var profileName: String

		private val _activityTitle = MutableLiveData<String>()

Fix part of #1633: Removed view dependency from ProfileEditViewModel #1925

Fix part of #1633: Removed view dependency from ProfileEditViewModel #1925

Conversation

alokbharti commented Oct 2, 2020 • edited Loading

Explanation

Checklist

Arjupta commented Oct 3, 2020

alokbharti commented Oct 3, 2020

alokbharti commented Oct 3, 2020

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 6, 2020

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

alokbharti commented Oct 7, 2020

anandwana001 left a comment

Choose a reason for hiding this comment

anandwana001 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 commented Nov 6, 2020

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Nov 23, 2020

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Nov 24, 2020 • edited Loading

alokbharti commented Oct 2, 2020 •

edited

Loading

BenHenning left a comment •

edited

Loading

BenHenning commented Nov 24, 2020 •

edited

Loading