Re-land: Add support for Tooltip hover#31699
Conversation
gspencergoog
force-pushed
the
fix_tooltip_hover
branch
2 times, most recently
from
2b02910 to
956c577
Compare
gspencergoog
force-pushed
the
fix_tooltip_hover
branch
from
956c577 to
6de810d
Compare
goderbauer
added
f: material design
gspencergoog
force-pushed
the
fix_tooltip_hover
branch
5 times, most recently
from
c2b5a00 to
3ba2d9b
Compare
|
OK, @HansMuller, this is ready for another round. I've updated this with the code to not even wrap the Listener widget in the tooltip unless there is a mouse connected, and also changed the Listener to not add the layer unless a mouse is connected. |
gspencergoog
force-pushed
the
fix_tooltip_hover
branch
3 times, most recently
from
ea3b74a to
c80d1b2
Compare
This is a re-land of flutter#31561, after fixing performance regressions.
gspencergoog
force-pushed
the
fix_tooltip_hover
branch
from
c80d1b2 to
e935f49
Compare
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
LGTM. Just some random comments to prove that I read through all of it.
| /// {@macro flutter.widgets.child} | ||
| final Widget child; | ||
|
|
||
| /// Specifies the decoration of the tooltip window. |
There was a problem hiding this comment.
The tooltip's shape and background color?
If I'm not mistaken, there's no window here.
There was a problem hiding this comment.
It was in the everythings-a-window sense. :-)
No, no window. Fixed.
| /// Specifies the decoration of the tooltip window. | ||
| /// | ||
| /// If not specified, defaults to a rounded rectangle with a border radius of | ||
| /// 4.0, and a color derived from the text theme. |
There was a problem hiding this comment.
There are three text themes in ThemeData: textTheme, primaryTextTheme, accentTextTheme. Assuming that we're referring to the first one, then maybe mention [ThemeData.textTheme].
There was a problem hiding this comment.
Well, it's theme.textTheme or theme.primaryTextTheme, depending upon whether it's a dark theme or not. I didn't change the logic, and I wanted to say loosely what it was based on, but without repeating the conditionals. Probably better to just be explicit, though.
| kind: kind, | ||
| pointer: pointer ?? _getNextPointer(), | ||
| ); | ||
| await gesture.addPointer(); |
There was a problem hiding this comment.
This seems like a non-trivial change. Should the doc for the method reflect it?
There was a problem hiding this comment.
Actually, I removed it and moved it into the test, since I think that the createGesture should probably not assume anything about the events a test might need. I may introduce a startHover method that will do it, but for now I'll just put the addPointer into the test.
| /// In a test, send a move event that moves the pointer by the given offset. | ||
| @visibleForTesting | ||
| Future<void> updateWithCustomEvent(PointerEvent event, { Duration timeStamp = Duration.zero }) { | ||
| Future<void> updateWithCustomEvent(PointerEvent event, {Duration timeStamp = Duration.zero}) { |
There was a problem hiding this comment.
{ Duration timeStamp = Duration.zero } I think I mentioned this earlier. Flutter sources generally include the spaces before named parameters.
There was a problem hiding this comment.
Yeah, this is dartfmt really not helping me. Fixed everywhere in this file.
4 participants
Description
This is a re-land of #31561, after fixing performance regressions.
Added change listening to the MouseTracker so that the Listener and tooltip can react to whether or not a mouse is connected at all. Added a change check to make sure
Listeneronly repaints when something changed.Here's a gist with the changes since the original change.
Fixes #22817