Skip to content

popover: Tell the trigger it is open instead of selected - #3125

Merged
huacnlee merged 1 commit into
mainfrom
popover-open-state
Sep 18, 2026
Merged

huacnlee merged 1 commit into
mainfrom
popover-open-state

Conversation

@huacnlee

Copy link
Copy Markdown
Member

Problem

A popover told its trigger it was open by borrowing the selected state:

trigger.selected(trigger.is_selected() || is_open)

"Open" (a popup is showing, the trigger stays lit) and "selected" (this is the
current view, the chosen item) are two different states. Downstream
(ai-desktop/pilot) a sidebar row implements Selectable::selected to mean "this
is the current view"; used as the account popover's trigger it had to be wrapped
in a SidebarRowTrigger adapter that re-mapped selected back into open state,
because the popover would otherwise light the row as if it were the current view.

Change

Selectable gains two default methods, so nothing breaks:

fn open(self, open: bool) -> Self { self.selected(open) }
fn is_open(&self) -> bool { self.is_selected() }

Naming follows the boolean setter/reader rule in CLAUDE.md: the setter takes the
field name, the reader is is_.

The crates/base change is limited to this — Selectable lives in
crates/base/src/component_traits.rs, so the seam has to be widened there; no
base component's appearance changes.

Everything that mapped "popup is open" onto the trigger now calls open:

  • gpui_base::Popover::trigger
  • gpui_component::popover::Popover::trigger
  • AppMenu in the app menu bar (its private is_selected helper is renamed
    is_open, which is what it always meant — the menu bar's open menu)

DropdownMenu/DropdownMenuPopover reach the trigger through
Popover::trigger, so they follow along; DropdownButton's selected is a
caller-set selection, not open state, and is left alone. Select, Combobox
and the sidebar submenu keep their own open flags and never went through
Selectable.

Button implements open explicitly and stores it in its own open field.
Open and selected look identical today, so both render through one
shows_selected_style() check — no visual change — but is_open() now reads
back what was set, and a future design can separate the two without another API
change.

Tests

  • crates/base/src/component_traits.rs: a trigger implementing only selected
    still receives open state through the default open/is_open.
  • crates/base/src/popover.rs: a recording trigger that keeps open and
    selected apart asserts an opening popover calls open(true) and never
    touches selected. Confirmed failing before the change
    (got [("selected", true), ...]).
  • crates/component/src/button/button.rs: open and selected are stored
    apart and both paint the selected style.
cargo test -p gpui-base --lib popover              6 passed
cargo test -p gpui-base --lib component_traits     2 passed
cargo test -p gpui-component --lib                 525 passed
cargo clippy --workspace --all-targets -- -D warnings   clean
cargo fmt --check / typos                          clean

Docs

Selectable's trait doc explains what open means and when to override it.
website/component/popover.md and website/zh-CN/component/popover.md gain a
"Custom Trigger" / 「自定义触发器」 section showing a trigger that implements
Selectable and overrides open/is_open to keep the two states apart.

Breaking Changes

None. open/is_open are default methods that fall back to selected/
is_selected, so existing Selectable implementations and trigger call sites
compile and behave exactly as before.

🤖 Generated with Claude Code

A popover mapped its open state onto `Selectable::selected`, so a trigger
whose selection already meant something else — a sidebar row that is
selected when it is the current view — had to be wrapped in an adapter
that re-mapped selection back into open state.

`Selectable` now carries `open`/`is_open` alongside `selected`, defaulting
to the selected pair, so existing triggers are unchanged. Popover, the
dropdown menu and the app menu bar mark their trigger with `open`, and
Button keeps the two apart in its own state while still painting them the
same.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@huacnlee
huacnlee merged commit c814086 into main Sep 18, 2026
12 checks passed
@huacnlee
huacnlee deleted the popover-open-state branch September 18, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant