Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@rokob
Copy link
Contributor

@rokob rokob commented Oct 29, 2019

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.

@stuartmorgan-g
Copy link
Contributor

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 resizable this isn't something that is fixed at window creation time. Native code (e.g., a plugin to control window behavior) should be able to change this at any time. So the C API and the Window wrapper should instead get a new method that calls through to the GLFW function, like is done with size. (Unlike size we don't also need it at construction time so it doesn't need to be in the creation info as well.)

Also, it seems like this is the right time to add a simple size struct since there are now a bunch more width/height pairs.

@rokob rokob force-pushed the set-window-size branch 2 times, most recently from ece0b44 to 191c321 Compare November 14, 2019 20:05
@rokob
Copy link
Contributor Author

rokob commented Nov 14, 2019

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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a 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.

@stuartmorgan-g
Copy link
Contributor

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.

@rokob
Copy link
Contributor Author

rokob commented Nov 15, 2019

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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a 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.

@rokob rokob force-pushed the set-window-size branch 9 times, most recently from bb122b0 to b075c04 Compare November 20, 2019 18:06
@rokob rokob requested a review from stuartmorgan-g November 20, 2019 18:35
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a 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.
Copy link
Contributor

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) {
Copy link
Contributor

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.
@stuartmorgan-g
Copy link
Contributor

Apologies, I didn't realize that this hadn't ever been landed. Merging now.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 8, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 8, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Dec 9, 2019
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
filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants