Fix #3488: Domain layer mechanism to resume explorations. [Blocked: #2476]

[MaskedCarrot]

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]

@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]

@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]

@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]

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]

yes that will work

[MaskedCarrot]

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]

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

[BenHenning]

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.

[BenHenning]

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

[BenHenning]

Nice fix.

[BenHenning]

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

[MaskedCarrot]

@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?

[yashraj-01]

@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?

[BenHenning]

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?

[MaskedCarrot]

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?

[MaskedCarrot]

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.

[MaskedCarrot]

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

[MaskedCarrot]

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.