Repository navigation
[macOS] Implement size to content regular and dialog windows. - #185256
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request implements support for windows sized to their content by adding sizedToContent factory constructors to window controllers and introducing a resizable property. The macOS implementation is updated with a FlutterViewContentDelegate to manage content-based resizing. Review feedback highlights a logic error in FlutterWindowController.mm where the window resizing condition on the first frame is inverted, potentially ignoring preferred sizes. A typo was also noted in the source comments.
There was a problem hiding this comment.
Code Review
This pull request adds support for windows sized to their content by introducing a sizedToContent factory for window controllers and a resizable property. The macOS implementation is updated to use a FlutterViewContentDelegate for content-based resizing and an onFirstFrame callback for window visibility. A potential retain cycle was identified in FlutterWindowController.mm where self is strongly captured within a block.
gaaclarke
left a comment
There was a problem hiding this comment.
I looked through everything and it is looking good to me. Maybe we can get loic's review too since he'll probably be more familiar with any potential problems in this code.
| if (self.onFirstFrame) { | ||
| self.onFirstFrame(); | ||
| self.onFirstFrame = nil; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this only in the else branch?
There was a problem hiding this comment.
This is a case where window positioner disagrees with window size. In which case we tighten the constraints and regenerate frame. We don't want to show the window if this happens, it's better to delay showing by one frame (on next frame with new constraints the else branch will happen).
There was a problem hiding this comment.
Working that into the comment will help. This logic is a bit fiddly.
There was a problem hiding this comment.
I updated the logic. When positioner disagrees with the size, new constraints are sent and the method exits early. That removes the onFirstFrame() from the else block. Seems a bit less fiddly to me.
b14d6e3 to
319f23c
Compare
gaaclarke
left a comment
There was a problem hiding this comment.
The code looks good to me and testing looks good. The logic is a bit fiddly. It would be nice if @loic-sharma could give it a look too since he's more familiar with this code and he might have an idea on how to simplify or make it more clear.
| if (self.onFirstFrame) { | ||
| self.onFirstFrame(); | ||
| self.onFirstFrame = nil; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Working that into the comment will help. This logic is a bit fiddly.
…e/FlutterView.h Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>
55105dc to
0e81599
Compare
Implements size to content in both resizable and non-resizable regular and dialog windows.
Delays displaying windows until first render.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.