Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,6 @@ nonisolated extension SidebarLayout {
return shown.first(where: { $0.id == member.id }).map { (i, $0) }
}
for (n, (childIndex, ws)) in kept.enumerated() { workspaceRow(ws, at: childIndex, last: n == kept.count - 1) }
} else if members.isEmpty {
// Subgroups and filters already sent the full layout above; the empty row is `make`'s.
if Self.listsEmptyRow(group, filtering: false, hasSubgroups: false) {
y = Self.appendEmptyRow(group, section: section.id, parent: parent, metrics: m, to: &built, y: y)
}
} else {
for (childIndex, member) in members.enumerated() {
switch member {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,12 +392,6 @@ public nonisolated struct SidebarLayout: Hashable, Sendable {
}
return
}
// An open group with nothing in it keeps one quiet member-sized row (user 9757): it reads as open
// and empty, and a drop over it goes into the group. A drag's gap here takes its place.
if members.isEmpty, o.gap?.group != group.id,
Self.listsEmptyRow(group, filtering: filtering, hasSubgroups: subgroupParents.contains(group.id)) {
y = Self.appendEmptyRow(group, section: section.id, parent: index, metrics: m, to: &rows, y: y)
}
let fold = SidebarDoneFoldID(section: section.id, group: group.id)
let drawn = Self.foldingDone(Self.treeOrder(Self.workspaceIDs(members), parents: o.sessionTree?.parents),
workspace: { if case let .workspace(ws) = members[$0] { ws } else { nil } }, fold: fold, options: o)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ extension SidebarListViewComponents {
case let (.workspace(id), rowView as WorkspaceRowView):
rowView.isDropTarget = drag?.target == .ontoWorkspace(id) || standsForDropGroup(row)
case let (.group(id), header as GroupHeaderRowView):
header.isDropTarget = drag?.target == .intoGroup(id)
header.isDropTarget = highlightsDropGroup(id)
default:
break
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ extension SidebarListViewComponents {
view.configure(group, row: row, details: details, status: subtree.statusRollUp(collapsed: row.isCollapsed),
animated: animated || model.followsUserIntent)
view.railReach = railReach(for: row)
view.isDropTarget = drag?.target == .intoGroup(id) || external?.proposal == .intoGroup(id)
view.isDropTarget = highlightsDropGroup(id)
case let (.section(id), view as SectionHeaderRowView):
guard let section = sections[id] else { return }
view.configure(section, row: row)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,15 @@ extension SidebarListViewComponents {
return true
}

/// An open empty group's insertion position is its header's drop target, too. Keep this
/// visual state tied to the live drag; at rest the header has no empty-state decoration.
func highlightsDropGroup(_ id: GroupID) -> Bool {
if drag?.target == .intoGroup(id) || external?.proposal == .intoGroup(id) { return true }
guard case let .position(position)? = drag?.target, position.group == id,
groups[id]?.members.isEmpty == true else { return false }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The check uses the cached model members, so it misses groups made empty by the current drag's excluded workspace. Use the displayed header's filtered childCount so a one-member group still highlights when its member is being dragged out.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextSidebar/Views/SidebarListView+PointerEnd.swift, line 127:

<comment>The check uses the cached model members, so it misses groups made empty by the current drag's excluded workspace. Use the displayed header's filtered `childCount` so a one-member group still highlights when its member is being dragged out.</comment>

<file context>
@@ -119,6 +119,15 @@ extension SidebarListViewComponents {
+    func highlightsDropGroup(_ id: GroupID) -> Bool {
+        if drag?.target == .intoGroup(id) || external?.proposal == .intoGroup(id) { return true }
+        guard case let .position(position)? = drag?.target, position.group == id,
+              groups[id]?.members.isEmpty == true else { return false }
+        return true
+    }
</file context>
Suggested change
groups[id]?.members.isEmpty == true else { return false }
displayed.row(for: .group(id))?.childCount == 0 else { return false }

return true
}

/// Sets each realized row's selection, multi-selection and drop-target fill from the model and the live drag, as
/// `configure` would, without configuring the rows (no content, no layout: a click stays cheap, u2455). A row whose
/// fill a gesture left behind clears here.
Expand All @@ -131,7 +140,7 @@ extension SidebarListViewComponents {
row.isDropTarget = external?.proposal == .intoWorkspace(id) || drag?.target == .ontoWorkspace(id)
|| displayed.row(for: key).map(standsForDropGroup) == true
case let (.group(id), header as GroupHeaderRowView):
header.isDropTarget = drag?.target == .intoGroup(id) || external?.proposal == .intoGroup(id)
header.isDropTarget = highlightsDropGroup(id)
default:
break
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ import Testing
list.press = SidebarPress(key: dragged, point: start)
list.beginDrag(try #require(list.press))
try #require(list.drag != nil, "the member did not lift")
// Past the last row as the drag draws the list: the emptied group shows a placeholder row
// while its member is lifted (0105692a41d), which pushes the rows below it down a row.
// Past the last row as the drag draws the list; the emptied group keeps only its header.
@MainActor func bottom() -> CGFloat { (list.displayed.rows.last?.maxY ?? 300) + 10 }
var y = start.y, gone: [Int] = [], frame = 0
while y < bottom(), frame < 200 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,12 @@ import CmuxNextDesign
import CmuxNextTestSupport
import Testing

/// User msg 17912 ("empty workspace needs plus button", a shot of an open, empty "Sleep alarm" group): the
/// empty group's row read only "No workspaces · drag one here", and its header hid the + an empty group needs
/// most. The row shows a + at rest, and the header offers its + on hover as a full group's does; both run
/// New Workspace in Group, the one action path the group editor, menu, palette and CLI share.
/// Empty groups show only their header; its hover + keeps New Workspace in Group reachable.
@MainActor @Suite(.serialized, .closesItsWindows) struct SidebarEmptyGroupAddButtonTests {
private static let group = GroupID("grp-empty")
static let group = GroupID("grp-empty")

/// A sidebar in a window with one open, empty group; `ran` collects the group actions it runs.
private static func sidebar() -> (SidebarView, NSWindow, Ran) {
static func sidebar() -> (SidebarView, NSWindow, Ran) {
let machine = SidebarMachine(id: .local, name: "This Mac", kind: .local)
let empty = SidebarGroup(id: group, name: "Sleep alarm", color: .purple, workspaces: [])
let loose = SidebarWorkspace(id: WorkspaceID("loose"), title: "loose")
Expand All @@ -39,21 +36,40 @@ import Testing
!view.isHidden && view.alphaValue > 0 && view.frame.width > 0 && row.bounds.contains(view.frame)
}

@Test func theEmptyRowShowsAPlusAtRestThatMakesAWorkspaceInTheGroup() throws {
let (sidebar, window, ran) = Self.sidebar()
@Test func theEmptyGroupShowsOnlyItsHeaderAtRest() throws {
let (sidebar, window, _) = Self.sidebar()
defer { window.close() }
let header = try #require(sidebar.list.rowViews[.group(Self.group)] as? GroupHeaderRowView)
header.isHovered = false
header.layoutSubtreeIfNeeded()
#expect(sidebar.list.rowViews[.emptyGroup(Self.group)] == nil)
#expect(header.addButton.isHidden)
#expect(!header.isDropTarget)
}

@Test func theEmptyHeaderHighlightsOnlyDuringADropIntoIt() throws {
let (sidebar, window, _) = Self.sidebar()
defer { window.close() }
let row = try #require(sidebar.list.rowViews[.emptyGroup(Self.group)], "the open empty group shows its row")
sidebar.list.wire(row, key: .emptyGroup(Self.group))
row.isHovered = false
row.layoutSubtreeIfNeeded()
let plus = try #require(Self.views(in: row).first { $0.accessibilityIdentifier() == "cmux.sidebar.group.grp-empty.emptyAdd" } as? NSButton,
"the empty group's row has no + button")
#expect(Self.drawn(plus, in: row), "the + does not show at rest (user 17912)")
#expect(plus.accessibilityLabel() == Strings.newWorkspace)
let center = row.convert(NSPoint(x: plus.frame.midX, y: plus.frame.midY), to: row.superview)
#expect(row.hitTest(center) === plus, "the + takes no click")
plus.performClick(nil)
#expect(ran.items == ["grp-empty workspaceGroup.newWorkspace"], "the + runs New Workspace in Group")
let list = sidebar.list
let header = try #require(list.rowViews[.group(Self.group)] as? GroupHeaderRowView)
let source = try #require(list.displayed.row(for: .workspace(WorkspaceID("loose"))))
let press = SidebarPress(key: source.key, point: NSPoint(x: 80, y: source.y + source.height / 2))
list.beginDrag(press)
let drag = try #require(list.drag)
defer {
list.cancelDrag()
if let landing = list.landing { list.finishLand(landing, then: nil) }
}
let row = try #require(list.displayed.row(for: .group(Self.group)))
let y = row.y + row.height / 2
drag.target = DropResolver.resolve(y: y, payload: .workspaces([WorkspaceID("loose")]),
base: list.displayed, sections: list.model.sections)
list.syncRowStates()
#expect(header.isDropTarget)
drag.target = nil
list.syncRowStates()
#expect(!header.isDropTarget)
#expect(list.rowViews[.emptyGroup(Self.group)] == nil)
}

@Test func theEmptyGroupsHeaderOffersItsPlusOnHover() throws {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import AppKit
@testable import CmuxNextDesign
@testable import CmuxNextSidebar
import CmuxNextTestSupport
import CmuxTheme
import Foundation
import Testing

/// Native design comparison, with synthetic data. The chosen header-only version is the live list;
/// the other two are gallery-only reconstructions, never product settings. Artifacts are 2x.
@MainActor @Suite(.serialized, .closesItsWindows) struct SidebarEmptyGroupGalleryTests {
enum Version: String, CaseIterable { case placeholder, headerOnly, alwaysAdd }
enum State: String, CaseIterable { case rest, hover, drop }

static func render(_ version: Version, state: State, dark: Bool) throws -> CGImage {
Motion.reduceMotionOverride = true
defer { Motion.reduceMotionOverride = nil }
let (sidebar, window, _) = SidebarEmptyGroupAddButtonTests.sidebar()
defer { window.close() }
let theme = dark ? "Soft Blue" : "Soft Blue Light"
window.appearance = NSAppearance(named: dark ? .darkAqua : .aqua)
let root = NSView(frame: sidebar.frame)
root.wantsLayer = true
root.appearance = window.appearance
window.contentView = root
root.addSubview(sidebar)
let input = try AANestedGroupsGalleryTests.input(theme)
_ = ThemeScope.app.store.apply(input)
let scope = ThemeScope(level: .room)
scope.setOverride(ThemeSpec(theme), input: input, animated: false)
scope.root(root)
root.layer?.backgroundColor = scope.perform { Palette.windowBackground.withAlphaComponent(1) }.cgColor
window.orderFrontRegardless()
sidebar.renderNow()
let list = sidebar.list
let group = SidebarEmptyGroupAddButtonTests.group
let header = try #require(list.rowViews[.group(group)] as? GroupHeaderRowView)
header.isHovered = state == .hover
header.isDropTarget = state == .drop
for view in AANestedGroupsGalleryTests.all(root) {
view.layoutSubtreeIfNeeded()
view.needsDisplay = true
view.displayIfNeeded()
}
if version == .placeholder {
// Reconstruct the previous native member-sized row without restoring it to product layout.
let height = SidebarLayoutMetrics.standard.height(for: WorkspaceRowContent())
let gap = SidebarLayoutMetrics.standard.rowSpacing
for (key, row) in list.rowViews where key != .group(group) && row.frame.minY >= header.frame.maxY {
row.frame.origin.y += height + gap
}
let row = EmptyGroupRowView(key: .emptyGroup(group))
row.onAdd = {}
row.configure(groupID: group, groupName: "Sleep alarm")
row.frame = NSRect(x: header.frame.minX, y: header.frame.maxY + gap, width: header.frame.width, height: height)
list.addSubview(row)
row.layoutSubtreeIfNeeded()
row.displayIfNeeded()
} else if version == .alwaysAdd {
// Compare a permanent header + against the chosen reveal-on-hover control.
header.addButton.isHidden = false
header.addButton.alphaValue = 1
}
CATransaction.flush()
let rep = try #require(NSBitmapImageRep(bitmapDataPlanes: nil, pixelsWide: Int(root.bounds.width * 2),
pixelsHigh: Int(root.bounds.height * 2), bitsPerSample: 8,
samplesPerPixel: 4, hasAlpha: true, isPlanar: false,
colorSpaceName: .deviceRGB, bytesPerRow: 0, bitsPerPixel: 0))
rep.size = root.bounds.size
root.cacheDisplay(in: root.bounds, to: rep)
return try #require(rep.cgImage)
}

@Test(.enabled(if: ProcessInfo.processInfo.environment["CMUX_TEST_REQUIRE_GUI"] == "1"))
func renderThreeVersions() throws {
let root = AANestedGroupsGalleryTests.packageRoot
let out = root.appending(path: "sidebar-empty-group-gallery")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: This gallery writes sidebar-empty-group-gallery/ and sidebar-empty-group-gallery.tar into Packages/macOS/CmuxNext, but .gitignore has no entries for them. Add Packages/macOS/CmuxNext/sidebar-empty-group-gallery/ and Packages/macOS/CmuxNext/sidebar-empty-group-gallery.tar next to the other gallery entries.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/SidebarEmptyGroupGalleryTests.swift, line 77:

<comment>This gallery writes `sidebar-empty-group-gallery/` and `sidebar-empty-group-gallery.tar` into `Packages/macOS/CmuxNext`, but `.gitignore` has no entries for them. Add `Packages/macOS/CmuxNext/sidebar-empty-group-gallery/` and `Packages/macOS/CmuxNext/sidebar-empty-group-gallery.tar` next to the other gallery entries.</comment>

<file context>
@@ -0,0 +1,100 @@
+    @Test(.enabled(if: ProcessInfo.processInfo.environment["CMUX_TEST_REQUIRE_GUI"] == "1"))
+    func renderThreeVersions() throws {
+        let root = AANestedGroupsGalleryTests.packageRoot
+        let out = root.appending(path: "sidebar-empty-group-gallery")
+        try FileManager.default.createDirectory(at: out, withIntermediateDirectories: true)
+        var html = "<meta charset='utf-8'><title>Empty group designs</title><style>body{font:16px system-ui}section{display:flex;gap:16px}img{width:280px;height:500px}figure{margin:0}</style><h1>Empty group designs</h1><p>Chosen: headerOnly. No extra row; + on hover; group highlight during a drop.</p>"
</file context>

try FileManager.default.createDirectory(at: out, withIntermediateDirectories: true)
var html = "<meta charset='utf-8'><title>Empty group designs</title><style>body{font:16px system-ui}section{display:flex;gap:16px}img{width:280px;height:500px}figure{margin:0}</style><h1>Empty group designs</h1><p>Chosen: headerOnly. No extra row; + on hover; group highlight during a drop.</p>"
for dark in [false, true] {
for state in State.allCases {
html += "<h2>\(dark ? "Dark" : "Light") — \(state.rawValue)</h2><section>"
for version in Version.allCases {
let name = "\(version.rawValue)-\(state.rawValue)-\(dark ? "dark" : "light").png"
try AAGroupHeaderDensityGalleryTests.write(try Self.render(version, state: state, dark: dark), to: out.appending(path: name))
html += "<figure><figcaption>\(version.rawValue)</figcaption><img src='\(name)'></figure>"
}
html += "</section>"
}
}
try html.write(to: out.appending(path: "index.html"), atomically: true, encoding: .utf8)
let tar = Process()
tar.executableURL = URL(fileURLWithPath: "/usr/bin/tar")
tar.currentDirectoryURL = root
tar.arguments = ["cf", "sidebar-empty-group-gallery.tar", "sidebar-empty-group-gallery"]
try tar.run()
tar.waitUntilExit()
#expect(tar.terminationStatus == 0)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@ import CmuxNextDesign
import CoreGraphics
import Testing

/// An expanded group with no workspaces (user 9757, "if group has no workspaces, we need better empty
/// space"): under its header the list keeps one quiet row, as tall and as far in as a member row, so
/// the group reads as open and empty, and a workspace dragged over that row drops into the group.
/// A collapsed empty group stays a header alone.
/// An empty group keeps only its header. Creation stays on the header's hover +,
/// and dropping a workspace on that header still inserts it into the group.
@MainActor struct SidebarEmptyGroupTests {
private let scratch = GroupID("g_scratch"), perf = GroupID("g_perf")
private let machine = SidebarMachine(id: .local, name: "This Mac", kind: .local)
Expand Down Expand Up @@ -34,18 +32,13 @@ import Testing
return next.group == group ? next : nil
}

@Test func anOpenEmptyGroupKeepsOneMemberSizedRowUnderItsHeader() throws {
@Test func anOpenEmptyGroupKeepsOnlyItsHeader() throws {
let layout = layout(sections())
let header = try #require(layout.row(for: .group(scratch)))
let placeholder = try #require(rowUnderHeader(layout, scratch), "the empty group shows a row under its header")
let memberHeight = SidebarLayoutMetrics.standard.height(for: WorkspaceRowContent())
#expect(placeholder.height == memberHeight, "as tall as a one-line member row")
#expect(placeholder.depth == header.depth + 1, "one level in, as a member")
#expect(placeholder.y == header.maxY + SidebarLayoutMetrics.standard.rowSpacing)
#expect(placeholder.isLastInGroup)
// The next group starts one row gap after the empty row, not right under the header.
#expect(rowUnderHeader(layout, scratch) == nil)
#expect(layout.row(for: .emptyGroup(scratch)) == nil)
let next = try #require(layout.row(for: .group(perf)))
#expect(next.y == placeholder.maxY + SidebarLayoutMetrics.standard.rowSpacing)
#expect(next.y == header.maxY + SidebarLayoutMetrics.standard.rowSpacing)
}

@Test func aCollapsedEmptyGroupStaysAHeaderAlone() {
Expand Down Expand Up @@ -93,12 +86,12 @@ import Testing
#expect(unfolded.totalHeight == open.totalHeight)
}

/// A loose workspace dragged over the empty row drops into the empty group, at its start.
@Test func aDropOnTheEmptyRowGoesIntoTheGroup() throws {
/// A loose workspace dragged over the empty header drops into the group, at its start.
@Test func aDropOnTheEmptyHeaderGoesIntoTheGroup() throws {
let sections = sections()
let layout = layout(sections)
let placeholder = try #require(rowUnderHeader(layout, scratch))
let y = placeholder.y + placeholder.height / 2
let header = try #require(layout.row(for: .group(scratch)))
let y = header.y + header.height / 2
let resolved = try #require(DropResolver.resolveDrag(centreY: y, leadingY: y, gapY: nil, gapHeight: 0,
payload: .workspaces([WorkspaceID("loose")]), base: layout, sections: sections))
let section = try #require(sections.first?.id)
Expand Down
Loading
Loading