Skip to content

fix(tray): apply menu property updates and activate the correct radio item - #1583

Merged
JakeStanger merged 1 commit into
JakeStanger:masterfrom
tomasz-feliksik:pr/tray-fixes
Sep 19, 2026
Merged

JakeStanger merged 1 commit into
JakeStanger:masterfrom
tomasz-feliksik:pr/tray-fixes

Conversation

@tomasz-feliksik

Copy link
Copy Markdown

Two related tray menu fixes which together make menus with radio groups work
end-to-end. The first implements the menu update bug you diagnosed in #1538;
the second fixes a wrong-id activation bug in the radio path that the first
fix made observable.

1. Apply MenuDiff updates so menus repaint after a click

The tray interface handled UpdateEvent::Menu (full replace) and
MenuConnect, but only trace!'d UpdateEvent::MenuDiff and dropped it.
MenuDiff is how the standard com.canonical.dbusmenu
ItemsPropertiesUpdated signal reaches the module, so every post-click
property change — a moved radio toggle-state, a refreshed label, an
enabled/disabled entry — was discarded and the widget kept rendering the
previous state. This is the case you described in #1538:

this is the first tray app I've seen that actually sends menu property
update events rather than replacing the whole thing, and I just skipped
implementing it previously as I had nothing to test against.

It affects any conforming tray implementation, not one toolkit: Qt,
libdbusmenu and ksni all report property-only changes this way and only
escalate to LayoutUpdated when the structure changes.

Why apply the diffs in ironbar rather than in system-tray: the crate's
own apply_menu_diffs walks only the top-level submenus and consumes diffs
positionally (next_if(|d| d.id == item.id)), so nested groups are never
patched — but fixing that alone wouldn't be enough, because ironbar dropped
the event before the widget could repaint. The widget side needs a retained
menu model and a rebuild regardless, so TrayMenu now keeps the last full
menu model and applies diffs itself, matching by global item id at any depth
(src/modules/tray/diff.rs, unit-tested including the nested-radio case).
Happy to send a recursion fix for the crate's apply_menu_diffs as a
follow-up if you'd take it.

2. Activate the clicked radio item, not the group's first

Clicking any radio item sent one fixed dbusmenu id, so the application either
did nothing or silently re-applied a setting it already had. Two defects
combined:

  1. Colliding action names. as_menu numbered radio groups with a counter
    local to the function, but recursed into each submenu carrying the same
    SimpleActionGroup. The counter restarted per submenu while the namespace
    did not, so every submenu's first group was named action_radio_1
    and ActionMap::add_action replaces on a duplicate name, leaving one
    surviving action driven by every radio item in the menu.
  2. The change-state handler discarded its parameter. One action backs a
    whole group, so the clicked option is identified only by the target string
    in the new state. The handler was move |_, _| activate(.., id), using an
    id captured at build time — the group's first item. On its own this breaks
    every group of more than one option, even without collisions.

The fix names the action from the group's first dbusmenu id (unique
menu-wide, so collision-free by construction) and reads the clicked id back
out of the change-state payload. The group's state is also parked on the item
the application reports as selected, so the radio mark renders on the actually
active option instead of always the first.

Standard and checkmark items were never affected — they use the globally
unique action_{id} path — which is why tray menus looked mostly functional
and only radio groups misbehaved.

Testing

  • Unit tests for the diff application (nested radio toggle-state move, label
    update, unknown-id no-op) and for the radio action contract (action-name
    uniqueness across groups, target string round-tripping the item id).
  • Verified live against a ksni app whose menu has five radio groups in five
    submenus. Before: every click in every group sent id 19 (confirmed via
    busctl … GetLayout against the app's real menu ids plus trace logging of
    the outgoing ActivateRequest). After: the sent id varies per click,
    selections apply across different groups and both options within a group,
    and the menu repaints to show the moved selection without reopening.
  • cargo fmt --check, cargo clippy --all-targets (0 warnings), builds with
    --no-default-features, cargo test all pass.

Refs #1538 — this implements the menu update part; the scroll-event and
libappindicator points from that issue are out of scope here.

@github-actions github-actions Bot added the Z:Review Required Pull request pending review label Aug 9, 2026
@tomasz-feliksik

Copy link
Copy Markdown
Author

PR description written using AI, but I've analysed and tested the code myself.
@JakeStanger - this is my first contribution to Ironbar, and since I'm using it every day probably not the last one. Let me know if anything can be improved ;)

@tomasz-feliksik

Copy link
Copy Markdown
Author

Pushed one more commit: fix(tray): detach the previous shortcut controller on menu rebuild.

While soak-testing this branch as my daily bar I found build_menu_widget leaked a ShortcutController per menu rebuild — add_controller appends, and nothing ever removed the previous controller (insert_action_group, by contrast, replaces by name). The leak predates this PR, but the MenuDiff handling here makes rebuilds more frequent, so it seemed wrong to ship the one without the other.

Measured with a scripted SNI app emitting 6000 toggle-state property updates (each one a rebuild): pre-fix RSS grew ~11 MB and accelerating; with the fix it stays flat (+0.17 MB total).

@JakeStanger

Copy link
Copy Markdown
Owner

The repo's AI policy clearly disallows vibe coding, and I would say this definitely qualifies as that. Since the damage is already done, I'm not going to close it and I'll look to merge this. Just don't expect a quick review because I've got to unpick a lot of fluff, and don't expect to credited.

@tomasz-feliksik

Copy link
Copy Markdown
Author

Fair - and to be fully straight: yes, the code was AI-assisted.

For what it's worth: I'd already fixed this for my own use - upstreaming it was meant as a thank-you for Ironbar, not a demand on your time. I tried to put more effort into the PR than the review would cost; I can see the extra prose achieved the opposite, and that's mine to fix. The diagnosis and testing were mine and I can answer for every line, so one ask: judge the code as code, at whatever pace suits you, or not at all - no obligation.

If it helps, I'll strip the comments and tests down to house style and split the leak fix into its own PR - say the word. No expectations on credit either.

@JakeStanger JakeStanger reopened this Sep 19, 2026
@github-actions github-actions Bot added the Z:Slop Detected PR has been detected to be potentially AI slop label Sep 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been automatically detected as being AI slop.

If you believe this is incorrect, please reply and it will be manually reviewed.

@github-actions github-actions Bot closed this Sep 19, 2026
@JakeStanger JakeStanger added the Z:Ignore AI Checks Disable anti-AI checks for this PR label Sep 19, 2026
@JakeStanger

Copy link
Copy Markdown
Owner

Cool, that works

@JakeStanger JakeStanger reopened this Sep 19, 2026
@JakeStanger
JakeStanger merged commit ef5e520 into JakeStanger:master Sep 19, 2026
137 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Z:Ignore AI Checks Disable anti-AI checks for this PR Z:Review Required Pull request pending review Z:Slop Detected PR has been detected to be potentially AI slop

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants