-
Notifications
You must be signed in to change notification settings - Fork 6k
Add support for setting window size limits for glfw #13415
Conversation
a177b75 to
cd26112
Compare
|
Sorry, I know I said this was fine in offline discussion, but after thinking about it a bit more carefully this should actually be a stand-alone function, not part of the window creation, because unlike Also, it seems like this is the right time to add a simple |
ece0b44 to
191c321
Compare
|
This is updated to add a function which can be passed a window and then delegates to the glfw function. I will add the size struct and use it in this function. It would be a breaking change to use it everywhere we are using width/height, so not sure if you want to go that far or not. |
stuartmorgan-g
left a comment
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.
Thanks for restructuring that; sorry again for giving bad advice originally.
|
I think I lost a comment: This API is explicitly unstable so that we can improve it without worrying about compatibility, so breaking changes are fine. But it's also fine if you want to just do the addition of size in this PR, and then I can do the breaking change to existing APIs later. |
191c321 to
7fe16d6
Compare
|
The test failures really seem unrelated but I am not 100% sure. I updated to use a size struct and I defined a constant that hides the GLFW_DONT_CARE constant. |
stuartmorgan-g
left a comment
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.
Just comment nits, except that it seems odd to not plumb this through to the C++ wrapper that I expect most clients will use. Could you add that in this PR?
This should also be added to the test stub when you do that, along with a simple test that the wrapper calls through as expected.
bb122b0 to
b075c04
Compare
b075c04 to
e70872b
Compare
stuartmorgan-g
left a comment
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.
LGTM!
| @@ -0,0 +1,57 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
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.
Thanks for adding this file! I forgot this hadn't been done for the window class yet; desktop embedding testing is not in a good state yet obviously, so getting more of the initial test files set up is great.
| // Sets the min/max size of |flutter_window| in screen coordinates. Use | ||
| // kFlutterDesktopDontCare for any dimension you wish to leave unconstrained. | ||
| void SetSizeLimits(FlutterDesktopSize minimum_size, | ||
| FlutterDesktopSize maximum_size) { |
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.
We may want to use different structs/constants rather than relying on the wrapped API's types, but that's fine to revisit later.
Add a function to the window which calls the glfw function for fixing the size limits of the window. This can then be called after window creation.
e70872b to
7737691
Compare
|
Apologies, I didn't realize that this hadn't ever been landed. Merging now. |
flutter/engine@e7b69ce...5b870a2 git log e7b69ce..5b870a2 --first-parent --oneline 2019-12-08 wvvwwvw@gmail.com Add support for setting window size limits for glfw (flutter/engine#13415) 2019-12-06 iska.kaushik@gmail.com [fuchsia] SnapToNextPhase refactor + add tests and documentation (flutter/engine#14158) 2019-12-06 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from VKso5... to 9C6UA... (flutter/engine#14161) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Add a function to the window which calls the glfw function for fixing the size limits of the window. This can then be called after window creation.
Add properties for speciying min/max width/height.
Pass those properties through to the window constructor.
Call glfwSetWindowSizeLimits with the values from those properties or
GLFW_DONT_CARE if they are set to zero which is the default implicit
initial value for integers.
glfwSetWindowSizeLimits must be called from the main thread, and I am
pretty sure we are on it given that we call other functions inside that
same function that must also only be called from the main thread.
I went with a nested struct rather than putting these at the same level as the other properties because they are logically related, but that is maybe a question of preference. I'd be happy to restructure that.