From c37f04d9154d349863edd098303b744b16a93c82 Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Thu, 6 Aug 2026 20:53:56 -0700 Subject: [PATCH 1/3] test: cover horizontal tab insertion indices --- .../TabDropIndexResolverTests.swift | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 Tests/BonsplitTests/TabDropIndexResolverTests.swift diff --git a/Tests/BonsplitTests/TabDropIndexResolverTests.swift b/Tests/BonsplitTests/TabDropIndexResolverTests.swift new file mode 100644 index 00000000..5d2c9198 --- /dev/null +++ b/Tests/BonsplitTests/TabDropIndexResolverTests.swift @@ -0,0 +1,84 @@ +import CoreGraphics +import Testing +@testable import Bonsplit + +@Suite("Horizontal tab strip drop index") +struct TabDropIndexResolverTests { + private let bounds = CGRect(x: 0, y: 0, width: 360, height: 34) + private let tabFrames = [ + CGRect(x: 12, y: 0, width: 80, height: 34), + CGRect(x: 100, y: 0, width: 120, height: 34), + CGRect(x: 228, y: 0, width: 72, height: 34), + ] + + @Test("Pointer position resolves every insertion slot") + func resolvesEveryInsertionSlot() { + #expect(insertionIndex(x: 20) == 0) + #expect(insertionIndex(x: 96) == 1) + #expect(insertionIndex(x: 224) == 2) + #expect(insertionIndex(x: 320) == 3) + } + + @Test("Tab midpoints divide adjacent insertion slots") + func usesTabMidpointsAsBoundaries() { + #expect(insertionIndex(x: 51.9) == 0) + #expect(insertionIndex(x: 52) == 1) + #expect(insertionIndex(x: 159.9) == 1) + #expect(insertionIndex(x: 160) == 2) + #expect(insertionIndex(x: 263.9) == 2) + #expect(insertionIndex(x: 264) == 3) + } + + @Test("Pane-local geometry produces the same index in a translated split") + func translatedSplitUsesItsOwnLocalGeometry() { + let translatedBounds = bounds.offsetBy(dx: 500, dy: 120) + let translatedFrames = tabFrames.map { $0.offsetBy(dx: 500, dy: 120) } + + #expect( + TabDropIndexResolver.insertionIndex( + at: CGPoint(x: 596, y: 137), + in: translatedBounds, + tabFrames: translatedFrames + ) == 1 + ) + #expect( + TabDropIndexResolver.insertionIndex( + at: CGPoint(x: 724, y: 137), + in: translatedBounds, + tabFrames: translatedFrames + ) == 2 + ) + } + + @Test("Locations outside the strip are rejected") + func rejectsLocationsOutsideStrip() { + #expect(insertionIndex(x: -1) == nil) + #expect(insertionIndex(x: 361) == nil) + #expect( + TabDropIndexResolver.insertionIndex( + at: CGPoint(x: 96, y: 35), + in: bounds, + tabFrames: tabFrames + ) == nil + ) + } + + @Test("An empty strip accepts its only insertion slot") + func emptyStripUsesZeroIndex() { + #expect( + TabDropIndexResolver.insertionIndex( + at: CGPoint(x: 180, y: 17), + in: bounds, + tabFrames: [] + ) == 0 + ) + } + + private func insertionIndex(x: CGFloat) -> Int? { + TabDropIndexResolver.insertionIndex( + at: CGPoint(x: x, y: bounds.midY), + in: bounds, + tabFrames: tabFrames + ) + } +} From 7d2aa113169d0183b00cae0ec4d97e3a1605b102 Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Thu, 6 Aug 2026 21:33:16 -0700 Subject: [PATCH 2/3] fix: resolve horizontal tab insertion positions --- .../Views/TabBarDropDestinationNSView.swift | 148 +++++++ .../Views/TabBarDropDestinationView.swift | 29 ++ .../Internal/Views/TabBarDropHandler.swift | 182 ++++++++ .../Bonsplit/Internal/Views/TabBarView.swift | 397 +----------------- .../Internal/Views/TabDropIndexResolver.swift | 55 +++ .../Bonsplit/Public/BonsplitController.swift | 20 +- Tests/BonsplitTests/BonsplitTests.swift | 69 ++- .../TabBarDropHandlerTests.swift | 238 +++++++++++ ...TabDropDelegateSamePaneFallbackTests.swift | 131 ------ .../TabDropIndexResolverTests.swift | 25 ++ 10 files changed, 730 insertions(+), 564 deletions(-) create mode 100644 Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift create mode 100644 Sources/Bonsplit/Internal/Views/TabBarDropDestinationView.swift create mode 100644 Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift create mode 100644 Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift create mode 100644 Tests/BonsplitTests/TabBarDropHandlerTests.swift delete mode 100644 Tests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift diff --git a/Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift b/Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift new file mode 100644 index 00000000..d3150a01 --- /dev/null +++ b/Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift @@ -0,0 +1,148 @@ +import AppKit + +/// AppKit bridge that keeps one authoritative pointer-to-index calculation per strip. +@MainActor +final class TabBarDropDestinationNSView: NSView { + weak var pane: PaneState? + weak var bonsplitController: BonsplitController? + weak var splitViewController: SplitViewController? + weak var geometryRegistry: TabBarItemGeometryRegistry? + var onIndicatorIndexChanged: ((Int?) -> Void)? + + private var presentedIndicatorIndex: Int? + + override init(frame frameRect: NSRect) { + super.init(frame: frameRect) + registerForDraggedTypes([ + TabBarDropHandler.tabTransferPasteboardType, + .fileURL, + ]) + } + + @available(*, unavailable) + required init?(coder: NSCoder) { + nil + } + + override func hitTest(_ point: NSPoint) -> NSView? { + guard super.hitTest(point) === self else { return nil } + let dragPasteboard = NSPasteboard(name: .drag) + let handler = makeHandler() + guard Self.shouldCaptureHitTest( + eventType: NSApp.currentEvent?.type, + pasteboardTypes: dragPasteboard.types, + hasLocalTabDrag: handler?.hasLiveTabDrag == true + ), handler?.operation(for: dragPasteboard) != [] else { + return nil + } + return self + } + + override func draggingEntered(_ sender: any NSDraggingInfo) -> NSDragOperation { + updateDrag(sender) + } + + override func draggingUpdated(_ sender: any NSDraggingInfo) -> NSDragOperation { + updateDrag(sender) + } + + override func draggingExited(_ sender: (any NSDraggingInfo)?) { + clearDropState() + } + + override func prepareForDragOperation(_ sender: any NSDraggingInfo) -> Bool { + updateDrag(sender) != [] + } + + override func performDragOperation(_ sender: any NSDraggingInfo) -> Bool { + defer { clearDropState() } + guard let handler = makeHandler(), + let targetIndex = insertionIndex(for: sender) else { + return false + } + return handler.performDrop(from: sender.draggingPasteboard, at: targetIndex) + } + + override func concludeDragOperation(_ sender: (any NSDraggingInfo)?) { + clearDropState() + } + + override func draggingEnded(_ sender: any NSDraggingInfo) { + clearDropState() + } + + static func shouldCaptureHitTest( + eventType: NSEvent.EventType?, + pasteboardTypes: [NSPasteboard.PasteboardType]?, + hasLocalTabDrag: Bool = false + ) -> Bool { + let hasSupportedPasteboardType = pasteboardTypes?.contains( + TabBarDropHandler.tabTransferPasteboardType + ) == true || pasteboardTypes?.contains(.fileURL) == true + guard hasLocalTabDrag || hasSupportedPasteboardType else { + return false + } + guard let eventType else { return true } + switch eventType { + case .leftMouseDragged, + .leftMouseUp, + .mouseMoved, + .cursorUpdate, + .periodic, + .appKitDefined, + .applicationDefined, + .systemDefined: + return true + default: + return false + } + } + + private func updateDrag(_ sender: any NSDraggingInfo) -> NSDragOperation { + guard let handler = makeHandler(), + let targetIndex = insertionIndex(for: sender) else { + clearDropState() + return [] + } + let operation = handler.operation(for: sender.draggingPasteboard) + guard operation != [] else { + clearDropState() + return [] + } + presentIndicator(at: handler.indicatorIndex(for: targetIndex)) + return operation + } + + private func insertionIndex(for sender: any NSDraggingInfo) -> Int? { + guard let pane, let geometryRegistry else { return nil } + let indexedFrames = pane.tabs.enumerated().compactMap { index, tab -> TabDropIndexResolver.IndexedFrame? in + guard let frame = geometryRegistry.frame(for: tab.id, in: self) else { return nil } + return TabDropIndexResolver.IndexedFrame(index: index, frame: frame) + } + return TabDropIndexResolver.insertionIndex( + at: convert(sender.draggingLocation, from: nil), + in: bounds, + indexedTabFrames: indexedFrames, + tabCount: pane.tabs.count + ) + } + + private func makeHandler() -> TabBarDropHandler? { + guard let pane, let bonsplitController, let splitViewController else { return nil } + return TabBarDropHandler( + pane: pane, + bonsplitController: bonsplitController, + splitViewController: splitViewController + ) + } + + private func presentIndicator(at index: Int?) { + guard presentedIndicatorIndex != index else { return } + presentedIndicatorIndex = index + onIndicatorIndexChanged?(index) + } + + private func clearDropState() { + presentIndicator(at: nil) + } +} diff --git a/Sources/Bonsplit/Internal/Views/TabBarDropDestinationView.swift b/Sources/Bonsplit/Internal/Views/TabBarDropDestinationView.swift new file mode 100644 index 00000000..b3ab79a3 --- /dev/null +++ b/Sources/Bonsplit/Internal/Views/TabBarDropDestinationView.swift @@ -0,0 +1,29 @@ +import SwiftUI + +/// A single native drop destination spanning one pane's horizontal tab strip. +@MainActor +struct TabBarDropDestinationView: NSViewRepresentable { + let pane: PaneState + let bonsplitController: BonsplitController + let splitViewController: SplitViewController + let geometryRegistry: TabBarItemGeometryRegistry + let onIndicatorIndexChanged: (Int?) -> Void + + func makeNSView(context: Context) -> TabBarDropDestinationNSView { + let view = TabBarDropDestinationNSView() + configure(view) + return view + } + + func updateNSView(_ nsView: TabBarDropDestinationNSView, context: Context) { + configure(nsView) + } + + private func configure(_ view: TabBarDropDestinationNSView) { + view.pane = pane + view.bonsplitController = bonsplitController + view.splitViewController = splitViewController + view.geometryRegistry = geometryRegistry + view.onIndicatorIndexChanged = onIndicatorIndexChanged + } +} diff --git a/Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift b/Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift new file mode 100644 index 00000000..2ec7e41e --- /dev/null +++ b/Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift @@ -0,0 +1,182 @@ +import AppKit +import SwiftUI +import UniformTypeIdentifiers + +/// Applies tab-strip drops through the controller's shared tab mutation APIs. +@MainActor +struct TabBarDropHandler { + static let tabTransferPasteboardType = NSPasteboard.PasteboardType(UTType.tabTransfer.identifier) + + let pane: PaneState + let bonsplitController: BonsplitController + let splitViewController: SplitViewController + + var hasLiveTabDrag: Bool { + liveDragSourcePaneId != nil + } + + func operation(for pasteboard: NSPasteboard) -> NSDragOperation { + guard splitViewController.isInteractive else { return [] } + + let types = pasteboard.types ?? [] + let hasTabTransfer = types.contains(Self.tabTransferPasteboardType) + let hasFileURL = types.contains(.fileURL) + + // A file-only pasteboard is authoritative even if a cancelled local tab + // drag has not finished clearing its in-memory state. + if hasFileURL, !hasTabTransfer { + guard bonsplitController.onExternalFileDrop != nil || splitViewController.onFileDrop != nil, + UnifiedPaneDropDelegate.hasReadableFileURLs(from: pasteboard) else { + return [] + } + return .copy + } + + // Item-provider publication can trail the start of a local SwiftUI drag. + // The controller's synchronous drag state is authoritative in that window. + if let sourcePaneId = liveDragSourcePaneId { + return permitsTabMove(from: sourcePaneId) ? .move : [] + } + + if hasTabTransfer { + guard let transfer = Self.decodeTransfer(from: pasteboard), transfer.isFromCurrentProcess else { + return [] + } + return permitsTabMove(from: PaneID(id: transfer.sourcePaneId)) ? .move : [] + } + + return [] + } + + func indicatorIndex(for targetIndex: Int) -> Int? { + guard let sourceIndex = liveSamePaneSourceIndex else { return targetIndex } + return Self.isNoopSamePaneTarget(sourceIndex: sourceIndex, targetIndex: targetIndex) + ? nil + : targetIndex + } + + func performDrop(from pasteboard: NSPasteboard, at targetIndex: Int) -> Bool { + guard operation(for: pasteboard) != [] else { return false } + let hasTabTransfer = pasteboard.types?.contains(Self.tabTransferPasteboardType) == true + let hasFileURL = pasteboard.types?.contains(.fileURL) == true + + if hasFileURL, !hasTabTransfer { + return performFileDrop(from: pasteboard, at: targetIndex) + } + if hasLiveTabDrag || hasTabTransfer { + return performTabDrop(from: pasteboard, at: targetIndex) + } + return false + } + + static func isNoopSamePaneTarget(sourceIndex: Int, targetIndex: Int) -> Bool { + // Insertion indices use the original array's coordinates. Removing the source + // makes its own slot and the slot immediately after it equivalent no-ops. + targetIndex == sourceIndex || targetIndex == sourceIndex + 1 + } + + static func sameProcessFallbackRequest( + transfer: TabTransferData, + targetPane: PaneID, + targetIndex: Int, + allowCrossPaneTabMove: Bool + ) -> BonsplitController.ExternalTabDropRequest? { + guard transfer.isFromCurrentProcess else { return nil } + let sourcePaneId = PaneID(id: transfer.sourcePaneId) + guard sourcePaneId != targetPane, allowCrossPaneTabMove else { return nil } + return BonsplitController.ExternalTabDropRequest( + tabId: TabID(id: transfer.tab.id), + sourcePaneId: sourcePaneId, + destination: .insert(targetPane: targetPane, targetIndex: targetIndex) + ) + } + + private var liveDragSourcePaneId: PaneID? { + guard splitViewController.activeDragTab != nil || splitViewController.draggingTab != nil else { + return nil + } + return splitViewController.activeDragSourcePaneId ?? splitViewController.dragSourcePaneId + } + + private var liveSamePaneSourceIndex: Int? { + guard let draggedTab = splitViewController.activeDragTab ?? splitViewController.draggingTab, + liveDragSourcePaneId == pane.id else { + return nil + } + return pane.tabs.firstIndex(where: { $0.id == draggedTab.id }) + } + + private func permitsTabMove(from sourcePaneId: PaneID) -> Bool { + sourcePaneId == pane.id + ? bonsplitController.configuration.allowTabReordering + : bonsplitController.configuration.allowCrossPaneTabMove + } + + private func performTabDrop(from pasteboard: NSPasteboard, at targetIndex: Int) -> Bool { + if let draggedTab = splitViewController.activeDragTab ?? splitViewController.draggingTab, + let sourcePaneId = liveDragSourcePaneId { + defer { splitViewController.clearTabDragState() } + + if sourcePaneId == pane.id, + let sourceIndex = pane.tabs.firstIndex(where: { $0.id == draggedTab.id }), + Self.isNoopSamePaneTarget(sourceIndex: sourceIndex, targetIndex: targetIndex) { + return true + } + + var handled = false + withTransaction(Transaction(animation: nil)) { + if sourcePaneId == pane.id { + handled = bonsplitController.reorderTab(TabID(id: draggedTab.id), toIndex: targetIndex) + } else { + handled = bonsplitController.moveTab( + TabID(id: draggedTab.id), + toPane: pane.id, + atIndex: targetIndex + ) + } + } + return handled + } + + guard let transfer = Self.decodeTransfer(from: pasteboard), + let request = Self.sameProcessFallbackRequest( + transfer: transfer, + targetPane: pane.id, + targetIndex: targetIndex, + allowCrossPaneTabMove: bonsplitController.configuration.allowCrossPaneTabMove + ) else { + return false + } + let handled = bonsplitController.onExternalTabDrop?(request) ?? false + if handled { + splitViewController.clearTabDragState() + } + return handled + } + + private func performFileDrop(from pasteboard: NSPasteboard, at targetIndex: Int) -> Bool { + let urls = UnifiedPaneDropDelegate.fileURLs(from: pasteboard) + guard !urls.isEmpty else { return false } + + let destination: BonsplitController.ExternalTabDropRequest.Destination = .insert( + targetPane: pane.id, + targetIndex: targetIndex + ) + if let handler = bonsplitController.onExternalFileDrop { + return handler(BonsplitController.ExternalFileDropRequest(urls: urls, destination: destination)) + } + return splitViewController.onFileDrop?(urls, pane.id) ?? false + } + + private static func decodeTransfer(from pasteboard: NSPasteboard) -> TabTransferData? { + if let data = pasteboard.data(forType: tabTransferPasteboardType), + let transfer = try? JSONDecoder().decode(TabTransferData.self, from: data) { + return transfer + } + guard let rawValue = pasteboard.string(forType: tabTransferPasteboardType), + let data = rawValue.data(using: .utf8) else { + return nil + } + return try? JSONDecoder().decode(TabTransferData.self, from: data) + } +} diff --git a/Sources/Bonsplit/Internal/Views/TabBarView.swift b/Sources/Bonsplit/Internal/Views/TabBarView.swift index ab7e1020..f467c51f 100644 --- a/Sources/Bonsplit/Internal/Views/TabBarView.swift +++ b/Sources/Bonsplit/Internal/Views/TabBarView.swift @@ -803,7 +803,6 @@ struct TabBarView: View { @AppStorage("debugFadeColorStyle") private var fadeColorStyle = -1 @State private var isHoveringTabBar = false @State private var dropTargetIndex: Int? - @State private var dropLifecycle: TabDropLifecycle = .idle @State private var scrollOffset: CGFloat = 0 @State private var contentWidth: CGFloat = 0 @State private var tabContentWidthExcludingSplitButtonLane: CGFloat? @@ -1015,6 +1014,20 @@ struct TabBarView: View { ) } + private var tabDropDestination: some View { + TabBarDropDestinationView( + pane: pane, + bonsplitController: controller, + splitViewController: splitViewController, + geometryRegistry: tabItemGeometryRegistry + ) { targetIndex in + guard dropTargetIndex != targetIndex else { return } + withTransaction(Transaction(animation: nil)) { + dropTargetIndex = targetIndex + } + } + } + private func focusPaneFromTabBarChrome() -> Bool { guard !isFocused else { return false } withTransaction(Transaction(animation: nil)) { @@ -1094,14 +1107,6 @@ struct TabBarView: View { performNewTerminalSplitButtonAction() } .frame(width: trailing + 30, height: tabBarHeight) - .onDrop(of: [.tabTransfer, .fileURL], delegate: TabDropDelegate( - targetIndex: pane.tabs.count, - pane: pane, - bonsplitController: controller, - controller: splitViewController, - dropTargetIndex: $dropTargetIndex, - dropLifecycle: $dropLifecycle - )) } } .coordinateSpace(name: "tabScroll") @@ -1149,6 +1154,7 @@ struct TabBarView: View { .overlay( TabBarHoverTrackingView { updateTabBarHover($0) } ) + .overlay(tabDropDestination) .background { if splitViewController.tabShortcutHintsEnabled { TabBarHostWindowReader { window in @@ -1168,7 +1174,6 @@ struct TabBarView: View { #endif if newValue == nil { dropTargetIndex = nil - dropLifecycle = .idle } } .onAppear { @@ -1279,14 +1284,6 @@ struct TabBarView: View { } preview: { TabDragPreview(tab: tab, appearance: appearance) } - .onDrop(of: [.tabTransfer, .fileURL], delegate: TabDropDelegate( - targetIndex: index, - pane: pane, - bonsplitController: controller, - controller: splitViewController, - dropTargetIndex: $dropTargetIndex, - dropLifecycle: $dropLifecycle - )) .overlay(alignment: .leading) { if dropTargetIndex == index { dropIndicator @@ -1311,7 +1308,6 @@ struct TabBarView: View { private func createItemProvider(for tab: TabItem) -> NSItemProvider { splitViewController.makeTabDragItemProvider(for: tab, from: pane.id) { dropTargetIndex = nil - dropLifecycle = .idle } } @@ -1345,14 +1341,6 @@ struct TabBarView: View { performNewTerminalSplitButtonAction() } .frame(width: 30, height: tabBarHeight) - .onDrop(of: [.tabTransfer, .fileURL], delegate: TabDropDelegate( - targetIndex: pane.tabs.count, - pane: pane, - bonsplitController: controller, - controller: splitViewController, - dropTargetIndex: $dropTargetIndex, - dropLifecycle: $dropLifecycle - )) .overlay(alignment: .leading) { if dropTargetIndex == pane.tabs.count { dropIndicator @@ -2876,358 +2864,3 @@ private final class TabControlShortcutKeyMonitor { } } } - - -/// Drop lifecycle state to prevent dropUpdated from re-setting state after performDrop -enum TabDropLifecycle { - case idle - case hovering -} - -// MARK: - Tab Drop Delegate - -struct TabDropDelegate: DropDelegate { - let targetIndex: Int - let pane: PaneState - let bonsplitController: BonsplitController - let controller: SplitViewController - @Binding var dropTargetIndex: Int? - @Binding var dropLifecycle: TabDropLifecycle - - func performDrop(info: DropInfo) -> Bool { - #if DEBUG - NSLog("[Bonsplit Drag] performDrop called, targetIndex: \(targetIndex)") - #endif -#if DEBUG - dlog( - "tab.drop pane=\(pane.id.id.uuidString.prefix(5)) " + - "targetIndex=\(targetIndex)" - ) -#endif - - // Ensure all drag/drop side-effects run on the main actor. SwiftUI can call these - // callbacks off-main, and SplitViewController is @MainActor. - if !Thread.isMainThread { - return DispatchQueue.main.sync { - performDrop(info: info) - } - } - - // Read from non-observable drag state — @Observable writes from createItemProvider - // may not have propagated yet when performDrop runs. - guard let draggedTab = controller.activeDragTab ?? controller.draggingTab, - let sourcePaneId = controller.activeDragSourcePaneId ?? controller.dragSourcePaneId else { - if let transfer = decodeTransfer(from: info) { - if transfer.isFromCurrentProcess { - return performSameProcessTransfer(transfer) - } - } - - return performFileDrop(info: info) - } - - if sourcePaneId == pane.id { - guard bonsplitController.configuration.allowTabReordering else { return false } - } else { - guard bonsplitController.configuration.allowCrossPaneTabMove else { return false } - } - - var handled = false - if sourcePaneId == pane.id { - handled = performSamePaneReorder(tabId: draggedTab.id) - } else { - withTransaction(Transaction(animation: nil)) { - handled = bonsplitController.moveTab( - TabID(id: draggedTab.id), - toPane: pane.id, - atIndex: targetIndex - ) - } - } - - // Clear visual state immediately to prevent lingering indicators. - // Must happen synchronously before returning, not in async callback. - // Setting dropLifecycle to idle prevents dropUpdated from re-setting dropTargetIndex. - clearDropState() - clearControllerDragState() - - return handled - } - - func dropEntered(info: DropInfo) { - #if DEBUG - NSLog("[Bonsplit Drag] dropEntered at index: \(targetIndex)") - dlog( - "tab.dropEntered pane=\(pane.id.id.uuidString.prefix(5)) targetIndex=\(targetIndex) " + - "hasDrag=\(controller.draggingTab != nil ? 1 : 0) " + - "hasActive=\(controller.activeDragTab != nil ? 1 : 0)" - ) - #endif - dropLifecycle = .hovering - updateDropTargetForHover() - } - - func dropExited(info: DropInfo) { - #if DEBUG - NSLog("[Bonsplit Drag] dropExited from index: \(targetIndex)") - dlog("tab.dropExited pane=\(pane.id.id.uuidString.prefix(5)) targetIndex=\(targetIndex)") - #endif - dropLifecycle = .idle - if dropTargetIndex == targetIndex { - dropTargetIndex = nil - } - } - - func dropUpdated(info: DropInfo) -> DropProposal? { - // Guard against dropUpdated firing after performDrop/dropExited - // This is the key fix for the lingering indicator bug - guard dropLifecycle == .hovering else { -#if DEBUG - dlog("tab.dropUpdated.skip pane=\(pane.id.id.uuidString.prefix(5)) targetIndex=\(targetIndex) reason=lifecycle_idle") -#endif - return DropProposal(operation: dropOperation(for: info)) - } - // Only update if this is the active target, and suppress same-pane no-op indicators. - updateDropTargetForHover() -#if DEBUG - dlog( - "tab.dropUpdated pane=\(pane.id.id.uuidString.prefix(5)) targetIndex=\(targetIndex) " + - "dropTarget=\(dropTargetIndex.map(String.init) ?? "nil")" - ) -#endif - return DropProposal(operation: dropOperation(for: info)) - } - - func validateDrop(info: DropInfo) -> Bool { - // Reject drops on inactive workspaces whose views are kept alive in a ZStack. - guard controller.isInteractive else { -#if DEBUG - dlog("tab.validateDrop pane=\(pane.id.id.uuidString.prefix(5)) allowed=0 reason=inactive") -#endif - return false - } - let hasTabTransfer = info.hasItemsConforming(to: [.tabTransfer]) - let hasFileURL = info.hasItemsConforming(to: [.fileURL]) - guard hasTabTransfer || hasFileURL else { return false } - - if hasFileURL, !hasTabTransfer { - return canHandleFileDrop(info: info) - } - - // Local drags use in-memory state and are always same-process. - if controller.activeDragTab != nil || controller.draggingTab != nil { - let sourcePaneID = controller.activeDragSourcePaneId ?? controller.dragSourcePaneId - if sourcePaneID == pane.id { - return bonsplitController.configuration.allowTabReordering - } - return bonsplitController.configuration.allowCrossPaneTabMove - } - - // Drops whose live drag state has already been cleared must still include - // a same-process payload so we can distinguish tab moves from file drops. - guard let transfer = decodeTransfer(from: info), - transfer.isFromCurrentProcess else { - return false - } - let sourcePaneID = PaneID(id: transfer.sourcePaneId) - if sourcePaneID == pane.id { - guard bonsplitController.configuration.allowTabReordering else { return false } - } else { - guard bonsplitController.configuration.allowCrossPaneTabMove else { return false } - } -#if DEBUG - let hasDrag = controller.draggingTab != nil - let hasActive = controller.activeDragTab != nil - dlog( - "tab.validateDrop pane=\(pane.id.id.uuidString.prefix(5)) " + - "allowed=\(hasTabTransfer ? 1 : 0) hasDrag=\(hasDrag ? 1 : 0) hasActive=\(hasActive ? 1 : 0)" - ) -#endif - return true - } - - private func clearDropState() { - dropLifecycle = .idle - dropTargetIndex = nil - } - - private func clearControllerDragState() { - controller.draggingTab = nil - controller.dragSourcePaneId = nil - controller.activeDragTab = nil - controller.activeDragSourcePaneId = nil - } - - static func samePaneDropTarget(sourceIndex: Int, targetIndex: Int) -> Int? { - isNoopSamePaneTarget(sourceIndex: sourceIndex, targetIndex: targetIndex) ? nil : targetIndex - } - - static func isNoopSamePaneTarget(sourceIndex: Int, targetIndex: Int) -> Bool { - // Insertion indices are expressed in "original array" coordinates; after removal, - // inserting at `sourceIndex` or `sourceIndex + 1` results in no change. - targetIndex == sourceIndex || targetIndex == sourceIndex + 1 - } - - static func sameProcessFallbackRequest( - transfer: TabTransferData, - targetPane: PaneID, - targetIndex: Int, - allowCrossPaneTabMove: Bool - ) -> BonsplitController.ExternalTabDropRequest? { - guard transfer.isFromCurrentProcess else { return nil } - let sourcePaneId = PaneID(id: transfer.sourcePaneId) - guard sourcePaneId != targetPane, - allowCrossPaneTabMove else { - return nil - } - return BonsplitController.ExternalTabDropRequest( - tabId: TabID(id: transfer.tab.id), - sourcePaneId: sourcePaneId, - destination: .insert(targetPane: targetPane, targetIndex: targetIndex) - ) - } - - private func performSameProcessTransfer(_ transfer: TabTransferData) -> Bool { - let sourcePaneId = PaneID(id: transfer.sourcePaneId) - if sourcePaneId == pane.id { - // Same-pane reorders are applied while live drag state is still present. - // A pasteboard-only same-pane callback is stale and can otherwise apply - // the same transfer again through another drop target. - clearDropState() - return false - } - - guard let request = Self.sameProcessFallbackRequest( - transfer: transfer, - targetPane: pane.id, - targetIndex: targetIndex, - allowCrossPaneTabMove: bonsplitController.configuration.allowCrossPaneTabMove - ) else { - return false - } - let handled = bonsplitController.onExternalTabDrop?(request) ?? false - if handled { - clearDropState() - clearControllerDragState() - } - return handled - } - - private func performSamePaneReorder(tabId: UUID) -> Bool { - Self.performSamePaneReorder( - tabId: tabId, - targetIndex: targetIndex, - pane: pane, - bonsplitController: bonsplitController - ) - } - - @discardableResult - static func performSamePaneReorder( - tabId: UUID, - targetIndex: Int, - pane: PaneState, - bonsplitController: BonsplitController - ) -> Bool { - guard let sourceIndex = pane.tabs.firstIndex(where: { $0.id == tabId }) else { - return false - } - guard let destinationIndex = samePaneDropTarget(sourceIndex: sourceIndex, targetIndex: targetIndex) else { - return true - } - - let orderBeforeReorder = pane.tabs.map { $0.id } - withTransaction(Transaction(animation: nil)) { - pane.moveTab(from: sourceIndex, to: destinationIndex) - } - if pane.tabs.map({ $0.id }) != orderBeforeReorder { - bonsplitController.delegate?.splitTabBar( - bonsplitController, - didReorderTabsInPane: pane.id, - orderedTabIds: pane.tabs.map { TabID(id: $0.id) } - ) - bonsplitController.selectTab(TabID(id: tabId)) - bonsplitController.notifyGeometryChange() - } - return true - } - - private func dropOperation(for info: DropInfo) -> DropOperation { - info.hasItemsConforming(to: [.fileURL]) && !info.hasItemsConforming(to: [.tabTransfer]) ? .copy : .move - } - - private func canHandleFileDrop(info: DropInfo) -> Bool { - guard info.hasItemsConforming(to: [.fileURL]) else { return false } - guard bonsplitController.onExternalFileDrop != nil || controller.onFileDrop != nil else { return false } - return UnifiedPaneDropDelegate.hasReadableFileURLs() - } - - private func performFileDrop(info: DropInfo) -> Bool { - guard canHandleFileDrop(info: info) else { return false } - let urls = UnifiedPaneDropDelegate.fileURLs(from: NSPasteboard(name: .drag)) - guard !urls.isEmpty else { return false } - - let destination: BonsplitController.ExternalTabDropRequest.Destination = .insert( - targetPane: pane.id, - targetIndex: targetIndex - ) - let handled = bonsplitController.onExternalFileDrop?( - BonsplitController.ExternalFileDropRequest(urls: urls, destination: destination) - ) ?? controller.onFileDrop?(urls, pane.id) ?? false - if handled { - clearDropState() - } - return handled - } - - private func updateDropTargetForHover() { - if let sourceIndex = samePaneLocalDragSourceIndex() { - dropTargetIndex = Self.samePaneDropTarget(sourceIndex: sourceIndex, targetIndex: targetIndex) - return - } - - if shouldSuppressIndicatorForNoopSamePaneDrop() { - if dropTargetIndex == targetIndex { - dropTargetIndex = nil - } - } else if dropTargetIndex != targetIndex { - dropTargetIndex = targetIndex - } - } - - private func samePaneLocalDragSourceIndex() -> Int? { - guard let draggedTab = controller.activeDragTab ?? controller.draggingTab, - (controller.activeDragSourcePaneId ?? controller.dragSourcePaneId) == pane.id else { - return nil - } - return pane.tabs.firstIndex(where: { $0.id == draggedTab.id }) - } - - private func shouldSuppressIndicatorForNoopSamePaneDrop() -> Bool { - guard let sourceIndex = samePaneLocalDragSourceIndex() else { - return false - } - return Self.isNoopSamePaneTarget(sourceIndex: sourceIndex, targetIndex: targetIndex) - } - - private func decodeTransfer(from string: String) -> TabTransferData? { - guard let data = string.data(using: .utf8), - let transfer = try? JSONDecoder().decode(TabTransferData.self, from: data) else { - return nil - } - return transfer - } - - private func decodeTransfer(from info: DropInfo) -> TabTransferData? { - let pasteboard = NSPasteboard(name: .drag) - let type = NSPasteboard.PasteboardType(UTType.tabTransfer.identifier) - if let data = pasteboard.data(forType: type), - let transfer = try? JSONDecoder().decode(TabTransferData.self, from: data) { - return transfer - } - if let raw = pasteboard.string(forType: type) { - return decodeTransfer(from: raw) - } - return nil - } -} diff --git a/Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift b/Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift new file mode 100644 index 00000000..0a9207df --- /dev/null +++ b/Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift @@ -0,0 +1,55 @@ +import CoreGraphics + +/// Resolves a pointer in a horizontal tab strip to an insertion index. +struct TabDropIndexResolver { + struct IndexedFrame { + let index: Int + let frame: CGRect + } + + static func insertionIndex( + at location: CGPoint, + in bounds: CGRect, + tabFrames: [CGRect] + ) -> Int? { + insertionIndex( + at: location, + in: bounds, + indexedTabFrames: tabFrames.enumerated().map { IndexedFrame(index: $0.offset, frame: $0.element) }, + tabCount: tabFrames.count + ) + } + + static func insertionIndex( + at location: CGPoint, + in bounds: CGRect, + indexedTabFrames: [IndexedFrame], + tabCount: Int + ) -> Int? { + guard bounds.contains(location), tabCount >= 0 else { return nil } + guard tabCount > 0 else { return 0 } + guard !indexedTabFrames.isEmpty, + indexedTabFrames.allSatisfy({ item in + (0.. 0 + && item.frame.midX.isFinite + }), + Set(indexedTabFrames.map(\.index)).count == indexedTabFrames.count else { + return nil + } + + let visuallyOrderedFrames = indexedTabFrames.sorted { lhs, rhs in + if lhs.frame.midX == rhs.frame.midX { + return lhs.index < rhs.index + } + return lhs.frame.midX < rhs.frame.midX + } + + for item in visuallyOrderedFrames where location.x < item.frame.midX { + return item.index + } + + guard let lastItem = visuallyOrderedFrames.last else { return nil } + return min(lastItem.index + 1, tabCount) + } +} diff --git a/Sources/Bonsplit/Public/BonsplitController.swift b/Sources/Bonsplit/Public/BonsplitController.swift index edcd9bc3..0a8109ae 100644 --- a/Sources/Bonsplit/Public/BonsplitController.swift +++ b/Sources/Bonsplit/Public/BonsplitController.swift @@ -445,18 +445,11 @@ public final class BonsplitController { let sourcePaneId = sourcePane.id if sourcePaneId == targetPane.id { - guard configuration.allowTabReordering else { return false } - // Reorder within same pane. let destinationIndex: Int = { if let index { return max(0, min(index, sourcePane.tabs.count)) } return sourcePane.tabs.count }() - sourcePane.moveTab(from: sourceIndex, to: destinationIndex) - sourcePane.selectTab(tabItem.id) - internalController.focusPane(sourcePane.id) - delegate?.splitTabBar(self, didSelectTab: movedTab, inPane: sourcePane.id) - notifyGeometryChange() - return true + return reorderTab(tabId, toIndex: destinationIndex) } guard configuration.allowCrossPaneTabMove else { return false } @@ -466,7 +459,7 @@ public final class BonsplitController { return true } - /// Reorder a tab within its pane. + /// Reorder a tab within its pane and report an actual order change to the delegate. /// - Parameters: /// - tabId: The tab to reorder. /// - toIndex: Destination index. @@ -476,9 +469,18 @@ public final class BonsplitController { guard configuration.allowTabReordering else { return false } guard let (pane, sourceIndex) = findTabInternal(tabId) else { return false } let destinationIndex = max(0, min(toIndex, pane.tabs.count)) + let orderBeforeReorder = pane.tabs.map(\.id) pane.moveTab(from: sourceIndex, to: destinationIndex) pane.selectTab(tabId.id) internalController.focusPane(pane.id) + let orderAfterReorder = pane.tabs.map(\.id) + if orderAfterReorder != orderBeforeReorder { + delegate?.splitTabBar( + self, + didReorderTabsInPane: pane.id, + orderedTabIds: orderAfterReorder.map { TabID(id: $0) } + ) + } if let tabIndex = pane.tabs.firstIndex(where: { $0.id == tabId.id }) { let tab = Tab(from: pane.tabs[tabIndex]) delegate?.splitTabBar(self, didSelectTab: tab, inPane: pane.id) diff --git a/Tests/BonsplitTests/BonsplitTests.swift b/Tests/BonsplitTests/BonsplitTests.swift index fb1e9d12..c8453dfa 100644 --- a/Tests/BonsplitTests/BonsplitTests.swift +++ b/Tests/BonsplitTests/BonsplitTests.swift @@ -2378,7 +2378,7 @@ final class BonsplitTests: XCTestCase { } @MainActor - func testTrailingTabBarChromeDropDestinationStaysOffTabPixels() throws { + func testTabBarDropDestinationSpansTabsAndTrailingChrome() throws { let appearance = BonsplitConfiguration.Appearance(splitButtons: []) let controller = BonsplitController( configuration: BonsplitConfiguration(appearance: appearance) @@ -2411,12 +2411,6 @@ final class BonsplitTests: XCTestCase { contentView.addSubview(hostingView) window.makeKeyAndOrderFront(nil) - func setDragHitTesting(_ view: NSView) { - (view as? TabBarDragZoneView.DragNSView)?.hitTestEventTypeOverride = .leftMouseDragged - for subview in view.subviews { - setDragHitTesting(subview) - } - } func tabDropDestinations(in view: NSView) -> [NSView] { var matches: [NSView] = [] if view.registeredDraggedTypes.contains(where: { pasteboardType in @@ -2430,27 +2424,17 @@ final class BonsplitTests: XCTestCase { } return matches } - func dragZones(in view: NSView) -> [TabBarDragZoneView.DragNSView] { - var matches = (view as? TabBarDragZoneView.DragNSView).map { [$0] } ?? [] - for subview in view.subviews { - matches.append(contentsOf: dragZones(in: subview)) + func firstTabHitRegion(in view: NSView) -> TabItemHitRegionView.RegionNSView? { + if let region = view as? TabItemHitRegionView.RegionNSView { + return region } - return matches - } - func framesMatch(_ lhs: NSView, _ rhs: NSView) -> Bool { - let lhsFrame = lhs.convert(lhs.bounds, to: nil) - let rhsFrame = rhs.convert(rhs.bounds, to: nil) - return abs(lhsFrame.minX - rhsFrame.minX) <= 0.5 - && abs(lhsFrame.maxX - rhsFrame.maxX) <= 0.5 - && abs(lhsFrame.minY - rhsFrame.minY) <= 0.5 - && abs(lhsFrame.maxY - rhsFrame.maxY) <= 0.5 + return view.subviews.lazy.compactMap { firstTabHitRegion(in: $0) }.first } - let tabPoint = NSPoint(x: 90, y: 30) let trailingEmptyPoint = NSPoint(x: 460, y: 30) let registrationDeadline = Date().addingTimeInterval(0.5) var dropDestinations: [NSView] = [] - var dragZoneViews: [TabBarDragZoneView.DragNSView] = [] + var tabPoint: NSPoint? func dropDestination(at point: NSPoint) -> NSView? { let pointInWindow = hostingView.convert(point, to: nil) @@ -2458,20 +2442,22 @@ final class BonsplitTests: XCTestCase { view.convert(view.bounds, to: nil).contains(pointInWindow) } } - func chromeDragZones(at point: NSPoint) -> [TabBarDragZoneView.DragNSView] { - let pointInWindow = hostingView.convert(point, to: nil) - return dragZoneViews.filter { dragZone in - dragZone.convert(dragZone.bounds, to: nil).contains(pointInWindow) - } - } - repeat { contentView.layoutSubtreeIfNeeded() - setDragHitTesting(hostingView) dropDestinations = tabDropDestinations(in: hostingView) - dragZoneViews = dragZones(in: hostingView) - let tabPointInWindow = hostingView.convert(tabPoint, to: nil) - if BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), + if let tabRegion = firstTabHitRegion(in: hostingView), !tabRegion.bounds.isEmpty { + let centerInWindow = tabRegion.convert( + NSPoint(x: tabRegion.bounds.midX, y: tabRegion.bounds.midY), + to: nil + ) + tabPoint = hostingView.convert(centerInWindow, from: nil) + } + if let tabPoint, + BonsplitTabItemHitRegionRegistry.containsWindowPoint( + hostingView.convert(tabPoint, to: nil), + in: window + ), + dropDestination(at: tabPoint) != nil, dropDestination(at: trailingEmptyPoint) != nil { break } @@ -2480,26 +2466,25 @@ final class BonsplitTests: XCTestCase { guard dropDestination(at: trailingEmptyPoint) != nil else { throw XCTSkip( - "This SwiftUI runtime does not expose view-local onDrop registration through registeredDraggedTypes" + "This SwiftUI runtime did not mount the native tab-bar drop destination" ) } - let tabPointInWindow = hostingView.convert(tabPoint, to: nil) + let resolvedTabPoint = try XCTUnwrap(tabPoint, "Expected a mounted pane tab hit region") + let tabPointInWindow = hostingView.convert(resolvedTabPoint, to: nil) XCTAssertTrue( BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), "The test point should be owned by the rendered pane tab" ) - for dragZone in chromeDragZones(at: tabPoint) { - XCTAssertFalse( - dropDestinations.contains(where: { framesMatch(dragZone, $0) }), - "Empty tab-bar chrome must not register an end-drop destination over a rendered tab" - ) - } + XCTAssertNotNil( + dropDestination(at: resolvedTabPoint), + "The strip-wide destination must resolve insertion slots over rendered tabs" + ) XCTAssertNotNil( dropDestination(at: trailingEmptyPoint), - "Actual empty trailing tab-bar space should still route tab transfers to the end-drop destination" + "The same destination must retain the final insertion slot in trailing chrome" ) } diff --git a/Tests/BonsplitTests/TabBarDropHandlerTests.swift b/Tests/BonsplitTests/TabBarDropHandlerTests.swift new file mode 100644 index 00000000..b8e1a83a --- /dev/null +++ b/Tests/BonsplitTests/TabBarDropHandlerTests.swift @@ -0,0 +1,238 @@ +import AppKit +@testable import Bonsplit +import XCTest + +@MainActor +final class TabBarDropHandlerTests: XCTestCase { + func testEarlierTabDropUsesResolvedMiddleIndex() throws { + let harness = try makeHarness() + let initialIds = harness.pane.tabs.map(\.id) + let movedTabId = initialIds[0] + let pasteboard = try makePasteboard(for: harness.pane.tabs[0], in: harness) + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[0], from: harness.pane.id) + + XCTAssertTrue(makeHandler(harness).performDrop(from: pasteboard, at: 3)) + XCTAssertEqual(harness.pane.tabs.map(\.id), [initialIds[1], initialIds[2], movedTabId, initialIds[3]]) + } + + func testLaterTabDropUsesResolvedMiddleIndex() throws { + let harness = try makeHarness() + let initialIds = harness.pane.tabs.map(\.id) + let movedTabId = initialIds[3] + let pasteboard = try makePasteboard(for: harness.pane.tabs[3], in: harness) + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[3], from: harness.pane.id) + + XCTAssertTrue(makeHandler(harness).performDrop(from: pasteboard, at: 1)) + XCTAssertEqual(harness.pane.tabs.map(\.id), [initialIds[0], movedTabId, initialIds[1], initialIds[2]]) + } + + func testCrossPaneDropUsesResolvedMiddleIndexInDestinationSplit() throws { + let harness = try makeHarness() + let sourcePane = harness.pane + let movedTab = sourcePane.tabs[3] + let targetPaneId = try XCTUnwrap( + harness.controller.splitPane(sourcePane.id, orientation: .vertical) + ) + _ = harness.controller.createTab(title: "Target First") + _ = harness.controller.createTab(title: "Target Second") + _ = harness.controller.createTab(title: "Target Third") + let targetPane = try XCTUnwrap( + harness.controller.internalController.paneState(for: targetPaneId) + ) + let initialTargetIds = targetPane.tabs.map(\.id) + let pasteboard = try makePasteboard(for: movedTab, sourcePane: sourcePane) + _ = harness.controller.internalController.beginTabDrag(movedTab, from: sourcePane.id) + + XCTAssertTrue(makeHandler(controller: harness.controller, pane: targetPane).performDrop( + from: pasteboard, + at: 1 + )) + XCTAssertEqual( + targetPane.tabs.map(\.id), + [initialTargetIds[0], movedTab.id, initialTargetIds[1], initialTargetIds[2]] + ) + XCTAssertFalse(sourcePane.tabs.contains(where: { $0.id == movedTab.id })) + } + + func testSamePaneReorderPreservesControllerMoveContract() throws { + let harness = try makeHarness() + let delegate = ReorderDelegateSpy() + harness.controller.delegate = delegate + let initialIds = harness.pane.tabs.map(\.id) + let movedTabId = initialIds[3] + let pasteboard = try makePasteboard(for: harness.pane.tabs[3], in: harness) + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[3], from: harness.pane.id) + + XCTAssertTrue(makeHandler(harness).performDrop(from: pasteboard, at: 1)) + + let expectedOrder = [initialIds[0], movedTabId, initialIds[1], initialIds[2]] + XCTAssertEqual(harness.pane.tabs.map(\.id), expectedOrder) + XCTAssertEqual(harness.pane.selectedTabId, movedTabId) + XCTAssertEqual(harness.controller.focusedPaneId, harness.pane.id) + XCTAssertEqual(delegate.selectedTabIds, [TabID(id: movedTabId)]) + XCTAssertEqual(delegate.reorderedTabIds, expectedOrder.map { TabID(id: $0) }) + XCTAssertEqual(delegate.geometryChangeCount, 1) + } + + func testDropIndicatorSuppressesSamePaneNoopTargets() throws { + let harness = try makeHarness() + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[1], from: harness.pane.id) + let handler = makeHandler(harness) + + XCTAssertNil(handler.indicatorIndex(for: 1)) + XCTAssertNil(handler.indicatorIndex(for: 2)) + XCTAssertEqual(handler.indicatorIndex(for: 0), 0) + XCTAssertEqual(handler.indicatorIndex(for: 3), 3) + } + + func testLiveDragIsAcceptedBeforePasteboardTypePublication() throws { + let harness = try makeHarness() + let emptyPasteboard = makeEmptyPasteboard() + let initialIds = harness.pane.tabs.map(\.id) + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[3], from: harness.pane.id) + + XCTAssertEqual(makeHandler(harness).operation(for: emptyPasteboard), .move) + XCTAssertTrue(makeHandler(harness).performDrop(from: emptyPasteboard, at: 1)) + XCTAssertEqual( + harness.pane.tabs.map(\.id), + [initialIds[0], initialIds[3], initialIds[1], initialIds[2]] + ) + } + + func testFileOnlyDropIgnoresStaleLiveTabDrag() throws { + let harness = try makeHarness() + let initialIds = harness.pane.tabs.map(\.id) + let pasteboard = makeEmptyPasteboard() + let fileURL = URL(fileURLWithPath: "/tmp/bonsplit-tab-drop-handler-file") + XCTAssertTrue(pasteboard.writeObjects([fileURL as NSURL])) + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[3], from: harness.pane.id) + + var receivedURLs: [URL] = [] + harness.controller.onExternalFileDrop = { request in + receivedURLs = request.urls + return true + } + + XCTAssertEqual(makeHandler(harness).operation(for: pasteboard), .copy) + XCTAssertTrue(makeHandler(harness).performDrop(from: pasteboard, at: 1)) + XCTAssertEqual(receivedURLs, [fileURL]) + XCTAssertEqual(harness.pane.tabs.map(\.id), initialIds) + } + + func testInactiveControllerRejectsLiveDrag() throws { + let harness = try makeHarness() + let emptyPasteboard = makeEmptyPasteboard() + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[1], from: harness.pane.id) + harness.controller.isInteractive = false + + XCTAssertEqual(makeHandler(harness).operation(for: emptyPasteboard), []) + } + + func testDestinationCapturesOnlyCompatibleDragEvents() { + XCTAssertTrue(TabBarDropDestinationNSView.shouldCaptureHitTest( + eventType: .leftMouseDragged, + pasteboardTypes: nil, + hasLocalTabDrag: true + )) + XCTAssertFalse(TabBarDropDestinationNSView.shouldCaptureHitTest( + eventType: .leftMouseDown, + pasteboardTypes: [TabBarDropHandler.tabTransferPasteboardType], + hasLocalTabDrag: true + )) + XCTAssertFalse(TabBarDropDestinationNSView.shouldCaptureHitTest( + eventType: .leftMouseDragged, + pasteboardTypes: nil + )) + } + + func testTranslatedDestinationHitTestsInSuperviewCoordinates() throws { + let harness = try makeHarness() + _ = harness.controller.internalController.beginTabDrag(harness.pane.tabs[1], from: harness.pane.id) + + let container = NSView(frame: NSRect(x: 0, y: 0, width: 500, height: 100)) + let destination = TabBarDropDestinationNSView( + frame: NSRect(x: 240, y: 20, width: 200, height: 34) + ) + destination.pane = harness.pane + destination.bonsplitController = harness.controller + destination.splitViewController = harness.controller.internalController + container.addSubview(destination) + + XCTAssertTrue(container.hitTest(NSPoint(x: 260, y: 30)) === destination) + XCTAssertFalse(container.hitTest(NSPoint(x: 20, y: 30)) === destination) + } + + private func makeHarness() throws -> Harness { + let controller = BonsplitController( + configuration: BonsplitConfiguration( + allowTabReordering: true, + allowCrossPaneTabMove: true, + newTabPosition: .end + ) + ) + let paneId = try XCTUnwrap(controller.focusedPaneId) + _ = controller.createTab(title: "Second") + _ = controller.createTab(title: "Third") + _ = controller.createTab(title: "Fourth") + let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId)) + XCTAssertEqual(pane.tabs.count, 4) + return Harness(controller: controller, pane: pane) + } + + private func makeHandler(_ harness: Harness) -> TabBarDropHandler { + makeHandler(controller: harness.controller, pane: harness.pane) + } + + private func makeHandler(controller: BonsplitController, pane: PaneState) -> TabBarDropHandler { + TabBarDropHandler( + pane: pane, + bonsplitController: controller, + splitViewController: controller.internalController + ) + } + + private func makeEmptyPasteboard() -> NSPasteboard { + let pasteboard = NSPasteboard( + name: NSPasteboard.Name("TabBarDropHandlerTests.\(UUID().uuidString)") + ) + _ = pasteboard.clearContents() + return pasteboard + } + + private func makePasteboard(for tab: TabItem, in harness: Harness) throws -> NSPasteboard { + try makePasteboard(for: tab, sourcePane: harness.pane) + } + + private func makePasteboard(for tab: TabItem, sourcePane: PaneState) throws -> NSPasteboard { + let pasteboard = makeEmptyPasteboard() + let transfer = TabTransferData(tab: tab, sourcePaneId: sourcePane.id.id) + let data = try JSONEncoder().encode(transfer) + XCTAssertTrue(pasteboard.setData(data, forType: TabBarDropHandler.tabTransferPasteboardType)) + return pasteboard + } +} + +@MainActor +private struct Harness { + let controller: BonsplitController + let pane: PaneState +} + +@MainActor +private final class ReorderDelegateSpy: BonsplitDelegate { + var selectedTabIds: [TabID] = [] + var reorderedTabIds: [TabID] = [] + var geometryChangeCount = 0 + + func splitTabBar(_ controller: BonsplitController, didSelectTab tab: Tab, inPane pane: PaneID) { + selectedTabIds.append(tab.id) + } + + func splitTabBar(_ controller: BonsplitController, didReorderTabsInPane pane: PaneID, orderedTabIds: [TabID]) { + reorderedTabIds = orderedTabIds + } + + func splitTabBar(_ controller: BonsplitController, didChangeGeometry snapshot: LayoutSnapshot) { + geometryChangeCount += 1 + } +} diff --git a/Tests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift b/Tests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift deleted file mode 100644 index 52beefeb..00000000 --- a/Tests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift +++ /dev/null @@ -1,131 +0,0 @@ -@testable import Bonsplit -import XCTest - -@MainActor -final class TabDropDelegateSamePaneFallbackTests: XCTestCase { - func testEarlierTabSwiftUIDropUsesMiddleTargetFromDelegate() throws { - let harness = try makeHarness() - let initialIds = harness.pane.tabs.map(\.id) - let movedTabId = initialIds[0] - - let targetIndex = try XCTUnwrap(swiftUISamePaneDropTarget( - tabId: movedTabId, - targetIndex: 3, - harness: harness - )) - - XCTAssertEqual(targetIndex, 3) - XCTAssertTrue(TabDropDelegate.performSamePaneReorder( - tabId: movedTabId, - targetIndex: targetIndex, - pane: harness.pane, - bonsplitController: harness.controller - )) - XCTAssertEqual(harness.pane.tabs.map(\.id), [initialIds[1], initialIds[2], movedTabId, initialIds[3]]) - } - - func testLaterTabSwiftUIDropUsesMiddleTargetFromDelegate() throws { - let harness = try makeHarness() - let initialIds = harness.pane.tabs.map(\.id) - let movedTabId = initialIds[3] - - let targetIndex = try XCTUnwrap(swiftUISamePaneDropTarget( - tabId: movedTabId, - targetIndex: 1, - harness: harness - )) - - XCTAssertEqual(targetIndex, 1) - XCTAssertTrue(TabDropDelegate.performSamePaneReorder( - tabId: movedTabId, - targetIndex: targetIndex, - pane: harness.pane, - bonsplitController: harness.controller - )) - XCTAssertEqual(harness.pane.tabs.map(\.id), [initialIds[0], movedTabId, initialIds[1], initialIds[2]]) - } - - func testSamePaneReorderPreservesControllerMoveContract() throws { - let harness = try makeHarness() - let delegate = ReorderDelegateSpy() - harness.controller.delegate = delegate - let initialIds = harness.pane.tabs.map(\.id) - let movedTabId = initialIds[3] - - XCTAssertTrue(TabDropDelegate.performSamePaneReorder( - tabId: movedTabId, - targetIndex: 1, - pane: harness.pane, - bonsplitController: harness.controller - )) - - let expectedOrder = [initialIds[0], movedTabId, initialIds[1], initialIds[2]] - XCTAssertEqual(harness.pane.tabs.map(\.id), expectedOrder) - XCTAssertEqual(harness.pane.selectedTabId, movedTabId) - XCTAssertEqual(harness.controller.focusedPaneId, harness.pane.id) - XCTAssertEqual(delegate.selectedTabIds, [TabID(id: movedTabId)]) - XCTAssertEqual(delegate.reorderedTabIds, expectedOrder.map { TabID(id: $0) }) - XCTAssertEqual(delegate.geometryChangeCount, 1) - } - - func testSwiftUIDropSuppressesSamePaneNoopTargets() throws { - let harness = try makeHarness() - let movedTabId = harness.pane.tabs[1].id - - XCTAssertNil(swiftUISamePaneDropTarget(tabId: movedTabId, targetIndex: 1, harness: harness)) - XCTAssertNil(swiftUISamePaneDropTarget(tabId: movedTabId, targetIndex: 2, harness: harness)) - XCTAssertEqual(swiftUISamePaneDropTarget(tabId: movedTabId, targetIndex: 0, harness: harness), 0) - XCTAssertEqual(swiftUISamePaneDropTarget(tabId: movedTabId, targetIndex: 3, harness: harness), 3) - } - - private func makeHarness() throws -> Harness { - let controller = BonsplitController( - configuration: BonsplitConfiguration( - allowTabReordering: true, - allowCrossPaneTabMove: true, - newTabPosition: .end - ) - ) - let paneId = try XCTUnwrap(controller.focusedPaneId) - _ = controller.createTab(title: "Second") - _ = controller.createTab(title: "Third") - _ = controller.createTab(title: "Fourth") - let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId)) - XCTAssertEqual(pane.tabs.count, 4) - return Harness(controller: controller, pane: pane) - } - - private func swiftUISamePaneDropTarget( - tabId: UUID, - targetIndex: Int, - harness: Harness - ) -> Int? { - let sourceIndex = harness.pane.tabs.firstIndex { $0.id == tabId }! - return TabDropDelegate.samePaneDropTarget(sourceIndex: sourceIndex, targetIndex: targetIndex) - } -} - -@MainActor -private struct Harness { - let controller: BonsplitController - let pane: PaneState -} - -@MainActor -private final class ReorderDelegateSpy: BonsplitDelegate { - var selectedTabIds: [TabID] = [] - var reorderedTabIds: [TabID] = [] - var geometryChangeCount = 0 - - func splitTabBar(_ controller: BonsplitController, didSelectTab tab: Tab, inPane pane: PaneID) { - selectedTabIds.append(tab.id) - } - - func splitTabBar(_ controller: BonsplitController, didReorderTabsInPane pane: PaneID, orderedTabIds: [TabID]) { - reorderedTabIds = orderedTabIds - } - - func splitTabBar(_ controller: BonsplitController, didChangeGeometry snapshot: LayoutSnapshot) { - geometryChangeCount += 1 - } -} diff --git a/Tests/BonsplitTests/TabDropIndexResolverTests.swift b/Tests/BonsplitTests/TabDropIndexResolverTests.swift index 5d2c9198..b29a529e 100644 --- a/Tests/BonsplitTests/TabDropIndexResolverTests.swift +++ b/Tests/BonsplitTests/TabDropIndexResolverTests.swift @@ -74,6 +74,31 @@ struct TabDropIndexResolverTests { ) } + @Test("Registered frames preserve their model insertion indices") + func indexedFramesPreserveModelIndices() { + let indexedFrames = [ + TabDropIndexResolver.IndexedFrame(index: 1, frame: tabFrames[1]), + TabDropIndexResolver.IndexedFrame(index: 2, frame: tabFrames[2]), + ] + + #expect( + TabDropIndexResolver.insertionIndex( + at: CGPoint(x: 120, y: bounds.midY), + in: bounds, + indexedTabFrames: indexedFrames, + tabCount: tabFrames.count + ) == 1 + ) + #expect( + TabDropIndexResolver.insertionIndex( + at: CGPoint(x: 280, y: bounds.midY), + in: bounds, + indexedTabFrames: indexedFrames, + tabCount: tabFrames.count + ) == 3 + ) + } + private func insertionIndex(x: CGFloat) -> Int? { TabDropIndexResolver.insertionIndex( at: CGPoint(x: x, y: bounds.midY), From 4da56abb6d64595970af2081726591889ca54982 Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Thu, 6 Aug 2026 21:47:01 -0700 Subject: [PATCH 3/3] fix: harden tab drop fallback resolution --- .../Views/TabBarDropDestinationNSView.swift | 4 ++- .../Internal/Views/TabBarDropHandler.swift | 8 ++++- .../Internal/Views/TabDropIndexResolver.swift | 22 +++++++++---- .../TabBarDropHandlerTests.swift | 25 +++++++++++++++ .../TabDropIndexResolverTests.swift | 32 +++++++++++++++---- 5 files changed, 75 insertions(+), 16 deletions(-) diff --git a/Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift b/Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift index d3150a01..57f596c8 100644 --- a/Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift +++ b/Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift @@ -9,9 +9,11 @@ final class TabBarDropDestinationNSView: NSView { weak var geometryRegistry: TabBarItemGeometryRegistry? var onIndicatorIndexChanged: ((Int?) -> Void)? + private let dropIndexResolver: TabDropIndexResolver private var presentedIndicatorIndex: Int? override init(frame frameRect: NSRect) { + dropIndexResolver = TabDropIndexResolver() super.init(frame: frameRect) registerForDraggedTypes([ TabBarDropHandler.tabTransferPasteboardType, @@ -119,7 +121,7 @@ final class TabBarDropDestinationNSView: NSView { guard let frame = geometryRegistry.frame(for: tab.id, in: self) else { return nil } return TabDropIndexResolver.IndexedFrame(index: index, frame: frame) } - return TabDropIndexResolver.insertionIndex( + return dropIndexResolver.insertionIndex( at: convert(sender.draggingLocation, from: nil), in: bounds, indexedTabFrames: indexedFrames, diff --git a/Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift b/Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift index 2ec7e41e..479d64ed 100644 --- a/Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift +++ b/Sources/Bonsplit/Internal/Views/TabBarDropHandler.swift @@ -42,7 +42,13 @@ struct TabBarDropHandler { guard let transfer = Self.decodeTransfer(from: pasteboard), transfer.isFromCurrentProcess else { return [] } - return permitsTabMove(from: PaneID(id: transfer.sourcePaneId)) ? .move : [] + let sourcePaneId = PaneID(id: transfer.sourcePaneId) + guard sourcePaneId != pane.id, + bonsplitController.configuration.allowCrossPaneTabMove, + bonsplitController.onExternalTabDrop != nil else { + return [] + } + return .move } return [] diff --git a/Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift b/Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift index 0a9207df..29670254 100644 --- a/Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift +++ b/Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift @@ -2,12 +2,14 @@ import CoreGraphics /// Resolves a pointer in a horizontal tab strip to an insertion index. struct TabDropIndexResolver { + /// A live tab frame paired with its index in the pane model. struct IndexedFrame { let index: Int let frame: CGRect } - static func insertionIndex( + /// Resolves frames whose offsets already match their pane-model indices. + func insertionIndex( at location: CGPoint, in bounds: CGRect, tabFrames: [CGRect] @@ -20,7 +22,8 @@ struct TabDropIndexResolver { ) } - static func insertionIndex( + /// Resolves leading-to-trailing live frames while preserving their pane-model indices. + func insertionIndex( at location: CGPoint, in bounds: CGRect, indexedTabFrames: [IndexedFrame], @@ -33,23 +36,28 @@ struct TabDropIndexResolver { (0.. 0 && item.frame.midX.isFinite - }), - Set(indexedTabFrames.map(\.index)).count == indexedTabFrames.count else { + }) else { return nil } - let visuallyOrderedFrames = indexedTabFrames.sorted { lhs, rhs in + let framesAreModelAndVisuallyOrdered = zip( + indexedTabFrames, + indexedTabFrames.dropFirst() + ).allSatisfy { pair in + let (lhs, rhs) = pair + guard lhs.index < rhs.index else { return false } if lhs.frame.midX == rhs.frame.midX { return lhs.index < rhs.index } return lhs.frame.midX < rhs.frame.midX } + guard framesAreModelAndVisuallyOrdered else { return nil } - for item in visuallyOrderedFrames where location.x < item.frame.midX { + for item in indexedTabFrames where location.x < item.frame.midX { return item.index } - guard let lastItem = visuallyOrderedFrames.last else { return nil } + guard let lastItem = indexedTabFrames.last else { return nil } return min(lastItem.index + 1, tabCount) } } diff --git a/Tests/BonsplitTests/TabBarDropHandlerTests.swift b/Tests/BonsplitTests/TabBarDropHandlerTests.swift index b8e1a83a..82c4809b 100644 --- a/Tests/BonsplitTests/TabBarDropHandlerTests.swift +++ b/Tests/BonsplitTests/TabBarDropHandlerTests.swift @@ -128,6 +128,31 @@ final class TabBarDropHandlerTests: XCTestCase { XCTAssertEqual(makeHandler(harness).operation(for: emptyPasteboard), []) } + func testPasteboardOnlySamePaneTransferIsRejected() throws { + let harness = try makeHarness() + let pasteboard = try makePasteboard(for: harness.pane.tabs[1], in: harness) + let handler = makeHandler(harness) + + XCTAssertEqual(handler.operation(for: pasteboard), []) + XCTAssertFalse(handler.performDrop(from: pasteboard, at: 3)) + } + + func testPasteboardOnlyCrossPaneTransferRequiresExternalHandler() throws { + let harness = try makeHarness() + let sourcePane = harness.pane + let targetPaneId = try XCTUnwrap( + harness.controller.splitPane(sourcePane.id, orientation: .vertical) + ) + let targetPane = try XCTUnwrap( + harness.controller.internalController.paneState(for: targetPaneId) + ) + let pasteboard = try makePasteboard(for: sourcePane.tabs[1], sourcePane: sourcePane) + let handler = makeHandler(controller: harness.controller, pane: targetPane) + + XCTAssertEqual(handler.operation(for: pasteboard), []) + XCTAssertFalse(handler.performDrop(from: pasteboard, at: 0)) + } + func testDestinationCapturesOnlyCompatibleDragEvents() { XCTAssertTrue(TabBarDropDestinationNSView.shouldCaptureHitTest( eventType: .leftMouseDragged, diff --git a/Tests/BonsplitTests/TabDropIndexResolverTests.swift b/Tests/BonsplitTests/TabDropIndexResolverTests.swift index b29a529e..6c1a1a9d 100644 --- a/Tests/BonsplitTests/TabDropIndexResolverTests.swift +++ b/Tests/BonsplitTests/TabDropIndexResolverTests.swift @@ -4,6 +4,7 @@ import Testing @Suite("Horizontal tab strip drop index") struct TabDropIndexResolverTests { + private let resolver = TabDropIndexResolver() private let bounds = CGRect(x: 0, y: 0, width: 360, height: 34) private let tabFrames = [ CGRect(x: 12, y: 0, width: 80, height: 34), @@ -35,14 +36,14 @@ struct TabDropIndexResolverTests { let translatedFrames = tabFrames.map { $0.offsetBy(dx: 500, dy: 120) } #expect( - TabDropIndexResolver.insertionIndex( + resolver.insertionIndex( at: CGPoint(x: 596, y: 137), in: translatedBounds, tabFrames: translatedFrames ) == 1 ) #expect( - TabDropIndexResolver.insertionIndex( + resolver.insertionIndex( at: CGPoint(x: 724, y: 137), in: translatedBounds, tabFrames: translatedFrames @@ -55,7 +56,7 @@ struct TabDropIndexResolverTests { #expect(insertionIndex(x: -1) == nil) #expect(insertionIndex(x: 361) == nil) #expect( - TabDropIndexResolver.insertionIndex( + resolver.insertionIndex( at: CGPoint(x: 96, y: 35), in: bounds, tabFrames: tabFrames @@ -66,7 +67,7 @@ struct TabDropIndexResolverTests { @Test("An empty strip accepts its only insertion slot") func emptyStripUsesZeroIndex() { #expect( - TabDropIndexResolver.insertionIndex( + resolver.insertionIndex( at: CGPoint(x: 180, y: 17), in: bounds, tabFrames: [] @@ -82,7 +83,7 @@ struct TabDropIndexResolverTests { ] #expect( - TabDropIndexResolver.insertionIndex( + resolver.insertionIndex( at: CGPoint(x: 120, y: bounds.midY), in: bounds, indexedTabFrames: indexedFrames, @@ -90,7 +91,7 @@ struct TabDropIndexResolverTests { ) == 1 ) #expect( - TabDropIndexResolver.insertionIndex( + resolver.insertionIndex( at: CGPoint(x: 280, y: bounds.midY), in: bounds, indexedTabFrames: indexedFrames, @@ -99,8 +100,25 @@ struct TabDropIndexResolverTests { ) } + @Test("Registered frames must stay in model and visual order") + func rejectsUnorderedIndexedFrames() { + let unorderedFrames = [ + TabDropIndexResolver.IndexedFrame(index: 1, frame: tabFrames[1]), + TabDropIndexResolver.IndexedFrame(index: 0, frame: tabFrames[0]), + ] + + #expect( + resolver.insertionIndex( + at: CGPoint(x: 120, y: bounds.midY), + in: bounds, + indexedTabFrames: unorderedFrames, + tabCount: tabFrames.count + ) == nil + ) + } + private func insertionIndex(x: CGFloat) -> Int? { - TabDropIndexResolver.insertionIndex( + resolver.insertionIndex( at: CGPoint(x: x, y: bounds.midY), in: bounds, tabFrames: tabFrames