Add Layouts Submenu to Application Menu #1804
Conversation
rgeraskin
force-pushed
the
add-layouts-to-menu
branch
from
b632ea3 to
eefc761
Compare
|
rebased on development branch |
Amethyst/AppDelegate.swift
Outdated
| currentLayoutKey = firstScreenManager.currentLayout?.layoutKey | ||
| } | ||
|
|
||
| // Filter to only enabled layouts and add menu items |
There was a problem hiding this comment.
This logic isn't quite correct because it is technically possible for there to be multiple instances of the same layout in a space.
There was a problem hiding this comment.
Fixed. Added currentLayoutIndexValue property to ScreenManager to expose the current layout index. The menu now compares by index instead of by layoutKey:
- Before:
currentLayout?.layoutKey == layoutKey(would checkmark ALL instances of same layout type) - After:
index == currentLayoutIndex(only checkmarks the specific instance)
This correctly handles configurations like [Tall, Wide, Tall] where only the active instance shows a checkmark.
- Populate layouts menu when main menu opens (not submenu) - Add screen detection fallback using mouse cursor position - Show error placeholder only when no screen manager available - Move empty layouts check before loop with early return - Fix multiple layout instances by comparing index instead of key - Add currentLayoutIndexValue property to ScreenManager
ianyh
left a comment
ianyh
left a comment
There was a problem hiding this comment.
My point about the layout instances is that we probably want this to be derived from the actual list of layouts in the screen manager.
Amethyst/AppDelegate.swift
Outdated
| let amScreen = AMScreen(screen: nsScreen) | ||
| return windowManager?.screenManager(for: amScreen) | ||
| } | ||
| return windowManager?.screenManager(at: 0) |
There was a problem hiding this comment.
Removed the screenManager(at: 0) fallback. Now returns nil if both focusedScreenManager() and mouse cursor screen detection fail, which will show the error placeholder.
- Add layoutsInfo property to ScreenManager to expose actual layouts - Remove screenManager(at: 0) fallback, return nil instead - Use screen manager's layouts for menu instead of UserConfiguration - Ensures indices match correctly for current layout detection
Refactored to derive the menu from the screen manager's actual layouts instead of the global UserConfiguration.shared.layoutKeys():
|
ianyh
left a comment
ianyh
left a comment
There was a problem hiding this comment.
One comment, but otherwise I think it looks good.
| /// The index of the current layout in the layouts array | ||
| var currentLayoutIndexValue: Int { | ||
| return currentLayoutIndex | ||
| } | ||
|
|
||
| /// Returns layout info (key and name) for all layouts in this screen manager | ||
| var layoutsInfo: [(key: String, name: String)] { | ||
| return layouts.map { (key: $0.layoutKey, name: $0.layoutName) } | ||
| } |
There was a problem hiding this comment.
- Could you change layoutsInfo to use a defined struct type instead of the named tuple?
- Could you include whether or not it is the selected layout in that struct instead of exposing the index separately above?
- Add LayoutMenuItemInfo struct with key, name, and isSelected properties - Remove currentLayoutIndexValue property from ScreenManager - Encapsulate selection logic in layoutsInfo property - Simplify AppDelegate menu population loop
2 participants
Add Layouts Submenu to Application Menu
Description
This PR introduces a new layouts submenu in the application menu, providing users with quick access to layout options directly from the menu bar.
Closes: #1080
Changes Made
What's New
Files Modified
Amethyst/AppDelegate.swift(+87 lines)Amethyst/Base.lproj/MainMenu.xib(+7 lines)