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

Conversation

@knopp
Copy link
Member

@knopp knopp commented Aug 15, 2022

Fixes: flutter/flutter#92673

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@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 (don't just cc him here, he won't see it! He's on Discord!).

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.

@chinmaygarde
Copy link
Member

cc @cbracken

@a-wallen
Copy link
Contributor

I'm not sure that this PR fixes flutter/flutter#92673 because flutter/flutter#92673 has two problems.

  • The red box to immediately be replaced with nothing (a grey background)
  • Upon resize, the window to be able to be resized, still just showing a grey background.

At the time of writing, this PR seems to solve one. I do think that we want this change, but I'm not sure if we can keep

Fixes: flutter/flutter#92673

in the PR description without solving both issues mentioned in flutter/flutter#92673.

@chinmaygarde
Copy link
Member

Ping @cbracken @a-wallen

@cbracken
Copy link
Member

We'll need to author a test for this. I'll give it a shot.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

@a-wallen is writing a test for this. The fix itself lgtm; we can land this once his test has been added.

The red box to immediately be replaced with nothing (a grey background)

Covered by this separate bug: flutter/flutter#76082

@a-wallen
Copy link
Contributor

a-wallen commented Sep 19, 2022

Writing a unit test for this PR in the engine would be easiest via embedding this flutter application

import 'package:flutter/widgets.dart';

void main() async {
  runApp(const Container(color: Color(0xffff0000)));
  runApp(const Container());
}

in a custom entry point for the embedder to consume. However, the engine cannot depend on the framework since the framework depends on the engine. There are two options to test this PR:

  1. use dart:ui to emulate the behavior in the above program in a custom dart entrypoint.
  2. write integration test in the framework which runs the example above and performs resize in an XCUITest.

@a-wallen
Copy link
Contributor

The PR above should address the tests for this PR.

@Hixie
Copy link
Contributor

Hixie commented Sep 21, 2022

test-exempt: test is separate

@chinmaygarde
Copy link
Member

@knopp May I land this please? Everything looks good to go.

@knopp knopp merged commit afd565e into flutter:main Sep 22, 2022
@knopp knopp deleted the fix_hang_with_no_content branch September 22, 2022 20:45
@a-wallen
Copy link
Contributor

FYI @chinmaygarde, the test for this isn't done.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 23, 2022
Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
…nt (flutter#35409)

* [macOS] Do not block main thread during resizing if there is no content

Fixes: flutter/flutter#92673
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[macOS] Flutter process appears to hang

5 participants