Implement multi-window support for RawTooltip#184401
Conversation
github-actions
Bot
added
framework
robert-ancell
left a comment
robert-ancell
left a comment
There was a problem hiding this comment.
Some code style and documentation suggestions but otherwise looks good.
|
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces multi-window tooltip support by abstracting display logic into a controller interface, allowing tooltips to render in overlays or separate windows. It adds preferBelow and verticalOffset properties to RawTooltip and Tooltip for improved positioning. A review comment suggests adding a mounted check before calling setState in a callback to avoid potential runtime errors.
| parent: windowController, | ||
| anchorRectGetter: _getAnchorRect, | ||
| positionerBuilder: _buildWindowPositioner, | ||
| onChanged: () => setState(() {}), |
There was a problem hiding this comment.
Calling setState in an asynchronous callback like onChanged should be guarded by a mounted check to avoid errors if the widget is removed from the tree before the callback is executed.
onChanged: () { if (mounted) { setState(() {}); } },There was a problem hiding this comment.
Maybe not necessary because _showController is disposed in dispose?
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
This is super exciting. The approach looks good to me. It avoids the post-decoupling problem of importing windowing stuff into Material that we found in #181861 thanks to the raw widget architecture.
CC @victorsanni as the author of RawTooltip
| parent: windowController, | ||
| anchorRectGetter: _getAnchorRect, | ||
| positionerBuilder: _buildWindowPositioner, | ||
| onChanged: () => setState(() {}), |
There was a problem hiding this comment.
Maybe not necessary because _showController is disposed in dispose?
github-actions
Bot
removed
d: api docs
| /// direction, the tooltip will be displayed in the opposite direction. | ||
| /// | ||
| /// Defaults to true. | ||
| /// {@endtemplate} |
There was a problem hiding this comment.
FYI, it looks like nothing is using this template currently
| void dispose(); | ||
| } | ||
|
|
||
| /// Shows the tooltip using an [OverlayPortalController] (the non-windowing path). |
There was a problem hiding this comment.
"the non-windowing path" might not be obvious to a reader, consider clarifying this means the tooltip is within the current window:
| /// Shows the tooltip using an [OverlayPortalController] (the non-windowing path). | |
| /// Shows the tooltip within the current window using an [OverlayPortalController]. |
| /// Shows the tooltip using a [TooltipWindowController] and [WindowRegistry] | ||
| /// (the windowing path). |
There was a problem hiding this comment.
Likewise "the windowing path" might not be obvious to a reader, consider explaining this creates a child window:
| /// Shows the tooltip using a [TooltipWindowController] and [WindowRegistry] | |
| /// (the windowing path). | |
| /// Shows the tooltip in a child window using a [TooltipWindowController] | |
| /// and the [WindowRegistry]. |
| @override | ||
| void didChangeDependencies() { | ||
| super.didChangeDependencies(); | ||
| if (_showControllerInitialized) { |
There was a problem hiding this comment.
I believe if a Tooltip has a GlobalKey, it can be reparented from one window to another. I suspect we need to handle this case by recreating the show controller.
| switch (_showController) { | ||
| case final _OverlayTooltipShowController overlayTooltipShowController: | ||
| assert(debugCheckHasOverlay(context)); | ||
| return OverlayPortal.overlayChildLayoutBuilder( | ||
| controller: overlayTooltipShowController.overlayController, | ||
| overlayChildBuilder: _buildTooltipOverlay, | ||
| child: result, | ||
| ); | ||
| case final _WindowTooltipShowController windowTooltipShowController: | ||
| return ViewAnchor( | ||
| view: TooltipWindow( | ||
| controller: windowTooltipShowController.windowController!, | ||
| child: widget.tooltipBuilder(context, _overlayAnimation), | ||
| ), | ||
| child: result, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Would it make sense to add a build method to _TooltipShowController? This would likely require being able to to move _buildTooltipOverlay down to _OverlayTooltipShowController. I don't feel strongly about this, just a thought in case it makes the code nicer.
loic-sharma
left a comment
loic-sharma
left a comment
There was a problem hiding this comment.
Looks good! Love the ASCII art in tests :)
Consider getting a review from @victorsanni as well as they are our RawTooltip expert :)
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
Why are we exposing preferBelow and verticalOffset? @chunhtai, @dkwingsmt and I had detailed conversations about why we shouldn't:
TLDR we left them out of RawTooltip because they are Material-opinionated properties, and there are ways to work around them (i.e RawTooltip.positionDelegate). We also have logic that resolves them in Tooltip so that they don't have to be passed down to RawTooltip (see Tooltip._getDefaultPositionDelegate).
| /// If there is insufficient space to display the tooltip in the preferred | ||
| /// direction, the tooltip will be displayed in the opposite direction. | ||
| /// | ||
| /// Defaults to true. |
There was a problem hiding this comment.
Can we keep the default outside the template so it can be reused in Tooltip.preferBelow?
| /// will position themselves above or below their corresponding widgets | ||
| /// depending on the value of [preferBelow] | ||
| /// | ||
| /// Defaults to 0.0. |
There was a problem hiding this comment.
Same here, Tooltip.verticalOffset defaults to the value set in the theme. If that is not provided, it defaults to 24.0. Removing the default from the template allows it to be reused in Tooltip.verticalOffset where defaults can be explained.
There was a problem hiding this comment.
Also why default to 0.0 instead of 24.0 as in Tooltip? 0.0 might cause visual overlap with the child. Tooltip uses 24.0 as a buffer for example.
| /// When [preferBelow] is set to true and tooltips have sufficient space to | ||
| /// display themselves, this property defines how much vertical space tooltips | ||
| /// will position themselves above or below their corresponding widgets | ||
| /// depending on the value of [preferBelow] |
There was a problem hiding this comment.
| /// depending on the value of [preferBelow] | |
| /// depending on the value of [preferBelow]. |
| /// When [preferBelow] is set to true and tooltips have sufficient space to | ||
| /// display themselves, this property defines how much vertical space tooltips | ||
| /// will position themselves above or below their corresponding widgets | ||
| /// depending on the value of [preferBelow] |
There was a problem hiding this comment.
This paragraph looks incoherent to me, seems like two separate sentences were merged at an arbitrary position.
Understood! I gave this a try this morning, and it isn't really possible with how the windowing API currently works. Rather than working with absolute positions and offsets, the windowing API expects to be given some semantic information about the placement of the window (e.g. its placement gravity). At runtime, the platform is able to reason about the desired behavior and do something smart in the event that it cannot perform it (e.g. if the tooltip was going to go off of the monitor, the platform will flip it to the other side in order to keep it on screen). Because of this, it makes sense to have some semantic information piped down to the RawTooltip, instead of just a resolver for a raw "offset". On top of this, we don't yet have a way for the window to provide its size before being shown, but that is a minor point that can certainly be added on our end if need be :) Let me know if you have any ideas on a path forward here. I attempted to "infer" the WindowPositioner _buildWindowPositioner() {
final TooltipPositionDelegate? delegate = widget.positionDelegate;
if (delegate == null) {
// Default: position below the target, matching the overlay path default.
return const WindowPositioner(
parentAnchor: WindowPositionerAnchor.bottom,
childAnchor: WindowPositionerAnchor.top,
);
}
// Probe the delegate with the real target rect and a zero-sized tooltip.
// The returned point is where the tooltip's top-left corner would be
// placed, which with Size.zero is effectively the anchor point.
// The offset from the target center to that point becomes the
// WindowPositioner offset with center/center anchors.
final Rect? anchorRect = _getAnchorRect();
if (anchorRect == null) {
return const WindowPositioner(
parentAnchor: WindowPositionerAnchor.bottom,
childAnchor: WindowPositionerAnchor.top,
);
}
final Offset result = delegate(
TooltipPositionContext(
target: anchorRect.center,
targetSize: anchorRect.size,
tooltipSize: Size.zero,
verticalOffset: 0.0,
),
);
return WindowPositioner(offset: result - anchorRect.center);
} |
|
Thanks for the reply. My understanding of your explanation (and this PR) is:
From this understanding, my guess is exposing The underlying /// Position a child box within a container box, either above or below a target
/// point.
...
Offset positionDependentBox({My hunch is windowing needs will be more complicated than Maybe the solution here instead is a new top-level property similar to |
I think that is an excellent idea! I can propose one in this PR, and I will re-request you when it's done. I need to think about what will work well across both use cases first. |
After reading through the thread here that's my best bet for how to resolve this too. Something like a windowPositioner parameter (or function that builds a WindowPositioner). It looks to me like if the user passes a WindowPositioner, then we can use that to produce an Offset similar to what's returned from positionDelegate. |
5 participants
What's new?
RawTooltipto show a true window in a tooltip when one is availablepreferBelowandverticalOffsetas parameters toRawTooltip_TooltipShowControllerclass so that the tooltip can decide between showing itself in an overlay vs a true windowtooltip.4.dartexample that usesrunWidgetwith aRegularWindowControllerso that the tooltip example can have a parentHow to test
Run:
The downside of this example is that the implicit view window opens (because the runner opens it).
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot 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.