Skip to content
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

WIP: Smooth window resizing on macOS #21252

Closed
wants to merge 2 commits into from

Conversation

knopp
Copy link
Member

@knopp knopp commented Sep 17, 2020

Description

Superseds #21110

Make window resizing on macOS smooth, responsive and synchronous with windows size.

Related design doc: https://flutter.dev/go/desktop-resize-macos

Notable change: It seems that double buffered IOSurface backed NSOpenGLContext is not all that double buffered after all and I was able to create a test case which resulted in partial drawn frame visible. This version uses single buffered NSOpenGLContext with two IOSurfaces (front and back) which eliminates the problem.

Depends on: #21108

Related Issues

flutter/flutter#44136

Tests

I added the following tests:

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this maks the existing FlutterView class more complex.

I recommend creating new classes in their own files that take over some of the responsibilities from FlutterView.

Maybe we can do something like this:

FlutterIOSurfaceManager

  • Manages the lifecycle of IOSurfaces: Creation, deletion, swap-buffers etc.
  • This will be created in initWithFrame of FlutterView and can also be called when replace existing usages of recreateSurfaceWithScaledSize to recreate surfaces.

FlutterResizer

  • Manages the synchronous resizing. This will hold the only reference to FlutterIOSurfaceManager.
  • Keep track of the front and back-buffer.

The way I see it, FlutterView will hold FlutterResizer, which will then hold FlutterIOSurfaceManager. FrameBuffer related requests in FlutterView will be delegated to the FlutterReizer which will hold context about the current resize lifecycle and respond to requests using FlutterIOSurfaceManager.

This will let us unit-test each of these classes better.

* Listener for view resizing.
*/
@protocol FlutterViewReshapeListener <NSObject>
@protocol FlutterViewDelegate <NSObject>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this FlutterSurfaceDelegate and move present and getFrameBufferIdForWidth to this?

/**
* Called when the view's backing store changes size.
*/
- (void)viewDidReshape:(nonnull NSView*)view;

- (void)scheduleOnRasterTread:(nonnull dispatch_block_t)block;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "RasterThread". Also, given that the embedder method is called FlutterEnginePostRenderThreadTask, maybe we can call this: scheduleOnRenderThread?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. Apart from FlutterEnginePostRenderThreadTask, the thread is referred to as raster thread in most places in the code.

@knopp knopp mentioned this pull request Sep 30, 2020
12 tasks
@knopp
Copy link
Member Author

knopp commented Sep 30, 2020

Replaced by #21525

@knopp knopp closed this Oct 1, 2020
@knopp knopp deleted the macos_smooth_resizing_wip_pr2 branch July 17, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants