Skip to content

[SuperEditor][SuperTextField][iOS] Allow animating caret blink (Resolves #2099) #2238

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

[SuperEditor][SuperTextField][iOS] Allow animating caret blink (Resolves #2099) #2238

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
507d134
[SuperEditor][SuperTextField][iOS] Allow animating caret blink (Resol…
angelosilvestre Aug 13, 2024
3ac0f64
Update transition duration
angelosilvestre Aug 13, 2024
5cb5f3a
PR updates
angelosilvestre Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion super_editor/lib/src/infrastructure/platforms/ios/ios_document_controls.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ class IosControlsDocumentLayerState extends DocumentLayoutLayerState<IosHandlesD
bool get isCaretDisplayed => layoutData?.caret != null;

@visibleForTesting
bool get isCaretVisible => _caretBlinkController.opacity == 1.0 && isCaretDisplayed;
bool get isCaretVisible => _caretBlinkController.isVisible && isCaretDisplayed;

@visibleForTesting
Duration get caretFlashPeriod => _caretBlinkController.flashPeriod;
Expand Down
67 changes: 60 additions & 7 deletions super_text_layout/lib/src/infrastructure/blink_controller.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,27 @@ class BlinkController with ChangeNotifier {
@visibleForTesting
static bool indeterminateAnimationsEnabled = true;

/// Creates a [BlinkController] that uses a ticker to switch between visible
/// and invisible.
///
/// If [animate] is `true`, the caret animates its [opacity] when switching
/// between visible and invisible. If [animate] is `false`, the caret
/// switches between fully opaque and fully transparent.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a point here about performance? The reason we originally got rid of animated blinking was to reduce Flutter re-renders.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the original performance issue really about re-renders? Or was it related to repaints? I'm wondering if using e RepaintBoundary in our overlayers would make a difference.

BlinkController({
required TickerProvider tickerProvider,
Duration flashPeriod = const Duration(milliseconds: 500),
}) : _flashPeriod = flashPeriod {
bool animate = false,
Duration fadeDuration = const Duration(milliseconds: 200),
}) : _flashPeriod = flashPeriod,
_fadeDuration = fadeDuration,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can the fade duration be independent of the flash period?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caret isn't constantly fading in and out. It stays visible or invisible during the flash period, then quickly fades in/out.

Should we just use half of the flash period?

_animate = animate {
_ticker = tickerProvider.createTicker(_onTick);
}

BlinkController.withTimer({
Duration flashPeriod = const Duration(milliseconds: 500),
}) : _flashPeriod = flashPeriod;
}) : _fadeDuration = Duration.zero,
_flashPeriod = flashPeriod;

@override
void dispose() {
Expand All @@ -39,10 +50,13 @@ class BlinkController with ChangeNotifier {

Timer? _timer;

final Duration _flashPeriod;

/// Duration to switch between visible and invisible.
Duration get flashPeriod => _flashPeriod;
final Duration _flashPeriod;

/// Duration of the fade in or out transition when switching
/// between visible and invisible.
final Duration _fadeDuration;

/// Returns `true` if this controller is currently animating a blinking
/// signal, or `false` if it's not.
Expand All @@ -62,8 +76,24 @@ class BlinkController with ChangeNotifier {
notifyListeners();
}

/// Whether or not the caret is currently visible.
///
/// It's possible that [isVisible] is `false` while [opacity] is greater than `0.0`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than sayign something is possible, please describe under which circumstances something happens or doesn't happen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

/// when the caret is fading out.
bool get isVisible => _isVisible;
bool _isVisible = true;
double get opacity => _isVisible ? 1.0 : 0.0;

double get opacity => _opacity;
double _opacity = 1.0;

/// Whether or not the caret should animate its opacity.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also specify the alternative. It's possible a developer reads this and then asks "what does it mean to not animate the opacity?" - let's make the distinction between fading blinking vs on/off blinking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates.

bool _animate = false;

/// Whether or not the caret should maintain its opacity until the next blink.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Dart Doc comment makes it sound like this property is a permanent configuration on this controller, but in fact it looks like a momentary flag that's switched on and off to capture the fact that we're at a specific moment in time. Please try to replace this description with one that describes what this property is tracking, including when we expect it to flip from true to false, or false to true. Also, the name of this should probably start with _isSomething..., if it represents a temporary condition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to improve it a little bit.

///
/// For example, when the user places the caret, we want it to be visible with
/// full opacity until the caret switches to invisible.
bool _remainOpaqueUntilNextBlink = false;

void startBlinking() {
if (!indeterminateAnimationsEnabled) {
Expand All @@ -73,6 +103,9 @@ class BlinkController with ChangeNotifier {
}

if (_ticker != null) {
// Don't animate the opacity until the flash period has elapsed.
_remainOpaqueUntilNextBlink = true;

// We're using a Ticker to blink. Restart it.
_ticker!
..stop()
Expand All @@ -88,12 +121,17 @@ class BlinkController with ChangeNotifier {
}

void stopBlinking() {
_isVisible = true; // If we're not blinking then we need to be visible
// If we're not blinking then we need to be visible with full opacity.
_isVisible = true;
_opacity = 1.0;

// Keep the caret opaque until the flash period has elapsed, when it
// starts to fade out.
_remainOpaqueUntilNextBlink = true;

if (_ticker != null) {
// We're using a Ticker to blink. Stop it.
_ticker?.stop();
_ticker = null;
} else {
// We're using a Timer to blink. Stop it.
_timer?.cancel();
Expand All @@ -118,6 +156,12 @@ class BlinkController with ChangeNotifier {
}

void _onTick(Duration elapsedTime) {
if (isBlinking && _animate && !_remainOpaqueUntilNextBlink) {
final percentage = ((elapsedTime - _lastBlinkTime).inMilliseconds / _fadeDuration.inMilliseconds).clamp(0.0, 1.0);
_opacity = _isVisible ? percentage : 1 - percentage;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment to clarify the meaning of _isVisible in this calculation, because you're actually using it as a proxy to decide whether we're "fading in" or "fading out", and that's not immediately obvious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

notifyListeners();
}

if (elapsedTime - _lastBlinkTime >= _flashPeriod) {
_blink();
_lastBlinkTime = elapsedTime;
Expand All @@ -126,6 +170,15 @@ class BlinkController with ChangeNotifier {

void _blink() {
_isVisible = !_isVisible;
if (!_animate) {
// We are not animating the visibility, so the caret is
// either fully visible or fully invisible.
_opacity = _isVisible ? 1.0 : 0.0;
} else {
// We are animating the visibility, start fading in or out.
_remainOpaqueUntilNextBlink = false;
}

notifyListeners();

if (_timer != null && _isBlinkingEnabled) {
Expand Down
Loading