Skip to content

Fix inverted on/off semantics of toggle bar - #12022

Closed
ram-devv1 wants to merge 1 commit into
omacom:quattrofrom
ram-devv1:fix/toggle-bar-semantics
Closed

ram-devv1 wants to merge 1 commit into
omacom:quattrofrom
ram-devv1:fix/toggle-bar-semantics

Conversation

@ram-devv1

Copy link
Copy Markdown

Fixes #11817, fixes the inversion half of #11769.

The flag is named bar-off (present = hidden) but omarchy-toggle-bar forwarded on/off literally, so on hid the bar and off showed it — the opposite of the documented examples.

  • Invert on/off in omarchy-toggle-bar before delegating to omarchy-toggle (toggle passes through).
  • Invert the delegation in omarchy-toggle-fullscreen-desktop so entering full screen still hides the bar.
  • Update toggle-test.sh and the session acceptance test, which both enshrined the inverted behavior.

Verified with a flag-level repro (on clears, off sets, toggle flips); toggle-test.sh passes including the fullscreen-desktop cases.

@llstrk

llstrk commented Sep 27, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: the fix gives omarchy toggle bar on/off the documented meaning. The no-argument form used by the hotkey and menu still flips the bar, and the full-screen desktop toggle ends in the same state as before. One shell test case no longer sets up the state it names, and three related open PRs would conflict with the new semantics if merged alongside it.

Command (isolated run, bar-off flag result) Base This PR
omarchy-toggle-bar on flag set (bar hidden) flag cleared (bar shown)
omarchy-toggle-bar off flag cleared (bar shown) flag set (bar hidden)
no argument, "", toggle flips flips
omarchy toggle bar on/off via the omarchy dispatcher inverted on shows, off hides
omarchy-toggle-fullscreen-desktop (4 start states × 6 argument forms) reference identical final flags and exit status in all 24 cases

