-
Notifications
You must be signed in to change notification settings - Fork 6k
Conversation
Based on a patch by jane400
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. 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. |
cc @jane400 |
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.
Overall this looks great; @loic-sharma or @goderbauer should probably give the final lgtm on this, API-wise. I'm assuming this will be followed by an update to the templates in the tool?
I'm almost ashamed to admit it, but I'm growing fonder of GTK with time.
Yes, there will be a follow up PR to change the template to use this. However, since we're changing the template I figured we should also do flutter/flutter#153812. I'm also going to have more of a play with the multi-window changes to FlApplication first so we don't end up changing the template more than we have to. |
Hello, thanks for sending this excellent patch!
We were considering doing something very similar as part of the multi-window effort to clean up macOS's and Windows's runner APIs, especially since these runner APIs are very inconsistent across all our platforms. However, I was concerned that the multi-window effort would require large amounts of API churn. We decided to punt the macOS/Windows runner API clean up until after the dust settles from the multi-window effort. This would let us have a stronger understanding of multi-window's requirements for each platform. In the meantime, we retrofitted existing APIs to add multi-window capabilities. See: https://flutter.dev/go/desktop-multi-view-runner-apis I worry that this is too early to revamp Linux's runner APIs, and I worry that this makes Linux's runner APIs less consistent with macOS and Windows. Sorry for being a downer. I'm not necessarily against this change, I just wanted to make sure I aired my concerns. @goderbauer what are your thoughts on this? |
Co-authored-by: Loïc Sharma <[email protected]>
The initial idea behind making FlApplication is to take pretty much all of the code out of the template and make it internal to FlApplication. I would expect most Linux Flutter applications would not need any modification beyond that (if we can move the window size and title into Dart code then there's really nothing the app is doing). All the new multi-window behaviour is then internal to FlApplication. Do you think macOS / Windows would end up in the same place or is that less possible on those platforms? Initially I thought we could just leave FlApplication in the embedder and wait for updating the template, but #54703 is the sort of change that would make use of it. I'm fairly confident that if we switched the template now we're not going to paint ourselves into a corner with future changes given how little the app is doing. |
…154569) flutter/engine@a3504c2...5373431 2024-09-03 [email protected] Roll Skia from ab2317b94853 to 2d5a75027691 (3 revisions) (flutter/engine#54940) 2024-09-03 [email protected] Roll Skia from 041fd378d332 to ab2317b94853 (3 revisions) (flutter/engine#54937) 2024-09-03 49699333+dependabot[bot]@users.noreply.github.com Bump actions/setup-python from 5.1.1 to 5.2.0 (flutter/engine#54933) 2024-09-03 [email protected] Roll Skia from 49ea0f383706 to 041fd378d332 (1 revision) (flutter/engine#54932) 2024-09-03 [email protected] Make FlApplication class (flutter/engine#54637) 2024-09-03 [email protected] Roll Fuchsia Linux SDK from ksNJVM5ZoBB74rba_... to BaO2fyTu4jhvdTtdE... (flutter/engine#54931) 2024-09-02 [email protected] Roll Skia from 3d0c9bf48176 to 49ea0f383706 (1 revision) (flutter/engine#54928) 2024-09-02 [email protected] Roll Skia from 03bdb5c60304 to 3d0c9bf48176 (1 revision) (flutter/engine#54925) 2024-09-02 [email protected] Roll Skia from c873eb5f38d5 to 03bdb5c60304 (1 revision) (flutter/engine#54924) 2024-09-02 [email protected] Roll Skia from 514feab300b4 to c873eb5f38d5 (1 revision) (flutter/engine#54923) 2024-09-02 [email protected] Roll Skia from 15641c0df7e8 to 514feab300b4 (1 revision) (flutter/engine#54922) 2024-09-01 [email protected] Roll Skia from 80f2cd706443 to 15641c0df7e8 (1 revision) (flutter/engine#54919) 2024-09-01 [email protected] Roll Fuchsia Linux SDK from uDdFN_cZC6F9BVTzz... to ksNJVM5ZoBB74rba_... (flutter/engine#54918) 2024-09-01 [email protected] Roll Skia from 5477dbb533bb to 80f2cd706443 (1 revision) (flutter/engine#54916) 2024-09-01 [email protected] Roll Skia from 95ef9caae482 to 5477dbb533bb (1 revision) (flutter/engine#54915) 2024-08-31 [email protected] Roll Fuchsia Linux SDK from JKPo9G1NaAdstrimW... to uDdFN_cZC6F9BVTzz... (flutter/engine#54914) 2024-08-31 [email protected] [Impeller] separate algorithm for computing render target size. (flutter/engine#54604) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from JKPo9G1NaAds to BaO2fyTu4jhv 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 [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Move all the current behaviour of the Linux template into FlApplication. An app can now be:
With this simplified, we can now build multi-window behaviour without having to modify the template much in the future.
This change is similar to what was proposed in #50868, but instead it only contains the current behaviour that we can build off.
Fixes flutter/flutter#142920