[go: nahoru, domu]
Skip to content

Commit

Permalink
Make _focusDebug not interpolate in debug mode (flutter#119680)
Browse files Browse the repository at this point in the history 
* Make _focusDebug not interpolate in debug mode

* Add test

* Revert undesired change

* Fix test to fail before too

* Remove accidental skips

* Switch to using a generating closure for arguments.

* Remove a word
  • Loading branch information
@gspencergoog
gspencergoog committed Feb 3, 2023
1 parent 66b2ca6 commit c626460
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 28 deletions.
78 changes: 50 additions & 28 deletions packages/flutter/lib/src/widgets/focus_manager.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,38 @@ import 'framework.dart';
/// be logged.
bool debugFocusChanges = false;

bool _focusDebug(String message, [Iterable<String>? details]) {
if (debugFocusChanges) {
debugPrint('FOCUS: $message');
if (details != null && details.isNotEmpty) {
for (final String detail in details) {
debugPrint(' $detail');
}
// When using _focusDebug, always call it like so:
//
// assert(_focusDebug(() => 'Blah $foo'));
//
// It needs to be inside the assert in order to be removed in release mode, and
// it needs to use a closure to generate the string in order to avoid string
// interpolation when debugFocusChanges is false.
//
// It will throw a StateError if you try to call it when the app is in release
// mode.
bool _focusDebug(
String Function() messageFunc, [
Iterable<Object> Function()? detailsFunc,
]) {
if (kReleaseMode) {
throw StateError(
'_focusDebug was called in Release mode. It should always be wrapped in '
'an assert. Always call _focusDebug like so:\n'
r" assert(_focusDebug(() => 'Blah $foo'));"
);
}
if (!debugFocusChanges) {
return true;
}
debugPrint('FOCUS: ${messageFunc()}');
final Iterable<Object> details = detailsFunc?.call() ?? const <Object>[];
if (details.isNotEmpty) {
for (final Object detail in details) {
debugPrint(' $detail');
}
}
// Return true so that it can be easily used inside of an assert.
// Return true so that it can be used inside of an assert.
return true;
}

Expand Down Expand Up @@ -118,10 +140,10 @@ class _Autofocus {
&& scope.focusedChild == null
&& autofocusNode.ancestors.contains(scope);
if (shouldApply) {
assert(_focusDebug('Applying autofocus: $autofocusNode'));
assert(_focusDebug(() => 'Applying autofocus: $autofocusNode'));
autofocusNode._doRequestFocus(findFirstFocus: true);
} else {
assert(_focusDebug('Autofocus request discarded for node: $autofocusNode.'));
assert(_focusDebug(() => 'Autofocus request discarded for node: $autofocusNode.'));
}
}
}
Expand Down Expand Up @@ -169,7 +191,7 @@ class FocusAttachment {
///
/// Calling [FocusNode.dispose] will also automatically detach the node.
void detach() {
assert(_focusDebug('Detaching node:', <String>[_node.toString(), 'With enclosing scope ${_node.enclosingScope}']));
assert(_focusDebug(() => 'Detaching node:', () => <Object>[_node, 'With enclosing scope ${_node.enclosingScope}']));
if (isAttached) {
if (_node.hasPrimaryFocus || (_node._manager != null && _node._manager!._markedForFocus == _node)) {
_node.unfocus(disposition: UnfocusDisposition.previouslyFocusedChild);
Expand Down Expand Up @@ -877,7 +899,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
scope._doRequestFocus(findFirstFocus: true);
break;
}
assert(_focusDebug('Unfocused node:', <String>['primary focus was $this', 'next focus will be ${_manager?._markedForFocus}']));
assert(_focusDebug(() => 'Unfocused node:', () => <Object>['primary focus was $this', 'next focus will be ${_manager?._markedForFocus}']));
}

/// Removes the keyboard token from this focus node if it has one.
Expand Down Expand Up @@ -1063,7 +1085,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
// Note that this is overridden in FocusScopeNode.
void _doRequestFocus({required bool findFirstFocus}) {
if (!canRequestFocus) {
assert(_focusDebug('Node NOT requesting focus because canRequestFocus is false: $this'));
assert(_focusDebug(() => 'Node NOT requesting focus because canRequestFocus is false: $this'));
return;
}
// If the node isn't part of the tree, then we just defer the focus request
Expand All @@ -1078,7 +1100,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
return;
}
_hasKeyboardToken = true;
assert(_focusDebug('Node requesting focus: $this'));
assert(_focusDebug(() => 'Node requesting focus: $this'));
_markNextFocus(this);
}

Expand Down Expand Up @@ -1109,7 +1131,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
FocusNode scopeFocus = this;
for (final FocusScopeNode ancestor in ancestors.whereType<FocusScopeNode>()) {
assert(scopeFocus != ancestor, 'Somehow made a loop by setting focusedChild to its scope.');
assert(_focusDebug('Setting $scopeFocus as focused child for scope:', <String>[ancestor.toString()]));
assert(_focusDebug(() => 'Setting $scopeFocus as focused child for scope:', () => <Object>[ancestor]));
// Remove it anywhere in the focused child history.
ancestor._focusedChildren.remove(scopeFocus);
// Add it to the end of the list, which is also the top of the queue: The
Expand Down Expand Up @@ -1276,7 +1298,7 @@ class FocusScopeNode extends FocusNode {
/// tree, the given scope must be a descendant of this scope.
void setFirstFocus(FocusScopeNode scope) {
assert(scope != this, 'Unexpected self-reference in setFirstFocus.');
assert(_focusDebug('Setting scope as first focus in $this to node:', <String>[scope.toString()]));
assert(_focusDebug(() => 'Setting scope as first focus in $this to node:', () => <Object>[scope]));
if (scope._parent == null) {
_reparent(scope);
}
Expand Down Expand Up @@ -1306,7 +1328,7 @@ class FocusScopeNode extends FocusNode {
}

assert(_manager != null);
assert(_focusDebug('Autofocus scheduled for $node: scope $this'));
assert(_focusDebug(() => 'Autofocus scheduled for $node: scope $this'));
_manager?._pendingAutofocuses.add(_Autofocus(scope: this, autofocusNode: node));
_manager?._markNeedsUpdate();
}
Expand Down Expand Up @@ -1542,7 +1564,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
void _markDetached(FocusNode node) {
// The node has been removed from the tree, so it no longer needs to be
// notified of changes.
assert(_focusDebug('Node was detached: $node'));
assert(_focusDebug(() => 'Node was detached: $node'));
if (_primaryFocus == node) {
_primaryFocus = null;
}
Expand All @@ -1551,7 +1573,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {

void _markPropertiesChanged(FocusNode node) {
_markNeedsUpdate();
assert(_focusDebug('Properties changed for node $node.'));
assert(_focusDebug(() => 'Properties changed for node $node.'));
_dirtyNodes.add(node);
}

Expand All @@ -1575,7 +1597,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
// Request that an update be scheduled, optionally requesting focus for the
// given newFocus node.
void _markNeedsUpdate() {
assert(_focusDebug('Scheduling update, current focus is $_primaryFocus, next focus will be $_markedForFocus'));
assert(_focusDebug(() => 'Scheduling update, current focus is $_primaryFocus, next focus will be $_markedForFocus'));
if (_haveScheduledUpdate) {
return;
}
Expand All @@ -1597,7 +1619,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
// then revert to the root scope.
_markedForFocus = rootScope;
}
assert(_focusDebug('Refreshing focus state. Next focus will be $_markedForFocus'));
assert(_focusDebug(() => 'Refreshing focus state. Next focus will be $_markedForFocus'));
// A node has requested to be the next focus, and isn't already the primary
// focus.
if (_markedForFocus != null && _markedForFocus != _primaryFocus) {
Expand All @@ -1613,7 +1635,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
}
assert(_markedForFocus == null);
if (previousFocus != _primaryFocus) {
assert(_focusDebug('Updating focus from $previousFocus to $_primaryFocus'));
assert(_focusDebug(() => 'Updating focus from $previousFocus to $_primaryFocus'));
if (previousFocus != null) {
_dirtyNodes.add(previousFocus);
}
Expand All @@ -1624,7 +1646,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
for (final FocusNode node in _dirtyNodes) {
node._notify();
}
assert(_focusDebug('Notified ${_dirtyNodes.length} dirty nodes:', _dirtyNodes.toList().map<String>((FocusNode node) => node.toString())));
assert(_focusDebug(() => 'Notified ${_dirtyNodes.length} dirty nodes:', () => _dirtyNodes));
_dirtyNodes.clear();
if (previousFocus != _primaryFocus) {
notifyListeners();
Expand Down Expand Up @@ -1766,9 +1788,9 @@ class _HighlightModeManager {
_lastInteractionWasTouch = false;
updateMode();

assert(_focusDebug('Received key event $message'));
assert(_focusDebug(() => 'Received key event $message'));
if (FocusManager.instance.primaryFocus == null) {
assert(_focusDebug('No primary focus for key event, ignored: $message'));
assert(_focusDebug(() => 'No primary focus for key event, ignored: $message'));
return false;
}

Expand All @@ -1794,11 +1816,11 @@ class _HighlightModeManager {
case KeyEventResult.ignored:
continue;
case KeyEventResult.handled:
assert(_focusDebug('Node $node handled key event $message.'));
assert(_focusDebug(() => 'Node $node handled key event $message.'));
handled = true;
break;
case KeyEventResult.skipRemainingHandlers:
assert(_focusDebug('Node $node stopped key event propagation: $message.'));
assert(_focusDebug(() => 'Node $node stopped key event propagation: $message.'));
handled = false;
break;
}
Expand All @@ -1808,7 +1830,7 @@ class _HighlightModeManager {
break;
}
if (!handled) {
assert(_focusDebug('Key event not handled by anyone: $message.'));
assert(_focusDebug(() => 'Key event not handled by anyone: $message.'));
}
return handled;
}
Expand Down
57 changes: 57 additions & 0 deletions packages/flutter/test/widgets/focus_manager_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1700,4 +1700,61 @@ void main() {
expect(messagesStr, contains('FOCUS: Notified 2 dirty nodes'));
expect(messagesStr, contains(RegExp(r'FOCUS: Scheduling update, current focus is null, next focus will be FocusScopeNode#.*parent1')));
});

testWidgets("doesn't call toString on a focus node when debugFocusChanges is false", (WidgetTester tester) async {
final bool oldDebugFocusChanges = debugFocusChanges;
final DebugPrintCallback oldDebugPrint = debugPrint;
final StringBuffer messages = StringBuffer();
debugPrint = (String? message, {int? wrapWidth}) {
messages.writeln(message ?? '');
};
Future<void> testDebugFocusChanges() async {
final BuildContext context = await setupWidget(tester);
final FocusScopeNode parent1 = FocusScopeNode(debugLabel: 'parent1');
final FocusAttachment parent1Attachment = parent1.attach(context);
final FocusNode child1 = debugFocusChanges ? FocusNode(debugLabel: 'child1') : _LoggingTestFocusNode(debugLabel: 'child1');
final FocusAttachment child1Attachment = child1.attach(context);
parent1Attachment.reparent(parent: tester.binding.focusManager.rootScope);
child1Attachment.reparent(parent: parent1);

child1.requestFocus();
await tester.pump();
child1.dispose();
parent1.dispose();
await tester.pump();
}
try {
debugFocusChanges = false;
await testDebugFocusChanges();
expect(messages, isEmpty);
expect(tester.takeException(), isNull);
debugFocusChanges = true;
await testDebugFocusChanges();
expect(messages.toString(), contains('FOCUS: Notified 3 dirty nodes:'));
expect(tester.takeException(), isNull);
} finally {
debugFocusChanges = oldDebugFocusChanges;
debugPrint = oldDebugPrint;
}
});
}

class _LoggingTestFocusNode extends FocusNode {
_LoggingTestFocusNode({super.debugLabel});

@override
String toString({
DiagnosticLevel minLevel = DiagnosticLevel.debug,
}) {
throw StateError("Shouldn't call toString here");
}

@override
String toStringDeep({
String prefixLineOne = '',
String? prefixOtherLines,
DiagnosticLevel minLevel = DiagnosticLevel.debug,
}) {
throw StateError("Shouldn't call toStringDeep here");
}
}

0 comments on commit c626460

Please sign in to comment.