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 #3488: Domain layer mechanism to resume explorations. [Blocked: #2476] #3507

Closed
wants to merge 32 commits into from

Conversation

MaskedCarrot
Copy link
Contributor

@MaskedCarrot MaskedCarrot commented Jul 20, 2021

Explanation

Fixes #3488

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.

@MaskedCarrot
Copy link
Contributor Author

MaskedCarrot commented Jul 20, 2021

@BenHenning Can you please take a look at these changes. I wanted your review because I am not sure if passing null (done in TopicLesssonsFragmentPresenter) is a good way to do this but right now this seems to be the only option to me.

So what I am doing here is TopicLessonsFragment ( or any other app module class that starts the exploration) will either launch ExplorationActivity or ResumeExplorationActivity (a new activity that will be added in one of the following PRs) depending upon the chapterPlayState ( if chapter play state is in_progress_saved/not_saved then ResumeExplorationActivity will be launched)

If ExplorationActivity is started there is no need to fetch the checkpoint so we will start the exploration and pass null in startPlayingExploration method of ExplorationDataController.

If the ResumeExplorationActivity is launched, ExplorationCheckpoint will be fetched. Then null or the fetched checkpoint will be passed the startPlayingExploration method of ExplorationDataController depending upon the option that the user chooses (start over or resume ).

Once the exploration is loaded, we will check if the checkpoint is null, then we will reset the StateDeck but if it is not null we will resume the exploration with the saved checkpoint.

@BenHenning
Copy link
Member

@MaskedCarrot I feel like feeding the checkpoint state back down to the domain layer is a bit of an inversion. Why not have the controller automatically fetch the checkpoint & resume if it's present? That seems like it would be an overall simplification to the system.

If there are other requirements I'm missing, that context might help provide insight on potentially a better communication strategy here vs. sending null.

@MaskedCarrot
Copy link
Contributor Author

MaskedCarrot commented Jul 21, 2021

@BenHenning Checkpoint state will not be used anywhere. Chapter play state will be used to decide if the exploration can be resumed or not ( exploration can only be resumed if it is IN_PROGRESS_SAVED/NOT_SAVED and the user chooses to resume the exploration). Chapter play state will not be sent to the domain layer. It will be used in the app layer to either launch exploration activity (when exploration does not resume) or ResumeExploration activity ( when the user has to decide if they wish to resume).

If we retrieve checkpoints in the domain layer then we will again face a similar problem of performing an async task in the domain layer as we did while saving checkpoints. To solve this we can change the retrieveCheckpoints function to return a deferred as we did to save checkpoints and then in invoke on completion method of this deferred we can load the state deck. But doing this will introduce some delay (I think it would be negligible to the user) in the exploration being loaded.

We can not load the exploration and retrieve the checkpoint simultaneously, we will have to start the function to fetch the checkpoint once the exploration is loaded because in invoke on completion method of the deferred we will have to load the statedeck and for that, the exploration has to be loaded first.

@BenHenning
Copy link
Member

BenHenning commented Jul 22, 2021

I see. In that case, passing through the app layer sounds good. Could we pass an empty checkpoint for the case when no checkpoint is available rather than null? That's probably a bit cleaner since proto defines an automatic "blank" sentinel value in its default instances. Would that work?

@MaskedCarrot
Copy link
Contributor Author

yes that will work

@MaskedCarrot MaskedCarrot marked this pull request as ready for review July 24, 2021 15:01
@MaskedCarrot MaskedCarrot removed their assignment Jul 24, 2021
@MaskedCarrot MaskedCarrot changed the base branch from develop to show-partial-progress-in-ui August 1, 2021 19:47
@MaskedCarrot
Copy link
Contributor Author

I have added two new progress controller methods, one to notify the state deck that an un-revealed solution is visible and the other to notify the state deck that an un-revealed hint is visible. I have also created a new proto, "HintState" which acts as a container to save all the local variables of the statePlayerRecyclerViewAssembler.hintHandler every time the state deck is notified (when an answer is submitted or a new hint/solution is visible or reveleaed). HintState is returned back with the ephemeral state to the UI layer and all the variables of statePlayerRecyclerViewAssembler.hintHandler are restored. (we have to create HintState in the UI layer instead of computing it in the domain layer because of the variables hintSequenceNumber and trackedAnswerCount which are not part of the domain layer and are essential if we want to restore the HintHandler back to the previous state).

Since HintHandler is restored every time an ephemeral state is returned, therefore this solves any issue with configuration changes. In future, this approach will also allow us to show hints selectively depending upon the checkpoint by modifying the HintState in the domain layer before sending it with the ephemeral state to the UI layer.

@MaskedCarrot
Copy link
Contributor Author

@BenHenning I have changed the approach to the one explained in the above comment. PTAL (mainly at ExplorationProgressController, StateFragmentPresenter and StatePlayerRecyclerViewAssemebler)

@BenHenning
Copy link
Member

