Use dedicated animator instance per controller#6
Use dedicated animator instance per controller#6SplittyDev wants to merge 1 commit intoalmonk:mainfrom
Conversation
|
@SplittyDev is attempting to deploy a commit to the Replay Team on Vercel. A member of the Team first needs to authorize it. |
|
|
||
| // Wait for layout | ||
| DispatchQueue.main.async { | ||
| Task { |
There was a problem hiding this comment.
This change isn't technically required for the fix, but it's good practice to use structured concurrency instead of GCD nowadays. The Task inherits the MainActor from NSViewRepresentable, so this is perfectly safe to do.
There was a problem hiding this comment.
Pull request overview
This PR addresses a multi-controller issue in Bonsplit (e.g., macOS multi-window) by eliminating the global SplitAnimator singleton and ensuring animations are scoped to each SplitViewController.
Changes:
- Removed
SplitAnimator.sharedin favor of per-controller animator instances. - Added an
animatorproperty toSplitViewController. - Updated
SplitContainerViewto use the controller-provided animator (via environment lookup).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Sources/Bonsplit/Internal/Views/SplitContainerView.swift | Switches split-pane animation to use the environment controller’s animator instead of a singleton. |
| Sources/Bonsplit/Internal/Utilities/SplitAnimator.swift | Removes the shared singleton and allows constructing animator instances. |
| Sources/Bonsplit/Internal/Controllers/SplitViewController.swift | Introduces a per-controller SplitAnimator instance to avoid cross-controller interference. |
Comments suppressed due to low confidence (1)
Sources/Bonsplit/Internal/Views/SplitContainerView.swift:83
- The inner
Task { ... }runs from inside aDispatchQueue.main.async@Sendableclosure, so it does not guarantee MainActor / main-thread execution, yet it mutates AppKit views (isHidden,setPosition) and calls a@MainActoranimator. Consider replacing this withTask { @MainActor in ... }(and/orawait Task.yield()if you truly need to wait a runloop for layout) to keep UI work on the main actor and preserve the original “wait for layout” behavior.
// Wait for layout
Task {
// Show the new pane and animate
splitView.arrangedSubviews[newPaneIndex].isHidden = false
splitController.animator.animate(
splitView: splitView,
from: startPosition,
to: targetPosition
) {
context.coordinator.isAnimating = false
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Sources/Bonsplit/Internal/Controllers/SplitViewController.swift
Outdated
Show resolved
Hide resolved
Sources/Bonsplit/Internal/Controllers/SplitViewController.swift
Outdated
Show resolved
Hide resolved
30d9898 to
ce9bfb4
Compare
Why
When multiple Bonsplit controllers are used (such as in a macOS multi-window setting), opening new panes doesn't work anymore.
I've tracked this down to the
SplitAnimatorusing asharedinstance across multiple controllers.Changes
SplitAnimator/sharedSplitViewController/animatorproperty (internal API)SplitContainerViewto use theanimatorinstance from theSplitViewController, which is obtained from the environment values