Adapt alert call sites to the async LoopKit delegate API - #3
Merged
Conversation
next-dev does not currently compile. The dev merge (#2) brought in the alerts rework, which calls PumpManagerDelegate.issueAlert and retractAlert -- both async in the tidepool-synced LoopKit -- from synchronous closures: MedtrumPumpManager.swift:903: error: 'async' call in a function that does not support concurrency MedtrumKitUICoordinator.swift:288: error: same PatchSettingsViewModel.swift:138,139: error: same These adaptations were made and verified while preparing #2 but were lost before it was pushed; the merge landed without them. This restores them. Follows the pattern already used in OmnipodKit: - MedtrumPumpManager.emitAlert -- issueAlert, then hasNewPumpEvents - PatchSettingsViewModel -- retractAlert, then issueAlert - MedtrumKitUICoordinator.pumpRemoval -- retractAlert In each case both delegate calls stay inside a single Task so their relative order is preserved. WeakSynchronizedDelegate.notify dispatches its block onto the delegate queue, so these ran sequentially before the API became async. Wrapping only the first call would let them race, and in PatchSettingsViewModel that would risk issuing the new patch-expiry notification before the old one is retracted, leaving a stale alert. A sweep confirms no other un-awaited issueAlert/retractAlert call sites remain. Workspace builds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
next-devdoes not currently compile. #2 brought in the alerts rework, which callsPumpManagerDelegate.issueAlertandretractAlert— bothasyncin the tidepool-synced LoopKit — from synchronous closures:These adaptations were made and verified while preparing #2, but were lost before it was pushed — my error — so the merge landed without them. This restores them.
Follows the pattern already used in OmnipodKit (
Task { await …delegate?.issueAlert(…) }):MedtrumPumpManager.emitAlertissueAlert, thenhasNewPumpEventsPatchSettingsViewModelretractAlert, thenissueAlertMedtrumKitUICoordinator.pumpRemovalretractAlertIn each case both delegate calls stay inside a single
Taskso their relative order is preserved.WeakSynchronizedDelegate.notifydispatches its block onto the delegate queue, so these ran sequentially before the API became async. Wrapping only the first would let them race — and inPatchSettingsViewModelthat risks issuing the new patch-expiry notification before the old one is retracted, leaving a stale alert.A sweep confirms no other un-awaited
issueAlert/retractAlertcall sites remain. Full LoopWorkspace builds.