AppKitNavigation - #200
Draft
Mx-Iris wants to merge 55 commits into
Draft
Conversation
stephencelis
marked this pull request as draft
August 14, 2024 17:55
Member
|
Awesome work! We're looking forward to diving into it soon 😄 I've converted things to a draft for now, so just tap the "Ready for review" button whenever you think things are in a good place. |
Member
|
@Mx-Iris Thanks again for your work on this! We took it for a spin and are definitely excited to incorporate things back into the library. I think to start it would be nice to break things down into smaller PRs in order to progressively ship things. This PR is still great to leave open as a "full vision" of things that folks can take for a spin. And now we could start isolating smaller chunks of work that can land in Some potential steps/chunks:
If there are any obvious ways of breaking any step down further, it's appreciated, and should make it easier to review each PR and also get each PR merged more quickly rather than have any giant PR sit around for too long. Excited for this! |
Member
|
Oh, and another note: let's not worry about documentation or case studies at all right now. Those are typically things we save till the end when we're close to release, and we can discuss how we may want to approach that when things are further along and everything else has landed in |
Contributor
Author
|
@stephencelis I understand your point now. This branch is mainly for quickly implementing my ideas and verifying if they're effective. I'll follow your suggestion and break them down into smaller units, then submit PRs for each one individually. |
Co-authored-by: Stephen Celis <stephen.celis@gmail.com>
Co-authored-by: Stephen Celis <stephen.celis@gmail.com>
Co-authored-by: Stephen Celis <stephen.celis@gmail.com>
…indings # Conflicts: # Package.swift # Sources/AppKitNavigation/AppKitAnimation.swift # Sources/AppKitNavigation/Internal/Exports.swift # Sources/AppKitNavigation/UIBinding.swift # Sources/AppKitNavigation/UITransaction.swift # Sources/SwiftNavigation/Internal/ErrorMechanism.swift # Sources/SwiftNavigation/Internal/ToOptionalUnit.swift
# Conflicts: # Package.swift # Package@swift-6.0.swift # Sources/AppKitNavigation/AppKitAnimation.swift # Sources/AppKitNavigation/Internal/Exports.swift # Sources/AppKitNavigation/Navigation/Modal.swift # Sources/AppKitNavigation/Navigation/ModalContent.swift # Sources/AppKitNavigation/Navigation/ModalSessionContent.swift # Sources/AppKitNavigation/Navigation/NavigationContent.swift # Sources/AppKitNavigation/Navigation/NavigationObserver.swift # Sources/AppKitNavigation/Navigation/Presentation.swift # Sources/AppKitNavigation/Navigation/Sheet.swift # Sources/AppKitNavigation/Navigation/SheetContent.swift # Sources/AppKitNavigation/UIBinding.swift # Sources/AppKitNavigation/UITransaction.swift # Sources/AppKitNavigationShim/include/shim.h # Sources/AppKitNavigationShim/shim.m
# Conflicts: # Examples/Examples.xcodeproj/project.pbxproj # Package.swift # Package@swift-6.0.swift # Sources/AppKitNavigation/Bindings/NSAlert.swift # Sources/AppKitNavigation/Bindings/NSColorPanel.swift # Sources/AppKitNavigation/Bindings/NSColorWell.swift # Sources/AppKitNavigation/Bindings/NSControl.swift # Sources/AppKitNavigation/Bindings/NSDatePicker.swift # Sources/AppKitNavigation/Bindings/NSFontManager.swift # Sources/AppKitNavigation/Bindings/NSMenuItem.swift # Sources/AppKitNavigation/Bindings/NSPathControl.swift # Sources/AppKitNavigation/Bindings/NSSaveOpenPanel.swift # Sources/AppKitNavigation/Bindings/NSSegmentedControl.swift # Sources/AppKitNavigation/Bindings/NSSlider.swift # Sources/AppKitNavigation/Bindings/NSStepper.swift # Sources/AppKitNavigation/Bindings/NSSwitch.swift # Sources/AppKitNavigation/Bindings/NSTextField.swift # Sources/AppKitNavigation/SwiftUI/Representable.swift # Sources/AppKitNavigationShim/include/shim.h # Sources/AppKitNavigationShim/shim.m # SwiftNavigation.xcworkspace/xcshareddata/swiftpm/Package.resolved
# Conflicts: # Package.swift # Package@swift-6.0.swift # SwiftNavigation.xcworkspace/xcshareddata/swiftpm/Package.resolved
# Conflicts: # Examples/CaseStudies/UIKit/WiFiFeature/WiFiSettingsFeature.swift # Package.resolved # Package.swift # Package@swift-5.9.swift # SwiftNavigation.xcworkspace/xcshareddata/swiftpm/Package.resolved
# Conflicts: # Package.resolved
# Conflicts: # Package.resolved # Package.swift # Package@swift-5.9.swift # Sources/AppKitNavigation/Internal/Exports.swift # SwiftNavigation.xcworkspace/xcshareddata/swiftpm/Package.resolved
Adapt AppKitNavigation to the current state of the package after upstream removed `swift-concurrency-extras`, adopted `InternalImportsByDefault`, and enabled `ExistentialAny`. - Replace `import ConcurrencyExtras` with SwiftNavigation's internal `LockIsolated` (via `@_spi(Internals) public import SwiftNavigation`); switch `withValue`/`@UncheckedSendable` to `withLock`/`nonisolated(unsafe)` to match UIKitNavigation's migration. - Promote `import AppKit` to `public import AppKit` in files that expose AppKit types through public API (NSAlert/NSColorPanel/NSColorWell/ NSDatePicker/NSPathControl/NSProgressIndicator/NSSaveOpenPanel/ NSSegmentedControl/NSSlider/NSStepper/NSSwitch/NSTextField/Modal/ Presentation/PresentationContent/Sheet). - Add `any` to protocol-as-type uses (SheetContent, TargetActionProtocol, NSViewControllerPresentationAnimator). - Drop the redundant `import IssueReporting` in NSSegmentedControl, which caused an ambiguous `reportIssue` call under Xcode's stricter resolution. - Split NSAlert.addButton into two overloads to silence the "default expression for inference" warning. - Clean up Package.swift duplicates (xctest-dynamic-overlay dependency and AppKitNavigation target) and propagate AppKitNavigationShim + IdentifiedCollections to Package@swift-6.0/6.1 manifests so 6.0/6.1 toolchains also build.
Introduce `NSView.popover(...)` driven by `NSPopover` (not `NSViewController.present(_:asPopoverRelativeTo:...)`), mirroring the existing `PopoverContent` / `PopoverObserver` shape used by Sheet, Modal, and Presentation. - `PopoverContent` protocol + default `show`/`close` extension that wires `NSPopover.didCloseNotification` into the binding lifecycle so transient popovers dismissed by clicking outside reset the source-of-truth. - Fileprivate `_PopoverPresentation` wraps a user-supplied `NSViewController` with an internally-managed `NSPopover` so the public `content` closure stays VC-only; popover knobs (`behavior`, `animates`) move onto the `popover(...)` method itself. - `PopoverPresentationsObserver` singleton retains active popovers (an `NSPopover` is not self-retained by `show`) and clears them on close, mirroring `ModalWindowsObserver`. - Public API: 5 overloads on `NSView` (`isPresented`, `item: Identifiable` value/binding, `item:id:KeyPath` value/binding) with `relativeTo`, `preferredEdge`, `behavior`, `animates`, `onDismiss`.
`NSPopover.show(relativeTo:)` was added in macOS 14 to anchor a popover to a toolbar item. Expose it through the same family of `popover(...)` bindings already provided on `NSView`, omitting the `rect` and `preferredEdge` parameters that the toolbar-item API does not accept.
`Navigated.content` is held weakly and `NSPopover` does not retain its `PopoverContent` host, so `_PopoverPresentation` was deallocated as soon as `begin(...)` returned. The `didCloseNotification` sink captured the host weakly, so `onEndNavigation` ran on a nil reference, the binding was never reset to `false`, and `navigatedByID` retained the prior id — blocking any subsequent presentation. Capture `self` strongly in the close subscription; lifetime stays bounded because the sink removes the cancellable from `PopoverPresentationsObserver.shared` once the close notification fires.
# Conflicts: # Examples/CaseStudiesTests/NavigationPathTests.swift # Package.resolved
Expose `AppKit`, `Combine`, and `IdentifiedCollections` via `public import` so the types they surface satisfy access-level-on-imports checks, matching the recently enabled `CheckImplementationOnly` feature. `Package.resolved` picks up the newly pinned `swift-collections` and `swift-identified-collections`.
`bind(isAnimated:)` wrapped its observation in a freshly created `ObserveToken`
and returned it without retaining it anywhere. Since the method is marked
`@discardableResult`, the ordinary call site dropped that wrapper, it
deallocated at the end of the statement, and its `deinit` cancelled the
observation behind it. The indicator therefore honoured the binding once,
synchronously, and never reacted to a change again. `init(frame:isAnimated:)`
discards the token internally, so it had the same defect.
Every other binding in the module already stores its token on the receiver via
an associated object; this one was the only exception. Store it the same way and
add `unbindIsAnimated()` to match `unbindURL()` and `unbind(_:)`.
Adds an `AppKitNavigationTests` target, which the module did not have. The
regression test fails against the previous implementation with
`("[false]") is not equal to ("[false, true]")`.
`TargetActionProxy` took over a control's single `target` / `action` pair,
saving the previous one and forwarding to it. That keeps a pair installed
*before* the first binding working, but assigning `target` or `action`
afterwards silently unhooks the proxy, and every binding and closure action on
that control stops firing.
AppKit has stored several target-action pairs per control since macOS 11
(`addTarget:action:forControlEvents:`), which removes the need to seize the pair
at all. The API is only published in the macOS 27 SDK, so declare it in the shim
behind an SDK check that compiles away once that SDK is in use.
The events are not as old as the API, though. Measured on macOS 26.6, a button
click delivers only `trackingBegan` and `trackingEndedInside`; `valueChanged`
and `primaryActionTriggered` are never sent, and tracking events do not stand
for "the action fired" (a keyboard-driven button never tracks). So the proxy
registers as an additional target from macOS 27 on and keeps taking the pair
over below that.
Text fields deliberately do not register `valueChanged`: their per-keystroke
changes already arrive through `textDidChangeNotification` on every version, and
registering both would deliver each edit twice.
Two things to know when touching the shim:
- Do not copy `NS_SWIFT_NAME(NSControl.Events)` from the SDK. Nesting a type
inside a class owned by a different module makes the Swift importer drop the
whole declaration, methods included.
- Never spell the option set at the call site. It is `NSControlEvents` from the
shim and `NSControl.Events` from the macOS 27 SDK; an array literal infers
under both.
Built against both the macOS 26 and macOS 27 SDKs. The macOS 27 path itself is
unverified at runtime for want of a machine running it; the tests are written as
cross-path contracts so running them there confirms it.
Conflicts were confined to the package manifests and the resolved files.
- Kept the fork's `swift-identified-collections` dependency, which
AppKitNavigation needs and upstream does not carry, and took upstream's
versions for everything else.
- `Package.swift` moved to `swift-issue-reporting` for
`IssueReportingTestSupport`; the new `AppKitNavigationTests` target follows.
The 6.0 / 6.1 / 6.2 manifests still use `xctest-dynamic-overlay`, so the
target is declared with that package there.
- Upstream added `Package@swift-6.2.swift` carrying its own AppKitNavigation
target, which only depends on SwiftNavigation. Upstream's AppKitNavigation is
seven files (animation and binding bridges); the fork's is fifty and also
needs `AppKitNavigationShim` and `IdentifiedCollections`. Aligned that
manifest with `Package.swift` and added the shim and test targets, otherwise
a Swift 6.2 toolchain — which prefers that file — would fail to build.
- Regenerated `Package.resolved`. The workspace one already pins
identified-collections transitively through swift-sharing.
Verified: builds under both the Swift 6.2 and 6.4 toolchains, AppKitNavigationTests 8/8.
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
3 participants
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.
Hello everyone, I love AppKit. I am very pleased to see that UIKit has such an excellent framework available.
I have also watched videos from pointfreeco, where it is mentioned more than once that domain modeling is the most important thing, and using tools can make any platform serve the model.
I spent some time porting part of the API, and most of the NSControl can be use. Next, I will add some AppKit-specific navigation, such as sheet, modal, and some macOS-specific controls, and a lot of documentation still needs to be refined, as well as example projects and tests.
This PR is not yet complete, so please do not merge it yet!
For those who are interested, you can try running the example project