-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Enhance window management for Windows: track manual maximized state and restore bounds #1649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…nd restore bounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances window maximize/restore functionality on Windows by implementing custom window bounds management to address platform-specific issues with the native maximize API. The changes introduce manual state tracking and saved bounds restoration to ensure the window returns to its previous size and position when unmaximized.
Key changes:
- Implements Windows-specific maximize logic that manually sets window bounds to the visible work area (excluding taskbar) instead of relying on the native maximize API
- Introduces state tracking via
_manualMaximizedflag and_savedBoundsvariable to remember the pre-maximized window dimensions - Adds
screen_retrieverdependency to obtain display information for calculating the maximized window bounds
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/manager/window_manager.dart
Outdated
| if (system.isWindows && _savedBounds != null) { | ||
| // Restore saved window bounds on Windows | ||
| await windowManager.setBounds(_savedBounds!); | ||
| _savedBounds = null; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When restoring from maximized state on Windows, _savedBounds is set to null immediately after restoration. However, if the setBounds operation fails or throws an exception, the saved bounds will be lost permanently and the window cannot be restored to its previous size. Consider setting _savedBounds to null only after successfully confirming the restoration, or implementing error handling to preserve the saved state.
lib/manager/window_manager.dart
Outdated
| ); | ||
|
|
||
| // On Windows, manually set to work area size (excluding taskbar) | ||
| final display = await screenRetriever.getPrimaryDisplay(); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic assumes the primary display is always where the window should be maximized. In multi-monitor setups, if the window is on a secondary monitor and the user clicks maximize, it will unexpectedly move to the primary display. Consider using screenRetriever.getCursorScreenPoint() or windowManager.getPosition() to determine which display the window is currently on, then maximize to that display's work area instead.
| final display = await screenRetriever.getPrimaryDisplay(); | |
| // Use the display where the window currently resides, not always the primary display. | |
| final display = | |
| await screenRetriever.getDisplayFromPoint(currentPosition); |
lib/manager/window_manager.dart
Outdated
| } else { | ||
| await windowManager.unmaximize(); | ||
| break; | ||
| case false: | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When unmaximizing on Windows and _savedBounds is null (which could occur if the window was maximized through external means like Win+Up arrow key or dragging to screen edge), the code falls back to the generic unmaximize() but _manualMaximized is still set to false. This creates an inconsistent state where the isMaximizedNotifier is false but the window may still be maximized. The else branch at line 184 should only be executed on non-Windows platforms, not when _savedBounds is null on Windows.
lib/manager/window_manager.dart
Outdated
| class _WindowHeaderState extends State<WindowHeader> { | ||
| final isMaximizedNotifier = ValueNotifier<bool>(false); | ||
| final isPinNotifier = ValueNotifier<bool>(false); | ||
| bool _manualMaximized = false; // Track manual maximized state on Windows |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming of this variable could be more precise. The term "manual" suggests user-initiated action, but this actually tracks whether Windows-specific maximize logic was used (as opposed to the native maximize API). Consider renaming to _customMaximizedOnWindows or _windowsBoundsMaximized to better reflect its purpose and reduce confusion.
| Future<void> _updateMaximized() async { | ||
| final isMaximized = await windowManager.isMaximized(); | ||
| switch (isMaximized) { | ||
| case true: | ||
| final isMaximized = system.isWindows | ||
| ? _manualMaximized | ||
| : await windowManager.isMaximized(); | ||
|
|
||
| if (isMaximized) { | ||
| // Unmaximize and restore previous size | ||
| if (system.isWindows && _savedBounds != null) { | ||
| // Restore saved window bounds on Windows | ||
| await windowManager.setBounds(_savedBounds!); | ||
| _savedBounds = null; | ||
| } else { | ||
| await windowManager.unmaximize(); | ||
| break; | ||
| case false: | ||
| } | ||
| _manualMaximized = false; | ||
| isMaximizedNotifier.value = false; | ||
| } else { | ||
| // Maximize window | ||
| if (system.isWindows) { | ||
| // Save current window bounds | ||
| final currentPosition = await windowManager.getPosition(); | ||
| final currentSize = await windowManager.getSize(); | ||
| _savedBounds = Rect.fromLTWH( | ||
| currentPosition.dx, | ||
| currentPosition.dy, | ||
| currentSize.width, | ||
| currentSize.height, | ||
| ); | ||
|
|
||
| // On Windows, manually set to work area size (excluding taskbar) | ||
| final display = await screenRetriever.getPrimaryDisplay(); | ||
| final visiblePosition = display.visiblePosition ?? Offset.zero; | ||
| final visibleSize = display.visibleSize ?? display.size; | ||
|
|
||
| await windowManager.setBounds( | ||
| Rect.fromLTWH( | ||
| visiblePosition.dx, | ||
| visiblePosition.dy, | ||
| visibleSize.width, | ||
| visibleSize.height, | ||
| ), | ||
| ); | ||
| _manualMaximized = true; | ||
| isMaximizedNotifier.value = true; | ||
| } else { | ||
| await windowManager.maximize(); | ||
| break; | ||
| isMaximizedNotifier.value = await windowManager.isMaximized(); | ||
| } | ||
| } | ||
| isMaximizedNotifier.value = await windowManager.isMaximized(); | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _updateMaximized method is not protected against concurrent calls. If a user rapidly double-clicks the window header or if the method is called programmatically in quick succession, multiple async operations could run simultaneously, leading to race conditions. The _savedBounds could be overwritten before restoration completes, or the window could be set to inconsistent states. Consider adding a flag to prevent concurrent execution or using a mutex/lock pattern.
lib/manager/window_manager.dart
Outdated
| await windowManager.setBounds( | ||
| Rect.fromLTWH( | ||
| visiblePosition.dx, | ||
| visiblePosition.dy, | ||
| visibleSize.width, | ||
| visibleSize.height, | ||
| ), | ||
| ); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If screenRetriever.getPrimaryDisplay() fails or returns a display with null values for both visiblePosition and visibleSize (falling back to display.size which could also be null), the code will proceed to call setBounds with potentially invalid dimensions. This could cause the maximize operation to fail silently or result in unexpected window sizing. Consider adding null checks and error handling to gracefully handle cases where display information cannot be retrieved.
| await windowManager.setBounds( | |
| Rect.fromLTWH( | |
| visiblePosition.dx, | |
| visiblePosition.dy, | |
| visibleSize.width, | |
| visibleSize.height, | |
| ), | |
| ); | |
| if (visibleSize != null) { | |
| await windowManager.setBounds( | |
| Rect.fromLTWH( | |
| visiblePosition.dx, | |
| visiblePosition.dy, | |
| visibleSize.width, | |
| visibleSize.height, | |
| ), | |
| ); | |
| } else { | |
| // Fallback: if display information is unavailable, use the default maximize behavior. | |
| await windowManager.maximize(); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| commonPrint.log( | ||
| 'Invalid display dimensions, using native maximize', | ||
| ); | ||
| await windowManager.maximize(); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the fallback to native maximize is used due to invalid display dimensions (line 290), _savedBounds is not cleared. This means the saved bounds from the current window position (lines 244-252) will persist even though the window is now using native maximize. If the user then unmaximizes, the code would attempt to restore these bounds (lines 188-193), which may not be appropriate since native maximize was used. Consider clearing _savedBounds when falling back to native maximize.
| await windowManager.maximize(); | |
| await windowManager.maximize(); | |
| _savedBounds = null; |
| } else { | ||
| // Non-Windows platforms: use standard unmaximize | ||
| await windowManager.unmaximize(); | ||
| _windowsBoundsMaximized = false; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the unmaximize flow on line 238, _windowsBoundsMaximized is set to false for non-Windows platforms, but this state variable should only be managed for Windows. Setting it to false on other platforms is unnecessary and could mask bugs. The state should only be modified when on Windows platforms.
| _windowsBoundsMaximized = false; |
| 'Invalid display dimensions, using native maximize', | ||
| ); | ||
| await windowManager.maximize(); | ||
| _windowsBoundsMaximized = false; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback logic on line 291 sets _windowsBoundsMaximized = false after calling native windowManager.maximize(), which is inconsistent. When using native maximize, the window is still maximized, so isMaximizedNotifier.value on line 292 would be true. However, setting _windowsBoundsMaximized = false means the next call to _updateMaximized() would read isMaximized from windowManager.isMaximized() (line 183) instead of from the state variable. This inconsistency could lead to unexpected behavior in subsequent maximize/unmaximize operations.
| _windowsBoundsMaximized = false; | |
| _windowsBoundsMaximized = true; |
| } else { | ||
| await windowManager.maximize(); | ||
| isMaximizedNotifier.value = await windowManager.isMaximized(); | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's duplicate code in the maximize branch (lines 306-308) and the fallback native maximize branch (lines 290-292). Both blocks call windowManager.maximize() followed by updating isMaximizedNotifier.value with the result of windowManager.isMaximized(). This could be refactored into a single helper method to reduce duplication and improve maintainability.
| } catch (e) { | ||
| // If restoration fails, keep saved bounds and try unmaximize | ||
| commonPrint.log('Failed to restore saved bounds: $e'); | ||
| await windowManager.unmaximize(); | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling for failed bounds restoration on lines 194-197 falls back to windowManager.unmaximize(), but doesn't clear the _windowsBoundsMaximized flag after this fallback. If the unmaximize call succeeds, the window is no longer maximized but _windowsBoundsMaximized remains true. On the next call to _updateMaximized(), it would read isMaximized = true from _windowsBoundsMaximized and attempt to unmaximize again. This should set _windowsBoundsMaximized = false after the fallback unmaximize call.
Co-authored-by: Copilot <[email protected]>
Fix issue #1626
This pull request improves the window maximize and restore functionality, specifically for Windows platforms, by implementing manual maximize logic to better handle window sizing and restoration. The changes introduce state tracking for manual maximization and ensure the window restores to its previous size and position when unmaximized.
Platform-specific window maximize behavior:
_manualMaximizedand_savedBoundsstate variables to track manual maximization and saved window bounds.Dependency updates:
screen_retrieverimport to obtain display information needed for custom maximize logic.