Verified (callers): the SUPER + SHIFT + SPACE binding (default/hypr/bindings/utilities.lua:16) and the menu entry (default/omarchy/omarchy-menu.jsonc:95) call omarchy-toggle-bar with no argument, so they are unaffected. Bar.qml reads the flag file (and the notifications service reads the bar's hidden state), and the flag's meaning (present = hidden) is unchanged, so existing installs keep their current bar state and no migration is needed. Apart from the full-screen toggle and the tests this PR updates, no script, migration or doc in the repository passes on/off to the bar toggle.

Verified (tests): the updated test/shell.d/toggle-test.sh passes on the PR head and fails on the base scripts at bar off sets the bar-off flag to hide the bar. The swapped acceptance test (session-test.sh) is consistent with the new meaning; it was read, not run.

Full-screen "half-hidden" test case no longer starts half-hidden

test/shell.d/toggle-test.sh:75 still calls omarchy-toggle-bar on. Before this PR that set bar-off, giving a bar-hidden, gaps-on desktop. Now it clears a flag that line 71 already cleared, so the case at lines 76-77 starts from a fully visible desktop and repeats the earlier "enter full screen" case.

state just before line 76      bar-off   window-no-gaps
base scripts + base test       set       absent          (half-hidden, as named)
PR scripts   + PR test         absent    absent          (fully visible)

Impact: the pull-into-line rule at bin/omarchy-toggle-fullscreen-desktop:13 is no longer tested. Two diagnostic mutants of that rule (decide by bar-off alone; || instead of &&) pass the PR's test but are caught by the base test and by the corrected line below.

Suggested change:

-HOME="$test_home" omarchy-toggle-bar on
+HOME="$test_home" omarchy-toggle-bar off

With this change the PR scripts still pass and both mutants fail at fullscreen toggle pulls a half-hidden desktop into full screen.

Related open PRs rely on the old meaning

These PRs apply cleanly to the current base but assume the pre-fix behaviour:

Impact: whichever of these merges alongside this fix would hide the bar when it should show it and show it when it should hide it (or, for #11803, document the reverse of the actual behaviour). The combination raises no merge conflict, and presentation mode itself reports no error; only the presentation PRs' own shell test catches it.

Suggested change: in whichever PR lands second, swap the two omarchy-toggle-bar calls in bin/omarchy-toggle-presentation; the presentation test already expects the right outcome and passes unchanged once the calls are swapped. Drop or reword #11803's summary if this fix merges.

Note: #7023, #12150, #12155 and #13451 also implement the on/off inversion in the same files, and each conflicts with this PR in a local merge, so only one of them can land as is. #7023 does not update omarchy-toggle-fullscreen-desktop; in an isolated run with #7023's wrapper on the current quattro tip, entering full screen showed the bar instead of hiding it, and the toggle no longer left full screen. This PR includes that caller.

Optional: unknown arguments (for example show, hide, ON) still pass through to omarchy-toggle, which prints Usage: omarchy-toggle <flag-name> [toggle|on|off], while omarchy-toggle-bar exits 0. This is the same on the base. After this PR, that usage line describes on/off in the flag's sense, the opposite of the bar command's. A usage arm in the case of omarchy-toggle-bar would give a correct message and a non-zero exit.


Review information

Test scope: Head 3110117e, base quattro at 2fbac0c8; the four changed files are identical on the current quattro tip. omarchy-toggle-bar (11 argument forms × 2 start states) and omarchy-toggle-fullscreen-desktop (24 cases) were run on the base, the head and a local merge onto the current quattro tip, and the omarchy dispatcher route on the base and the head, in an isolated sandbox with synthetic home directories. toggle-test.sh was run, including cross runs (head test on base scripts and the reverse) and the mutants above. Related PRs were checked from their diffs and by local merges with this PR onto the quattro tip; the presentation tests of #12422 and #12730 were run on those merges. Not run: the graphical acceptance test, the full test/shell and test/cli suites, and the shell IPC nudge (omarchy-shell -q omarchy.bar syncHidden, unchanged by this PR). No live desktop or real bar was used.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed at 3110117. The bug is real: on quattro, omarchy-toggle-bar passes on/off straight to the bar-off flag, so omarchy toggle bar on hides the bar. Inverting the arguments in the wrapper and compensating in omarchy-toggle-fullscreen-desktop is the right size of fix, and your changes to both scripts and to session-test.sh are correct.

This is one of six open pull requests that make the same fix: #7023, #12022, #12150, #12155, #13451 and #13670. Only one of them can land, so I compared them rather than verifying each one separately. #7023 is the one being taken forward. It is the oldest, it has already been brought up to date with quattro and verified on a disposable Omarchy VM, and it covers everything this one does. It also gets two things right that this one does not:

  • test/shell.d/toggle-test.sh:75 still runs omarchy-toggle-bar on to set up the "fullscreen toggle pulls a half-hidden desktop into full screen" case. With the new meaning, that line leaves the bar visible, so the case starts from a fully visible desktop. It passes, but it repeats the ordinary entry case and no longer tests the pull-into-line rule at bin/omarchy-toggle-fullscreen-desktop:13. It needs omarchy-toggle-bar off, which is what Fix inverted on/off semantics in omarchy-toggle-bar #7023 has.
  • An unknown argument such as omarchy-toggle-bar bogus prints omarchy-toggle's usage line but exits 0, because the shell nudge after it succeeds. That behaviour is already on quattro, so this pull request did not introduce it. Fix inverted on/off semantics in omarchy-toggle-bar #7023 rejects the argument with its own usage line and exit status, and tests that the flag is left alone.

Because this one lost the comparison, it was not run on a worker. The comparison and the two findings above come from reading the diffs against quattro. Codex Medium reviewed the same six diffs separately. It also picked #7023 and found the same toggle-test.sh:75 problem. It may have read this review's working notes, so its agreement is not guaranteed to be independent.

This now waits on the maintainer to choose between the competing fixes. #11803, which is still open, documents the current behaviour ("on hides the bar, off shows it") as intended and would be wrong after any of these fixes merges, so that is part of the same decision.

@omarchybot

Copy link
Copy Markdown
Collaborator

Thank you for this, @ram-devv1. #7023 and #11803 fixes the same problem in a way the review judged better, so this is closed in favour of it. If it does not cover your case, please say so there.

Closed at the maintainer's request. The review comment above has the details; it was done by Claude Opus 5.5 with Codex Medium as a second opinion, whose agreement is not independent.

@omarchybot omarchybot closed this Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

omarchy toggle bar on/off has inverted semantics

4 participants