offline: Consolidate conditions into mode #1279
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Close #1104. This PR fixes a problem with the
refreshablemodifier. It used to give a console warning:which indicates that the task in
refreshablewas canceled and destroyed prematurely (or the environmentRefreshActionwas changed in an unexpected way).Previously, this was mitigated by using an unstructured
Taskblock, as its lifespan isn't affected by the view; However, it still leads to the drag-down refresh animation to be cut short.Proposed Fix
The root cause of the bug was a messed lifecycle of the
refreshabletask.isLoadingModelsboolean was immediately set totruewhen theloadModels()method is called, causing the progress view to show up and destroy the refreshable modifier. Instead of using a conditional view, i.e.,moving everything into the
switch-caseallows the refreshable to stay put while the mode is being determined.Similarly, I moved the
offlineDisabledViewinto theswitch-case. It doesn't have to be moved but I figured why not - now all views are tidily nested under the same level.To Discuss
Previously, whenever we pull to refresh, the progress view will be there for the entire time of
loadModels(). Now, it only appears on the first method call. Is it OK?switch-case, an alternative is to move it to anoverlay. Do you prefer this approach?