New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more RenderEditable test coverage #27003
Conversation
@fkorotkov: if you're still collecting data about sudden test terminations, I ran into a few instances here: |
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.
LGTM modulo some indentation nits.
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.
LGTM
051507e
to
b9cb9e3
Compare
Some wild indents snuck into the copypasta. Fixed |
0189c69
to
8d0181c
Compare
Looks like Cirrus is not happy with this PR. |
@@ -1516,7 +1516,7 @@ class RenderEditable extends RenderBox { | |||
_textPainter.paint(context.canvas, effectiveOffset); | |||
|
|||
if (_selection != null && !_floatingCursorOn) { | |||
if (_selection.isCollapsed && cursorColor != null && _hasFocus) { |
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.
reverted this as discussed.
@@ -565,7 +565,8 @@ class EditableText extends StatefulWidget { | |||
/// State for a [EditableText]. | |||
class EditableTextState extends State<EditableText> with AutomaticKeepAliveClientMixin<EditableText>, WidgetsBindingObserver, TickerProviderStateMixin<EditableText> implements TextInputClient, TextSelectionDelegate { | |||
Timer _cursorTimer; | |||
final ValueNotifier<bool> _showCursor = ValueNotifier<bool>(false); |
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.
@jslavitz I refactored this since the new _showCursor is not a source of truth (which is ready to be fed into RenderEditable) but a desired destination. Split it out into another field.
} | ||
|
||
/// Whether the blinking cursor is actually visible at this precise moment | ||
/// (it's hidden half the time, since it blinks). | ||
@visibleForTesting | ||
bool get cursorCurrentlyVisible => _showCursor.value; | ||
bool get cursorCurrentlyVisible => _cursorBlinkOpacityController.value > 0; |
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.
Refactored this to keep it working. Ideally we should get rid of it since it's not that useful but it's already public.
expect(renderEditable.cursorColor.alpha, 255); | ||
|
||
await tester.pump(); | ||
await tester.pump(const Duration(milliseconds: 200)); |
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.
@jslavitz I tweaked this a bit. In general, we want to be very with the number and durations of pumps to make sure the animation happens exactly the way we expect it to.
50e8a54
to
b548f86
Compare
#26026