[V3] Fix initially-hidden menuItem [mac/win] #4116
Conversation
WalkthroughThis pull request introduces a hidden menu item labeled "I was hidden" in the Demo submenu and a new toggle menu item to switch its visibility. The toggle action is implemented via an Changes
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
IanVS
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/pkg/application/menu_darwin.go (1)
102-104: Good fix for initially-hidden menu itemsThis change appropriately addresses the issue by ensuring hidden menu items are still created but visually suppressed using
setHidden(true). Previously, it appears these items were skipped entirely during menu processing, making them impossible to show later.This implementation allows for dynamic toggling of menu item visibility at runtime, which aligns with the PR objective of fixing initially-hidden menuItems on macOS.
This approach is architecturally sound as it maintains a consistent relationship between the application's menu model and the OS menu representation, regardless of visibility state.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/examples/menu/main.go(1 hunks)v3/pkg/application/menu_darwin.go(1 hunks)
🔇 Additional comments (1)
v3/examples/menu/main.go (1)
47-52: Good demonstration of the hidden menu functionalityThis implementation effectively demonstrates the new functionality:
- Creates a menu item that's initially hidden
- Provides a toggle mechanism to change visibility at runtime
- Uses the
Hidden()method to check current state and negate itThe example serves as good documentation for how developers should implement this feature in their applications.
One small suggestion to enhance this example even further:
// Hidden menu item that can be unhidden hidden := myMenu.Add("I was hidden").SetHidden(true) myMenu.Add("Toggle hidden menu").OnClick(func(ctx *application.Context) { hidden.SetHidden(!hidden.Hidden()) + // Update the toggle menu text to reflect current state + if hidden.Hidden() { + ctx.ClickedMenuItem().SetLabel("Show hidden menu") + } else { + ctx.ClickedMenuItem().SetLabel("Hide menu item") + } })
Previously, when calling `.SetHidden(false)` on a menu item that was not hidden, a duplicate menu item was created.
IanVS
force-pushed
the
fix-hidden-menuitem
branch
from
5140cc0 to
42c6c2b
Compare
IanVS
changed the title
And bring menu_windows into alignment with popupmenu_windows
Previously, when calling `.SetHidden(false)` on a menu item that was not hidden, a duplicate menu item was created.
And bring menu_windows into alignment with popupmenu_windows
…nVS/fix-hidden-menuitem
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/src/content/docs/changelog.mdx (1)
78-79: Changelog Entry for the NewSetMenu()Feature
This new bullet point clearly documents the addition of theSetMenu()method on the window. Please verify that:
- The changelog description accurately reflects the implementation and intended usage of
SetMenu().- There is consistency across the documentation and examples regarding any platform-specific behaviors (for instance, if there are differences between macOS, Windows, and Linux).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/src/content/docs/changelog.mdx (2)
78-79: New Changelog Entry forSetMenu()Method:
A new changelog entry now documents the addition of theSetMenu()method on the window interface. Please ensure that its usage is also clearly described in the relevant documentation (and examples) so that developers can understand how it integrates with dynamic menu handling.
109-109: Stylistic Consistency for Compound Modifier:
The entry “Fixed initially-hidden menu items…” uses the hyphen in “initially-hidden.” It is standard in English not to hyphenate when an adverb ending in “ly” modifies an adjective. Consider revising it to “Fixed initially hidden menu items…” for clarity and consistency. For example:- Fixed initially-hidden menu items by [@IanVS](https://github.com/IanVS) in [#4116](https://github.com/wailsapp/wails/pull/4116) + Fixed initially hidden menu items by [@IanVS](https://github.com/IanVS) in [#4116](https://github.com/wailsapp/wails/pull/4116)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/content/docs/changelog.mdx(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/changelog.mdx
[uncategorized] ~111-~111: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...b.com//pull/4100) - Fixed initially-hidden menu items by [@IanVS](https://github.c...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/src/content/docs/changelog.mdx (2)
78-78: Clarify the SetMenu() Changelog EntryThe new changelog entry "Add
SetMenu()on window to allow for setting a menu on a window" is clear and concise. Please ensure that the accompanying documentation in the relevant docs is updated to explain the intended use and any caveats if needed.
111-111: Review Hyphenation in the 'Fixed initially-hidden menu items' EntryAccording to standard style guidelines, compound modifiers with adverbs ending in “ly” (e.g., "initially") do not require a hyphen. It might be clearer to write "Fixed initially hidden menu items".
🧰 Tools
🪛 LanguageTool
[uncategorized] ~111-~111: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...b.com//pull/4100) - Fixed initially-hidden menu items by [@IanVS](https://github.c...(HYPHENATED_LY_ADVERB_ADJECTIVE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/content/docs/changelog.mdx(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/changelog.mdx
[uncategorized] ~111-~111: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...b.com//pull/4100) - Fixed initially-hidden menu items by [@IanVS](https://github.c...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
Description
Fixes #4088
This addresses the referenced issue, allowing menu items to be hidden at first, and then shown in response to some other event.
This also fixes a bug on Windows, which caused duplicate menu items to be added when calling
.SetHidden(false)on a menu item that was not hidden.Type of change
Please select the option that is relevant.
How Has This Been Tested?
I tested this by updating the
menuexample and checking that I can toggle the visibility of the menu item.I tried in windows, but was not able to figure it out. I thought maybe removing the
continuein the similarprocessMenuwould have an effect, but it did not. In fact commenting out the entireprocessMenufunction body seemed to have no effect. I don't know what's going on there.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Bug Fixes
Documentation