-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Ignore progress updates after suspending #6186
Conversation
@1ec5, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh to be potential reviewers. |
I'll delegate review here to someone more familiar with |
@@ -93,6 +95,11 @@ - (void)resume { | |||
- (void)suspend { | |||
MGLAssertOfflinePackIsValid(); | |||
|
|||
if (self.state == MGLOfflinePackStateActive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does resume
need to set _isSuspending to NO
? I'm not certain but it seems like a resumed offline pack may no longer report progress even if the state is set to MGLOfflinePackStateActive
because of a region status change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3b546b9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the question about the case of suspend
, this looks good to me.
adf5733
to
3fb254b
Compare
-suspend and -resume are now presumed to be synchronous, even if technically they remain asynchronous under the hood.
3fb254b
to
3b546b9
Compare
This is a cosmetic fix that makes an offline pack appear to stop downloading as soon as
-suspend
is called on it.Note that this change does not address the crash in #6092, which occurs because the
MBGLOfflineRegionObserver
’s weak pointer to the MGLOfflinePack object has gone stale by the time late notifications come in from thembgl::OfflineRegion
.MBGLOfflineRegionObserver
is owned bymbgl::OfflineRegion
and MGLOfflinePack doesn’t currently have a reference to it, although that might be one future direction for improvement.Fixes #5538.
/cc @jfirebaugh