V3/mac menu fixes#4312
Conversation
still bugs present but this fixes the window not rendering due to an update loop on startup
WalkthroughThe changes improve submenu label updates in the menu example by using stored submenu references with error handling and detailed output. The macOS application menu setup is refactored for explicit initialization, error reporting, and safer native menu assignment. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainApp
participant Submenu
User->>MainApp: Request submenu label update
MainApp->>Submenu: Check submenu reference and current label
alt Submenu reference is valid and label differs
Submenu->>Submenu: Set new label
MainApp->>MainApp: Call menu.Update()
MainApp->>User: Print success and verification messages
else Submenu reference is nil
MainApp->>User: Print error message
else Label already set
MainApp->>User: Print no update needed message
end
sequenceDiagram
participant macosApp
participant Menu
participant macosMenuImpl
participant MainThread
macosApp->>Menu: Check if menu is nil
alt menu is nil
macosApp->>Menu: Create default menu
Menu->>Menu: Update to initialize impl and nsMenu
Menu->>macosApp: Return impl
alt impl is nil
macosApp->>macosApp: Print error and abort
else
macosApp->>MainThread: Set native menu pointer
end
else
macosApp->>Menu: Check impl
alt impl is nil
macosApp->>Menu: Initialize impl (process radio groups, create macosMenu)
macosMenuImpl->>macosMenuImpl: update()
end
alt impl not macosMenuImpl
macosApp->>macosApp: Print critical error and abort
else
macosApp->>MainThread: Set native menu pointer
end
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
|
This PR has been expanded to beyond just Mac fixes and is a rewrite of how menus are organized |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
v3/pkg/application/menu.go (1)
118-127: Consider optimizing label-based lookups.The current implementation performs a linear search through all elements, which could be slow with many menu items. Consider maintaining a secondary map for label-to-element mapping.
For better performance with frequent label lookups, you could maintain a secondary index:
var ( globalMenuRegistry = make(map[string]MenuElementInterface) globalLabelRegistry = make(map[string][]MenuElementInterface) // labels can be non-unique globalMenuRegistryMutex sync.RWMutex globalElementIDCounter uint64 )Then update registration/unregistration to maintain both indexes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v3/pkg/application/menu.go(8 hunks)v3/pkg/application/menu_darwin.go(2 hunks)v3/pkg/application/menuitem.go(3 hunks)v3/pkg/application/menuitem_dev.go(1 hunks)v3/pkg/application/menuitem_roles.go(19 hunks)v3/pkg/application/menuitem_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
v3/pkg/application/menuitem_roles.go (3)
v3/pkg/application/roles.go (5)
Hide(21-21)HideOthers(22-22)Quit(36-36)ToggleFullscreen(45-45)Open(52-52)v3/pkg/application/menuitem.go (2)
MenuItem(51-72)NewMenuItem(74-84)v3/pkg/application/context.go (1)
Context(3-6)
v3/pkg/application/menu_darwin.go (3)
v3/pkg/application/menu.go (1)
Menu(63-86)v3/pkg/application/menuitem.go (2)
NewMenuItem(74-84)MenuItem(51-72)v3/pkg/application/roles.go (1)
ServicesMenu(18-18)
🔇 Additional comments (18)
v3/pkg/application/menuitem_dev.go (1)
7-13: LGTM!The method calls have been correctly updated to use the new fluent interface methods (
SetAcceleratorItemandOnClickItem) as part of the broader MenuItem API refactoring.v3/pkg/application/menuitem_test.go (1)
17-17: Test updates correctly reflect the API changes.The test cases have been properly updated to use
SetAcceleratorIteminstead ofSetAccelerator, maintaining test coverage for the refactored MenuItem API.Also applies to: 44-44
v3/pkg/application/menuitem_roles.go (1)
16-17: All method calls have been correctly updated to the new API.The systematic replacement of
SetAccelerator→SetAcceleratorItem,SetRole→SetRoleItem, andOnClick→OnClickItemhas been applied consistently throughout all menu item factory functions. Platform-specific logic and accelerator overrides have been preserved correctly.Also applies to: 22-23, 36-36, 38-38, 51-51, 53-53, 65-65, 68-68, 80-80, 83-83, 95-95, 98-98, 110-110, 115-115, 118-118, 136-137, 144-144, 147-147, 163-163, 170-171, 181-182, 192-193, 203-204, 211-211, 219-220, 230-231, 241-242, 252-253, 263-263, 273-273, 283-283, 288-288, 301-301, 306-307, 312-312, 317-317, 322-322, 327-327, 332-332, 337-337, 342-342, 347-347, 352-352, 357-357
v3/pkg/application/menu_darwin.go (3)
77-77: Good encapsulation improvement.Using the
Label()method instead of direct field access aligns with the new interface-based design.
87-100: Menu-as-submenu handling looks correct.The explicit type checking and handling for
Menuitems is a good improvement for type safety. The temporaryMenuItemcreation pattern (lines 93-96) appears necessary to bridge between the Go menu structure and the native NSMenu API.
103-136: MenuItem processing is well-structured.The refactored code properly handles different menu item types with clear separation of concerns. Good improvements:
- Using
Hidden()method instead of direct field access (line 119)- Proper bitmap handling within the MenuItem context (lines 127-134)
- Preserved ServicesMenu role handling (lines 113-115)
v3/pkg/application/menuitem.go (6)
18-38: Thread-safe implementation looks good!The global menu item registry is properly protected with mutex locks, and the ID generation uses atomic operations for thread safety.
53-72: Good struct enhancements for interface implementation.The addition of
elementIDandparentMenufields properly supports the new MenuElementInterface design and hierarchical menu structure.
139-232: Excellent implementation of MenuElementInterface!The interface methods are well-implemented with:
- Proper nil checks for platform implementations
- Consistent label synchronization between menu items and their submenus
- Good error handling in SetAccelerator
- Recursive update propagation for submenus
329-355: Well-implemented deep copy functionality.The Clone method properly:
- Generates new unique IDs to avoid conflicts
- Performs deep copies of all nested structures
- Registers the cloned item in the global registries
357-380: Thorough cleanup in Destroy method.The method properly handles all cleanup tasks:
- Unregisters from both global registries
- Destroys platform implementations
- Recursively destroys submenus
- Clears all references to prevent memory leaks
523-557: Good convenience methods for fluent chaining.These wrapper methods maintain backward compatibility while allowing fluent method chaining by returning
*MenuIteminstead of the interface type.v3/pkg/application/menu.go (6)
14-37: Well-designed interface for unified menu element handling.The MenuElementInterface provides a clean abstraction that enables:
- Uniform handling of both Menu and MenuItem types
- Method chaining with the interface return type
- Clear separation of concerns (identity, state, behavior)
63-86: Excellent improvements to Menu struct design.The changes enable:
- Flexible element types through interface usage
- Fast O(1) lookups with local item registry
- Proper thread safety with read-write mutex
- Support for mixed menu hierarchies
260-303: Well-structured update propagation logic.The method properly:
- Recursively updates all child elements
- Handles radio group processing
- Safely updates attached windows with proper locking
Note: The
convertToOldMenu()method currently returns self - ensure platform-specific implementations override this if needed.
396-423: Robust implementation of InsertAtIndex.The method properly:
- Handles bounds checking with clamping
- Correctly inserts elements in the slice
- Updates the registry
- Sets parent references for both Menu and MenuItem types
596-635: Well-implemented radio group processing.The method correctly:
- Groups consecutive radio menu items
- Handles type conversions for backward compatibility
- Recursively processes submenus
- Properly closes radio groups when encountering non-radio items
677-685: Correct implementation of Prepend.The method properly inserts all items at the beginning of the menu while maintaining their relative order.
| func NewMenuItemSeparator() *MenuItem { | ||
| result := &MenuItem{ | ||
| id: uint(atomic.AddUintptr(&menuItemID, 1)), | ||
| itemType: separator, | ||
| id: uint(atomic.AddUintptr(&menuItemID, 1)), | ||
| elementID: generateElementID(), | ||
| itemType: separator, | ||
| } | ||
| registerElement(result) | ||
| return result | ||
| } |
There was a problem hiding this comment.
Missing menu item map registration for separators.
The separator constructor doesn't call addToMenuItemMap(result), which is inconsistent with other menu item constructors. This could cause issues when trying to look up separators by ID using getMenuItemByID().
Apply this diff to fix the registration:
func NewMenuItemSeparator() *MenuItem {
result := &MenuItem{
id: uint(atomic.AddUintptr(&menuItemID, 1)),
elementID: generateElementID(),
itemType: separator,
}
+ addToMenuItemMap(result)
registerElement(result)
return result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewMenuItemSeparator() *MenuItem { | |
| result := &MenuItem{ | |
| id: uint(atomic.AddUintptr(&menuItemID, 1)), | |
| itemType: separator, | |
| id: uint(atomic.AddUintptr(&menuItemID, 1)), | |
| elementID: generateElementID(), | |
| itemType: separator, | |
| } | |
| registerElement(result) | |
| return result | |
| } | |
| func NewMenuItemSeparator() *MenuItem { | |
| result := &MenuItem{ | |
| id: uint(atomic.AddUintptr(&menuItemID, 1)), | |
| elementID: generateElementID(), | |
| itemType: separator, | |
| } | |
| addToMenuItemMap(result) | |
| registerElement(result) | |
| return result | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/menuitem.go around lines 86 to 94, the
NewMenuItemSeparator function creates a separator MenuItem but does not register
it in the menu item map by calling addToMenuItemMap(result). To fix this, add a
call to addToMenuItemMap(result) after creating the MenuItem and before
returning it, ensuring separators are properly registered and can be looked up
by ID.
| func (m *MenuItem) RemoveAccelerator() { | ||
| m.accelerator = nil | ||
| } |
There was a problem hiding this comment.
Platform implementation not updated when removing accelerator.
The method sets the accelerator to nil but doesn't propagate this change to the platform implementation, which could cause the accelerator to still function.
Apply this diff to fix the issue:
func (m *MenuItem) RemoveAccelerator() {
m.accelerator = nil
+ if m.impl != nil {
+ m.impl.setAccelerator(nil)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *MenuItem) RemoveAccelerator() { | |
| m.accelerator = nil | |
| } | |
| func (m *MenuItem) RemoveAccelerator() { | |
| m.accelerator = nil | |
| if m.impl != nil { | |
| m.impl.setAccelerator(nil) | |
| } | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/menuitem.go around lines 268 to 270, the RemoveAccelerator
method sets the accelerator field to nil but does not update the
platform-specific implementation, causing the accelerator to remain active.
Modify the method to also call the appropriate platform implementation function
to remove or clear the accelerator, ensuring the change is fully propagated.
| func (m *Menu) AddElement(element MenuElementInterface) *Menu { | ||
| m.registryMutex.Lock() | ||
| defer m.registryMutex.Unlock() | ||
|
|
||
| m.items = append(m.items, element) | ||
| m.itemRegistry[element.ID()] = element | ||
|
|
||
| // Set parent reference | ||
| if menu, ok := element.(*Menu); ok { | ||
| menu.parentMenu = m | ||
| } | ||
|
|
||
| return m | ||
| } |
There was a problem hiding this comment.
Missing parent reference for MenuItem elements.
The method sets the parent reference for Menu elements but not for MenuItem elements, even though MenuItem has a parentMenu field.
Apply this diff to fix the parent reference:
func (m *Menu) AddElement(element MenuElementInterface) *Menu {
m.registryMutex.Lock()
defer m.registryMutex.Unlock()
m.items = append(m.items, element)
m.itemRegistry[element.ID()] = element
// Set parent reference
if menu, ok := element.(*Menu); ok {
menu.parentMenu = m
+ } else if item, ok := element.(*MenuItem); ok {
+ item.parentMenu = m
}
return m
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *Menu) AddElement(element MenuElementInterface) *Menu { | |
| m.registryMutex.Lock() | |
| defer m.registryMutex.Unlock() | |
| m.items = append(m.items, element) | |
| m.itemRegistry[element.ID()] = element | |
| // Set parent reference | |
| if menu, ok := element.(*Menu); ok { | |
| menu.parentMenu = m | |
| } | |
| return m | |
| } | |
| func (m *Menu) AddElement(element MenuElementInterface) *Menu { | |
| m.registryMutex.Lock() | |
| defer m.registryMutex.Unlock() | |
| m.items = append(m.items, element) | |
| m.itemRegistry[element.ID()] = element | |
| // Set parent reference | |
| if menu, ok := element.(*Menu); ok { | |
| menu.parentMenu = m | |
| } else if item, ok := element.(*MenuItem); ok { | |
| item.parentMenu = m | |
| } | |
| return m | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/menu.go around lines 210 to 223, the AddElement method
sets the parentMenu field only for elements of type Menu but not for MenuItem
elements, which also have a parentMenu field. To fix this, add a similar check
for MenuItem type elements and set their parentMenu field to the current Menu
instance (m) inside the method, ensuring both Menu and MenuItem elements have
their parent references correctly assigned.
| func (m *Menu) Clear() { | ||
| for _, item := range m.items { | ||
| if menuItem, ok := item.(*MenuItem); ok { | ||
| removeMenuItemByID(menuItem.id) | ||
| } | ||
| unregisterElement(item.ID()) | ||
| } | ||
| m.items = nil | ||
| } |
There was a problem hiding this comment.
Incomplete cleanup - submenus are not destroyed.
The Clear method only cleans up MenuItem elements but ignores Menu elements (submenus), which could lead to resource leaks.
Apply this diff to properly clean up all elements:
func (m *Menu) Clear() {
for _, item := range m.items {
if menuItem, ok := item.(*MenuItem); ok {
removeMenuItemByID(menuItem.id)
+ } else if submenu, ok := item.(*Menu); ok {
+ submenu.Destroy()
}
unregisterElement(item.ID())
}
m.items = nil
+ m.itemRegistry = make(map[string]MenuElementInterface)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *Menu) Clear() { | |
| for _, item := range m.items { | |
| if menuItem, ok := item.(*MenuItem); ok { | |
| removeMenuItemByID(menuItem.id) | |
| } | |
| unregisterElement(item.ID()) | |
| } | |
| m.items = nil | |
| } | |
| func (m *Menu) Clear() { | |
| for _, item := range m.items { | |
| if menuItem, ok := item.(*MenuItem); ok { | |
| removeMenuItemByID(menuItem.id) | |
| } else if submenu, ok := item.(*Menu); ok { | |
| submenu.Destroy() | |
| } | |
| unregisterElement(item.ID()) | |
| } | |
| m.items = nil | |
| m.itemRegistry = make(map[string]MenuElementInterface) | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/menu.go around lines 525 to 533, the Clear method
currently only cleans up MenuItem elements and does not handle Menu elements
(submenus). To fix this, update the Clear method to check if each item is a
MenuItem or a Menu. For MenuItem, call removeMenuItemByID with its id; for Menu
(submenu), call its Clear method recursively to ensure proper cleanup. Then
unregister the element by its ID as before and finally set m.items to nil.
atterpac
merged commit a3c0024
into
wailsapp:v3-alpha-chore/menu-fixes-2
1 participant
Fixes infinite loop on startup and fixes example to match changes to API
Summary by CodeRabbit
New Features
Bug Fixes
Refactor