Add disable zoom on mac #3289
Add disable zoom on mac #3289
Conversation
|
Warning Rate Limit Exceeded@wizzymore has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 27 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent update introduces a feature allowing the disabling of the zoom button on macOS applications without affecting the window's resize capability. This enhancement is implemented through a new Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (7)
- v2/internal/frontend/desktop/darwin/Application.h (1 hunks)
- v2/internal/frontend/desktop/darwin/Application.m (2 hunks)
- v2/internal/frontend/desktop/darwin/WailsContext.h (1 hunks)
- v2/internal/frontend/desktop/darwin/WailsContext.m (3 hunks)
- v2/internal/frontend/desktop/darwin/window.go (2 hunks)
- v2/pkg/options/mac/mac.go (1 hunks)
- website/versioned_docs/version-v2.8.0/reference/options.mdx (2 hunks)
Additional comments: 4
v2/pkg/options/mac/mac.go (1)
- 24-24: The addition of the
DisableZoomboolean field to theOptionsstruct is clear and aligns with the PR's objectives. Ensure that all usages of theOptionsstruct across the codebase are updated to consider this new field where necessary.v2/internal/frontend/desktop/darwin/WailsContext.h (1)
- 67-67: The update to the
CreateWindowmethod to include azoomableparameter is consistent with the PR's objectives. Ensure that the implementation of this method and all calls to it correctly handle the new parameter.Verification successful
To ensure a thorough verification, let's broaden the search to include all file types and adjust the search pattern to capture potential variations in how the
CreateWindowmethod might be called or referenced. This approach will help us gather more information about the integration of the newzoomableparameter.
The verification process has confirmed that the
zoomableparameter has been correctly integrated into theCreateWindowmethod for the Darwin platform, as evidenced by the method signature inWailsContext.h, its implementation inWailsContext.m, and the call inApplication.m. This aligns with the objectives of providing more granular control over window zoom capabilities on macOS.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to the CreateWindow method to ensure the new parameter is correctly used. rg "CreateWindow\(" --type objcLength of output: 33
Script:
#!/bin/bash # Broaden the search to include all file types and adjust the pattern to capture potential variations in method calls or references. rg "CreateWindow"Length of output: 4772
v2/internal/frontend/desktop/darwin/window.go (1)
- 111-112: The logic to control the
zoomablevariable based on theDisableZoomoption is correctly implemented. Ensure thorough testing of this functionality to confirm it behaves as expected under various configurations.v2/internal/frontend/desktop/darwin/Application.m (1)
- 17-17: The inclusion of the
zoomableparameter in theCreatefunction and its correct usage in the call toCreateWindowaligns with the PR's objectives. Verify the correct handling of this parameter throughout the codebase to ensure it functions as intended.Verification successful
Given the lack of results from the initial search, let's broaden our approach to find how the
zoomableparameter is handled in the codebase. We'll search for all occurrences of thezoomableparameter in Objective-C files.
The
zoomableparameter is correctly included in theCreateWindowmethod signatures and is actively used in the window creation logic, as evidenced by its presence in both the header and implementation files (WailsContext.handWailsContext.m), and its correct passing in theCreatefunction withinApplication.m. This confirms that the changes related to thezoomableparameter have been correctly implemented across the relevant parts of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of CreateWindow to ensure the zoomable parameter is correctly handled. rg "CreateWindow\(" --type objcLength of output: 33
Script:
#!/bin/bash # Search for all occurrences of the "zoomable" parameter in Objective-C files. rg "zoomable" --type objcLength of output: 3015
| @@ -109,6 +109,7 @@ func main() { | |||
| Appearance: mac.NSAppearanceNameDarkAqua, | |||
| WebviewIsTransparent: true, | |||
| WindowIsTranslucent: false, | |||
| DisableZoom: false, | |||
There was a problem hiding this comment.
The addition of the DisableZoom option to the mac.Options struct is correctly documented. However, it would be beneficial to include a brief example of how to set this option in code, similar to other options documented in this file. This would provide developers with a clear, practical example of how to use the new feature.
Consider adding a code snippet demonstrating how to set the DisableZoom option to true or false within the mac.Options struct.
| [self.mainWindow setAppearance:nsAppearance]; | ||
| } | ||
|
|
||
| if (!zoomable && resizable) { | ||
| NSButton *button = [self.mainWindow standardWindowButton:NSWindowZoomButton]; | ||
| [button setEnabled: NO]; | ||
| } | ||
|
|
||
| NSSize minSize = { minWidth, minHeight }; | ||
| NSSize maxSize = { maxWidth, maxHeight }; |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [139-193]
The introduction of the zoomable parameter and the associated logic to disable the zoom button if zoomable is false is a well-implemented feature that directly addresses the PR objectives. However, there are a few considerations and potential improvements:
-
Parameter Documentation: It's important to document the new
zoomableparameter in the method's header comment (if applicable) to ensure that other developers understand its purpose and how it affects window behavior. -
Consistency with macOS Guidelines: Disabling the zoom button while allowing resizing is unconventional in macOS applications. Ensure this behavior aligns with macOS Human Interface Guidelines and consider how it might affect user expectations.
-
Testing: The PR description mentions that unit tests for this feature have not been added due to questions about testability. While testing UI behavior can be challenging, consider exploring UI testing frameworks like XCTest for macOS that support automated UI tests. This could help ensure the feature works as expected across different macOS versions and configurations.
-
Fallback for Non-resizable Windows: The logic correctly disables the zoom button only if the window is resizable. Consider adding a comment or documentation noting that this feature is intended for use with resizable windows, as the behavior might be unclear if
zoomableis set to false for a non-resizable window.
Overall, the changes are aligned with the PR objectives and are a valuable addition to the Wails framework for macOS developers. Just ensure to address the considerations mentioned for documentation, testing, and adherence to macOS guidelines.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- website/src/pages/changelog.mdx (1 hunks)
Additional comments: 1
website/src/pages/changelog.mdx (1)
- 18-18: The addition of the
DisableZoomoption for macOS applications is clearly documented in the changelog. It's important to ensure that all relevant details about the new feature are included, such as its impact on existing functionality and any necessary steps for developers to utilize it. The current description seems concise and follows the established format of the changelog, which is good for maintaining consistency.
I see this file is ignore, but i think it's better to keep it up-to-date so if we ever unignore it will work correctly.
Description
Add a new boolean inside
mac.OptionscalledDisableZoom, this setting will disable the green zoom button when the window is resizable.Currently you can only disable the green zoom button by setting the window as "not resizable" and this is unwanted behaviour, prior to this PR you can't disable the button without disabling resizing.
I could add a test, but i don't know if this change is testable or if it is, how to test it.
Fixes #3288
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I tested the application by running all the possible configurations:
DisableResize: true&Frameless: false&DisableZoom: true( does nothing new )DisableResize: true&Frameless: true&DisableZoom: true( does nothing new )DisableResize: false&Frameless: false&DisableZoom: true( the button is disabled )DisableResize: false&Frameless: true&DisableZoom: true( does nothing new )Windows
macOS
Linux
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
zoomableoption, replacing fullscreen.DisableZoomoption to control window zoom on Mac.