also looking at this
fix(ios): ask again when Core Location has no fix yet, rather than failing the caller
#264
3 files
+120
-7
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -407,6 +407,27 @@ describe('Craft iOS builder', () => { | ||
| 407 | 407 | expect(body.indexOf('content.userInfo = info')).toBeLessThan(body.indexOf('UNNotificationRequest(identifier: id, content: content')) |
| 408 | 408 | }) |
| 409 | 409 | |
| 410 | it('asks again when Core Location has no fix yet, rather than failing the caller', async () => { | |
| 411 | // #260: kCLErrorLocationUnknown rejected a getCurrentPosition whose fix | |
| 412 | // was seconds away. requestLocation gives up on it, so the one-shot asks | |
| 413 | // again while the same caller waits; its timeout still bounds the wait. | |
| 414 | const output = mkdtempSync(join(tmpdir(), 'craft-ios-location-no-fix-')) | |
| 415 | await init({ name: 'WildLoop', bundleId: 'org.wildloop.app', output, config: { enableGeolocation: true } }) | |
| 416 | const swift = readFileSync(join(output, 'Sources', 'WildLoopApp.swift'), 'utf8') | |
| 417 | ||
| 418 | const failed = swift.slice(swift.indexOf('didFailWithError error: Error) {')) | |
| 419 | const body = failed.slice(0, failed.indexOf('\n }\n')) | |
| 420 | const noFixYet = body.indexOf('nativeError.code == CLError.Code.locationUnknown.rawValue') | |
| 421 | expect(noFixYet).toBeGreaterThan(-1) | |
| 422 | // Decided before anything settles the caller or tells the page. | |
| 423 | expect(noFixYet).toBeLessThan(body.indexOf('finishSingleLocationRequest()')) | |
| 424 | expect(noFixYet).toBeLessThan(body.indexOf('rejectCallback(')) | |
| 425 | expect(noFixYet).toBeLessThan(body.indexOf('sendToWeb("craftLocationError"')) | |
| 426 | // Asks again only for the caller that was told to wait. | |
| 427 | expect(body).toContain('guard let self, self.singleLocationCallbackId == waiting else { return }') | |
| 428 | expect(body).toContain('self.locationManager?.requestLocation()') | |
| 429 | }) | |
| 430 | ||
| 410 | 431 | it('settles calendar callbacks only after a real EventKit operation', async () => { |
| 411 | 432 | const output = mkdtempSync(join(tmpdir(), 'craft-ios-calendar-')) |
| 412 | 433 | await init({ |
| @@ -4119,9 +4119,23 @@ struct CraftWebView: UIViewRepresentable { | ||
| 4119 | 4119 | } |
| 4120 | 4120 | |
| 4121 | 4121 | func locationManager(_ manager: CLLocationManager, didFailWithError error: Error) { |
| 4122 | let nativeError = error as NSError | |
| 4123 | // kCLErrorLocationUnknown is Core Location saying it has no fix | |
| 4124 | // *yet*, not that it failed. requestLocation gives up on it, so a | |
| 4125 | // one-shot asks again shortly instead of failing a caller whose | |
| 4126 | // fix was seconds off; its timeout still bounds the wait (#260). | |
| 4127 | // Nor is it an error event: a watch keeps running through it. | |
| 4128 | if nativeError.domain == kCLErrorDomain && nativeError.code == CLError.Code.locationUnknown.rawValue { | |
| 4129 | if let waiting = singleLocationCallbackId { | |
| 4130 | DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) { [weak self] in | |
| 4131 | guard let self, self.singleLocationCallbackId == waiting else { return } | |
| 4132 | self.locationManager?.requestLocation() | |
| 4133 | } | |
| 4134 | } | |
| 4135 | return | |
| 4136 | } | |
| 4122 | 4137 | let callbackId = singleLocationCallbackId |
| 4123 | 4138 | finishSingleLocationRequest() |
| 4124 | let nativeError = error as NSError | |
| 4125 | 4139 | let code = nativeError.domain == kCLErrorDomain && nativeError.code == CLError.Code.denied.rawValue |
| 4126 | 4140 | ? "PERMISSION_DENIED" |
| 4127 | 4141 | : "POSITION_UNAVAILABLE" |
| @@ -1779,6 +1779,20 @@ fn takePendingFix() ?PendingFix { | ||
| 1779 | 1779 | return call; |
| 1780 | 1780 | } |
| 1781 | 1781 | |
| 1782 | /// Read the slot without clearing it, for an answer that is not yet the answer. | |
| 1783 | fn peekPendingFix() ?PendingFix { | |
| 1784 | state_mutex.lock(); | |
| 1785 | defer state_mutex.unlock(); | |
| 1786 | return pending_fix; | |
| 1787 | } | |
| 1788 | ||
| 1789 | fn pendingFixIs(ticket: ios_async.Ticket) bool { | |
| 1790 | state_mutex.lock(); | |
| 1791 | defer state_mutex.unlock(); | |
| 1792 | const call = pending_fix orelse return false; | |
| 1793 | return call.ticket.index == ticket.index and call.ticket.generation == ticket.generation; | |
| 1794 | } | |
| 1795 | ||
| 1782 | 1796 | fn takePendingFixIf(ticket: ios_async.Ticket) ?PendingFix { |
| 1783 | 1797 | state_mutex.lock(); |
| 1784 | 1798 | defer state_mutex.unlock(); |
| @@ -1800,6 +1814,21 @@ fn schedulePositionTimeout(ticket: ios_async.Ticket, timeout_ms: u32) void { | ||
| 1800 | 1814 | ios_async.scheduleDeadline(ticket, timeout_ms, positionTimedOut); |
| 1801 | 1815 | } |
| 1802 | 1816 | |
| 1817 | /// How long a one-shot told "no fix yet" waits before asking again. | |
| 1818 | const no_fix_retry_ms: u32 = 1_000; | |
| 1819 | ||
| 1820 | /// Ask again for the fix `kCLErrorLocationUnknown` said was not there yet, | |
| 1821 | /// provided the call that was told to wait is still the one waiting. One that | |
| 1822 | /// has since been answered, timed out or displaced stops the asking, so its | |
| 1823 | /// own deadline is what bounds the whole wait. | |
| 1824 | fn retryPendingFix(context: ?*anyopaque) callconv(.c) void { | |
| 1825 | if (!is_darwin) return; | |
| 1826 | if (!pendingFixIs(ios_async.ticketFromDeadline(context))) return; | |
| 1827 | const mgr = ensureManager() catch return; | |
| 1828 | const sel_request = selector("requestLocation") catch return; | |
| 1829 | objc.msgSend(mgr, sel_request); | |
| 1830 | } | |
| 1831 | ||
| 1803 | 1832 | /// Record the running watch. Overwriting is the whole behaviour: a second |
| 1804 | 1833 | /// `watchPosition` is idempotent in Swift, and the selectors are the same ones. |
| 1805 | 1834 | fn publishWatch(sels: Sels) void { |
| @@ -1960,13 +1989,39 @@ fn didUpdateLocations(_: Id, _: Id, _: Id, locations: Id) callconv(.c) void { | ||
| 1960 | 1989 | |
| 1961 | 1990 | fn didFailWithError(_: Id, _: Id, _: Id, err_object: Id) callconv(.c) void { |
| 1962 | 1991 | if (!is_darwin) return; |
| 1992 | handleLocationFailure( | |
| 1993 | readNSString(err_object, "domain") orelse "(none)", | |
| 1994 | readNSInteger(err_object, "code"), | |
| 1995 | readNSString(err_object, "localizedDescription") orelse "(none)", | |
| 1996 | ); | |
| 1997 | } | |
| 1963 | 1998 | |
| 1964 | // Read once and reused below: the log line is the one channel a | |
| 1965 | // `BridgeError` enum leaves for a rejected caller, and the event is the | |
| 1966 | // only channel a watch has at all. | |
| 1967 | const description = readNSString(err_object, "localizedDescription") orelse "(none)"; | |
| 1968 | const domain = readNSString(err_object, "domain") orelse "(none)"; | |
| 1969 | const native_code = readNSInteger(err_object, "code"); | |
| 1999 | /// `kCLErrorLocationUnknown`: Core Location has no fix *yet*. | |
| 2000 | /// | |
| 2001 | /// Apple documents it as the manager being unable to get a location right | |
| 2002 | /// away, not as a failure. `requestLocation` gives up on it, though, so a | |
| 2003 | /// one-shot told this asks again shortly rather than failing a caller whose | |
| 2004 | /// fix was seconds off: a phone with a cold GPS, or a loaded simulator, where | |
| 2005 | /// it broke the slice job's whole location chain (#260). The caller's own | |
| 2006 | /// timeout still bounds the wait. | |
| 2007 | fn isNoFixYet(domain: []const u8, native_code: c_long) bool { | |
| 2008 | return native_code == 0 and std.mem.eql(u8, domain, "kCLErrorDomain"); | |
| 2009 | } | |
| 2010 | ||
| 2011 | /// What a CoreLocation error does, apart from reading it off the `NSError`. | |
| 2012 | fn handleLocationFailure(domain: []const u8, native_code: c_long, description: []const u8) void { | |
| 2013 | // Not a failure, so neither a rejection nor `craftLocationError`: a watch | |
| 2014 | // keeps running through it, since `startUpdatingLocation` does keep trying, | |
| 2015 | // and an error event would tell the page otherwise. | |
| 2016 | if (isNoFixYet(domain, native_code)) { | |
| 2017 | if (peekPendingFix()) |call| { | |
| 2018 | std.log.info("location: no fix yet (kCLErrorLocationUnknown); asking again in {d} ms", .{no_fix_retry_ms}); | |
| 2019 | ios_async.scheduleDeadline(call.ticket, no_fix_retry_ms, retryPendingFix); | |
| 2020 | } else { | |
| 2021 | std.log.info("location: no fix yet (kCLErrorLocationUnknown); Core Location is still trying", .{}); | |
| 2022 | } | |
| 2023 | return; | |
| 2024 | } | |
| 1970 | 2025 | |
| 1971 | 2026 | std.log.warn( |
| 1972 | 2027 | "location: CLLocationManager failed - domain={s} code={d} description={s}", |
| @@ -2584,6 +2639,29 @@ test "the error event carries Swift's one-key message object" { | ||
| 2584 | 2639 | try testing.expect(parsed.value.object.get("domain") == null); |
| 2585 | 2640 | } |
| 2586 | 2641 | |
| 2642 | test "no fix yet keeps the one-shot waiting, and a real failure still settles it" { | |
| 2643 | // #260: `kCLErrorLocationUnknown` rejected a getCurrentPosition whose fix | |
| 2644 | // was seconds away, and on a loaded simulator that broke the slice job's | |
| 2645 | // whole location chain. Core Location's own word for it is "not yet". | |
| 2646 | try testing.expect(isNoFixYet("kCLErrorDomain", 0)); | |
| 2647 | try testing.expect(!isNoFixYet("kCLErrorDomain", 1)); | |
| 2648 | try testing.expect(!isNoFixYet("another-domain", 0)); | |
| 2649 | ||
| 2650 | _ = takeWatchAndPending(); | |
| 2651 | const ticket: ios_async.Ticket = .{ .index = 4, .generation = 7 }; | |
| 2652 | try testing.expect(publishPendingFix(ticket, empty_sels) == null); | |
| 2653 | ||
| 2654 | handleLocationFailure("kCLErrorDomain", 0, "The operation couldn't be completed."); | |
| 2655 | const waiting = peekPendingFix() orelse return error.NoFixYetSettledTheCall; | |
| 2656 | try testing.expectEqual(ticket.generation, waiting.ticket.generation); | |
| 2657 | try testing.expect(pendingFixIs(ticket)); | |
| 2658 | try testing.expect(!pendingFixIs(.{ .index = 4, .generation = 8 })); | |
| 2659 | ||
| 2660 | // Anything else is still an answer, and it takes the call. | |
| 2661 | handleLocationFailure("kCLErrorDomain", 2, "network"); | |
| 2662 | try testing.expect(takePendingFix() == null); | |
| 2663 | } | |
| 2664 | ||
| 2587 | 2665 | test "only Core Location denial becomes a permission error" { |
| 2588 | 2666 | try testing.expectEqual( |
| 2589 | 2667 | bridge_error.BridgeError.PermissionDenied, |