Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1737: Singleton for exploration to handle input for configuration changes #2514

Closed

Conversation

FareesHussain
Copy link
Contributor

@FareesHussain FareesHussain commented Jan 18, 2021

Explanation

Fixes #1737

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.

@FareesHussain
Copy link
Contributor Author

@rt4914 @BenHenning Here I can get the pending answer and can save it after configuration change but how do set it to the InteractionView for a particular State


import org.oppia.android.app.model.UserAnswer

object RetriveUserAnswer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a Dagger singleton (see other @Singleton marked classes in the codebase).

@BenHenning
Copy link
Member

@FareesHussain I suspect that you will need to find a way to pre-populate the views with initial user answers pre-populated. You can determine the need for that at the fragment level (e.g. on configuration change), and you'll need to find a way to incorporate the initial answer as part of the view models that are used to populate answers. Note also that we will need to store a plain text representation of the answer to keep it exactly the same (e.g. if a user enters ' 2 ' then the spaces should be retained on configuration change). This may require additional considerations since the answers do have different types, and 'UserAnswer' cannot correctly capture these sorts of nuances in answers.

Also, keep in mind that this was not expect to be an easy issue, and I wouldn't be surprised if many different files need to be changed in order for this functionality to be properly built.

@rt4914
Copy link
Contributor

rt4914 commented Jan 23, 2021

Will be checking this on Monday. Thanks.

@rt4914
Copy link
Contributor

rt4914 commented Jan 27, 2021

Will discuss in meeting as per out chat.

@rt4914 rt4914 removed their assignment Jan 27, 2021
@@ -56,19 +57,24 @@ class FractionInteractionViewModel(
return userAnswerBuilder.build()
}

override fun setPendingAnswer(userAnswer: UserAnswer) {
answerText.set(userAnswer.plainAnswer)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenHenning Here I'm unable to update the View using this method is there any other way to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be specific on what isn’t working? It’s sometimes hard to tell from just the code. The more specific you can be the better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I'm able to send the userAnswer (pending answer after configuration change) to the FractionInteractionViewModel but i'm unable to update it using the ObservableField for answerText (updated it string -> ObservableField to update the Input through viewmodel). Here I'm able to get the value but I'm unable to update the FractionInteractionView.

Is the implementation correct
Is there any other way to update the FractionInteractionView using the ViewModel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Just to check: did you confirm:

  • setPendingAnswer is being called here
  • setPendingAnswer is being passed new data
  • The current value of answerText when setPendingAnswer is called isn't what you expect (e.g. empty)

If all are true, I'm not exactly sure what's wrong. observable fields should work out of the box, but you could try calling notifyChange on the view model (per https://github.com/oppia/oppia-android/blob/develop/app/src/main/java/org/oppia/android/app/viewmodel/ObservableViewModel.kt#L23). FWIW I think you can actually avoid observable field if you use notifyChange, but maybe start with both to reduce the number of things that could be wrong here.

Copy link
Contributor

@rt4914 rt4914 Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this:
answerText is already set to the value which is coming from setPendingAnswer but the FractionInteractionView is not displaying it.
Also, notifyChange() did not work.
I think the issues is something else because answerText is showing the correct value its just not visible in screen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'm wondering if this is behaving this way because we have a text watcher set. It might be the case that we aren't actually copying over the value to answerText directly from databinding (& instead rely on the text watcher). I won't have time to dig into this right now, but @FareesHussain I suggest searching for whether a text watcher can interfere with databinding.

@rt4914 rt4914 self-assigned this Feb 8, 2021
@rt4914 rt4914 removed their assignment Feb 9, 2021
@FareesHussain
Copy link
Contributor Author

FareesHussain commented Feb 10, 2021

@BenHenning I tried to use notifyChange by removing the ObservableField for the answerText but I'm unable to update the view similar to #2514 (comment)

Is there any other way to update the input value of FractionInteractionView through FractionInteractionViewModel

@FareesHussain FareesHussain removed their assignment Feb 10, 2021
@BenHenning
Copy link
Member

BenHenning commented Feb 13, 2021

@FareesHussain de-assigning myself for now. Like mentioned earlier, I suspect this is tied to the TextWatcher (e.g. it might be intercepting text changes rather than letting them propagate correctly with databinding) and will require some level of debugging to verify. If it is caused by that, then you may need to manually call setText on the custom view when the observable changes (which may require calling a custom method in the custom view class to setText rather than using the built-in adapter for TextView via the layout file).

@BenHenning BenHenning removed their assignment Feb 13, 2021
@BenHenning BenHenning added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Feb 24, 2021
@BenHenning BenHenning removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Feb 24, 2021
@anandwana001
Copy link
Contributor

@FareesHussain Is this PR blocked on something?

@anandwana001 anandwana001 added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label May 7, 2021
@anandwana001 anandwana001 removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label May 13, 2021
@anandwana001
Copy link
Contributor

@FareesHussain any update on this?

@FareesHussain
Copy link
Contributor Author

@FareesHussain any update on this?

will discuss this in today's meeting

@rt4914
Copy link
Contributor

rt4914 commented Aug 4, 2021

Un-assigning as I was unable to solve this.

@rt4914 rt4914 removed their assignment Aug 4, 2021
@oppiabot
Copy link

oppiabot bot commented Aug 14, 2021

Hi @FareesHussain, 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 3 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!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 14, 2021
@vrajdesai78 vrajdesai78 self-assigned this Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Corresponds to items that haven't seen a recent update and may be automatically closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Singleton class in domain to manage configuration changes
5 participants