Repository navigation
Improve initial size and placement logic for popups and tooltips on Linux - #189994
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces deferred-show and reposition-on-resize behavior for anchored, sized-to-content Linux windows (tooltips and popups) by monitoring view size changes via FFI. The review feedback highlights several important issues to address: potential null pointer exceptions when accessing RendererBinding.instance or native type names, resource leaks from unclosed _viewMonitor and uncancelled _showFallbackTimer instances, and a lack of exception safety when updating window positions that could permanently lock the repositioning state.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. |
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
robert-ancell
left a comment
There was a problem hiding this comment.
Tested here and works, thanks!
|
CI failure was an infra issue, re-running. This might need a rebase to merge as well. |
|
@knopp // ignore_for_file: invalid_use_of_internal_member
// ignore_for_file: implementation_imports
import 'package:flutter/material.dart';
import 'package:flutter/src/widgets/_window.dart';
import 'package:flutter/src/widgets/_window_positioner.dart';
void main() {
WidgetsFlutterBinding.ensureInitialized();
runWidget(const WindowingApp());
}
final ThemeData _appTheme = ThemeData(
colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
);
class WindowingApp extends StatefulWidget {
const WindowingApp({super.key});
@override
State<WindowingApp> createState() => _WindowingAppState();
}
class _CallbackPopupDelegate extends PopupWindowControllerDelegate {
_CallbackPopupDelegate({required this.onDestroyCallback});
final VoidCallback onDestroyCallback;
@override
void onWindowDestroyed() {
onDestroyCallback();
}
}
class _WindowingAppState extends State<WindowingApp> {
final WindowController _windowController = WindowController(
size: Size(800, 600),
title: "Tester",
);
PopupWindowController? _popupWindowController;
void _togglePopup(Rect anchorRect) {
if (_popupWindowController != null) {
_popupWindowController!.destroy();
return;
}
setState(() {
_popupWindowController = PopupWindowController(
parent: _windowController,
anchorRect: anchorRect,
positioner: const WindowPositioner(
parentAnchor: .topRight,
childAnchor: .topLeft,
constraintAdjustment: WindowPositionerConstraintAdjustment(
flipY: true,
slideX: true,
slideY: true,
resizeY: true,
),
),
delegate: _CallbackPopupDelegate(
onDestroyCallback: () {
setState(() {
_popupWindowController = null;
});
},
),
);
});
}
@override
Widget build(BuildContext context) {
return ViewCollection(
views: [
Window(
controller: _windowController,
child: MyApp(onTogglePopup: _togglePopup),
),
if (_popupWindowController != null)
PopupWindow(
controller: _popupWindowController!,
child: SizedBox(
width: 400,
height: 200,
child: MaterialApp(
debugShowCheckedModeBanner: false,
theme: _appTheme,
home: Material(
color: _appTheme.colorScheme.surfaceContainer,
child: ListView.builder(
shrinkWrap: true,
itemCount: 100,
itemBuilder: (BuildContext context, int index) {
return ListTile(
dense: true,
title: Text('Item $index'),
onTap: () => _popupWindowController?.destroy(),
);
},
),
),
),
),
),
],
);
}
}
class MyApp extends StatelessWidget {
const MyApp({super.key, required this.onTogglePopup});
final ValueChanged<Rect> onTogglePopup;
// This widget is the root of your application.
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
theme: _appTheme,
home: MyHomePage(
title: 'Flutter Demo Home Page',
onTogglePopup: onTogglePopup,
),
);
}
}
class MyHomePage extends StatefulWidget {
const MyHomePage({
super.key,
required this.title,
required this.onTogglePopup,
});
final String title;
final ValueChanged<Rect> onTogglePopup;
@override
State<MyHomePage> createState() => _MyHomePageState();
}
class _MyHomePageState extends State<MyHomePage> {
final GlobalKey _fabKey = GlobalKey();
void _onPressed() {
final Rect? anchorRect = _getAnchorRect();
if (anchorRect == null) {
return;
}
widget.onTogglePopup(anchorRect);
}
Rect? _getAnchorRect() {
final RenderBox? renderBox =
_fabKey.currentContext?.findRenderObject() as RenderBox?;
if (renderBox == null) {
return null;
}
final Rect rect = renderBox.localToGlobal(Offset.zero) & renderBox.size;
return rect;
}
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
backgroundColor: Theme.of(context).colorScheme.inversePrimary,
title: Text(widget.title),
),
body: Center(
child: Column(
mainAxisAlignment: .center,
children: [const Text('Click the button to show the popup')],
),
),
floatingActionButton: FloatingActionButton(
key: _fabKey,
onPressed: _onPressed,
tooltip: 'Show Popup',
child: const Icon(Icons.add),
),
);
}
}
But replacing the PopupWindow(
controller: _popupWindowController!,
child: Container(
child: MaterialApp(
debugShowCheckedModeBanner: false,
theme: _appTheme,
home: Material(
color: _appTheme.colorScheme.surfaceContainer,
child: ListView.builder(
shrinkWrap: true,
itemCount: 100,
itemBuilder: (BuildContext context, int index) {
return ListTile(
dense: true,
title: Text('Item $index'),
onTap: () => _popupWindowController?.destroy(),
);
},
),
),
),
),
) |
|
The problem in this case is that MaterialApp can never size itself to content. It is because it has Navigator, which has Overlay, which has unfortunate property where it always sizes to constraints.biggest. Which is likely bigger than what compositor is happy with. Try removing the Material app, build your own scaffold of widgets that are needed for popup content. If you need Overlay in there, make sure to set |
…ips on Linux (flutter#189994)" This reverts commit f6061d0.
……ips on Linux (flutter#189994) (flutter#191498) This reverts commit f6061d0. Reverts flutter#189994 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md Co-authored-by: Robert Ancell <[email protected]>
Why?
The initial sizing of popup and tooltip windows on Linux was broken such that windows that calculated their initial size too slowly would have their content overflowing off the edge of the screen. Because they never reposition themselves, this content would be lost forever.
This PR fixes that initial sizing such that we wait for the convergence of initial sizes between the embedder and the framework before showing the popup/tooltip.
What's new?
on_size_changedcallback to theFlViewMonitorthat notifies us when the view's size changesThis fix consistently manages to place my windows correctly, which is very awesome.
How it works
Pre-launch Checklist
///).