Apologies, will need to look at this a bit later today (my time). That being said, your approach seems sound @MaskedCarrot, I just haven't yet verified the code.

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @MaskedCarrot. Left one high-level comment on specifically the hints & solutions piece, and didn't look closely at the resume bits yet since I think my comment may result in some more major changes to the piping (but hopefully an overall simplification).

@@ -4,6 +4,6 @@ import org.oppia.android.app.model.HelpIndex

/** Callback interface for when hints can be made available to the learner. */
interface ShowHintAvailabilityListener {
/** Called when a hint is available to be shown, or null if all hints have been revealed. */
/** Called when a hint is available to be shown, or if hints have been revealed. */
Copy link
Member

Choose a reason for hiding this comment

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

Nice fix.

@@ -205,10 +223,65 @@ class ExplorationProgressController @Inject constructor(
}
}

fun submitUnrevealedHintIsVisible(
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I'm starting to think these changes are more complicated than just moving the hint/solution logic to this controller.

I'm partly wondering if we could just move HintHandler to this class and, when a hint/solution availability changes, notify that the ephemeral state DataProvider has changed (and ensure the necessary hint/solution data is included within EphemeralState). At that point, all we need to track is which hints are revealed & whether the solution is revealed. All the extra piping to keep the UI & domain layers in sync would become much simpler.

Beyond that, we also can probably then simplify what needs to be stored to retain hint/solution state. One HintHandler is needed per session (& we should be resetting it across session states) which means we don't need to keep track of hint sequence number or tracked wrong answer count since these can be reasonably reset upon resuming a checkpoint. Ditto I think for isHintVisibleInLatestState (which would mean only HelpIndex actually needs to be stored within the checkpoint).

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 Moving hint handler to the domain layer will simplify the flow but it might create clashes with @yashraj-01's project. Because I think in one of the PRs he has to move the handler out of the StatePlayerRecyclerViewAssemebler as a separate class to show hints in developer options.

@BenHenning @yashraj-01 WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MaskedCarrot I'm not sure if this is a good idea. I also thought of moving the HintHandler to the domain layer but it interacts with the UI in some cases. For example, it calls the listeners while showing a hint. Also, in my PR #3613, we are putting the HintHandler modules in the FragmentComponent so, I'm not sure how it will affect your new implementation or vice-versa.

@BenHenning any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Good point--this is an unexpected clash between the two projects. Unfortunately, the complexity we're seeing here trying to make this all work might force our hand toward trying to move it to the domain layer.

My thinking here is that we change the flow to the following: StateFragment reactively shows hints/solution entirely based on EphemeralState (mitigating the need for the listener at all--instead, we would notify that the DataProvider has changed in that same location when the class is moved to the domain layer).

The module changes should be workable: we can just use ApplicationComponent, instead, since this becomes an application-level thing instead of a fragment-level thing. I suspect it won't change too much for your project @yashraj-01, but it will definitely change some of it. I also suspect that it will probably be easier to base your work off of @MaskedCarrot's, but we probably should have a separate, preliminary PR that moves the handler to the domain layer so that it's not coupled with the rest of this resume work & so that your work isn't blocked for as long.

Would this be workable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think moving the hint handler to the domain layer might get complicated. We have lifecycle aware timers that the hint handler uses to show hints. Since these timers are being observed as live data, they will have to remain in the app layer. Also, the delay for which the timer runs depends upon the current state, i.e. it is 60 seconds for the first hint, 30 for others and 10 seconds if the user submits an answer. Since the delay is variable and not fixed, we will have to compute that in the app layer.

So I think we will have to distribute the hint handler between the app and domain layers, which might complicate the solution.

@BenHenning WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I was able to move the hint handler to the domain layer, and I think it does simplify the implementation of both the hint handler and the resuming mechanism.
There are a couple of edge cases left that I still need to take care of. I will probably send a PR in a few hours or by tomorrow.

Copy link
Contributor Author

@MaskedCarrot MaskedCarrot Aug 8, 2021

Choose a reason for hiding this comment

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

Created a new PR (#3659) to move the hint handler to the domain module.

@BenHenning BenHenning assigned MaskedCarrot and unassigned BenHenning Aug 5, 2021
@MaskedCarrot MaskedCarrot changed the title Fix #3488: Domain layer mechanism to resume explorations. Fix #3488: Domain layer mechanism to resume explorations. [Blocked: #2476] Aug 6, 2021
Base automatically changed from show-partial-progress-in-ui to develop August 10, 2021 22:47
@MaskedCarrot
Copy link
Contributor Author

Closing this PR because now that the implementation hint handler has changed quite significantly, so there will be a lot of merge conflicts in this and it will be much easier to do everything in a new PR.

@MaskedCarrot MaskedCarrot deleted the domain-layer-mech-to-resume-checkpoints branch August 24, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement domain layer mechanism to resume explorations with checkpoints.
6 participants