Skip to content

Commit

Permalink
Don't show an error when selecting an on-device implementation widget (
Browse files Browse the repository at this point in the history
  • Loading branch information
elliette authored Dec 12, 2024
1 parent 2da31a2 commit 4d41156
Show file tree
Hide file tree
Showing 7 changed files with 170 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ class InspectorController extends DisposableController
final pendingSelectionFuture = group.getSelection(
selectedDiagnostic,
treeType,
isSummaryTree: isSummaryTree,
restrictToLocalProject: isSummaryTree,
);

final pendingDetailsFuture =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import '../../shared/diagnostics/diagnostics_node.dart';
import '../../shared/diagnostics/inspector_service.dart';
import '../../shared/diagnostics/primitives/instance_ref.dart';
import '../../shared/globals.dart';
import '../../shared/managers/notifications.dart';
import '../../shared/primitives/query_parameters.dart';
import '../../shared/primitives/utils.dart';
import '../inspector_shared/inspector_screen.dart';
Expand Down Expand Up @@ -731,37 +732,26 @@ class InspectorController extends DisposableController
final pendingSelectionFuture = group.getSelection(
selectedDiagnostic,
treeType,
// If implementation widgets are hidden, the only widgets in the tree are
// those that were created by the local project.
restrictToLocalProject: implementationWidgetsHidden.value,
);

try {
final newSelection = await pendingSelectionFuture;

// Show an error and don't update the selected node in the tree if the
// user selected an implementation widget in the app while implementation
// widgets are hidden in the tree.
if (implementationWidgetsHidden.value && newSelection != null) {
final isInTree = valueToInspectorTreeNode.containsKey(
newSelection.valueRef,
);
final hasParent = newSelection.parent != null;
final isImplementationWidget = !isInTree && !hasParent;
if (isImplementationWidget) {
notificationService.pushError(
'Selected an implementation widget. Please toggle "Show Implementation Widgets" and select a widget from the device again.',
allowDuplicates: true,
isReportable: false,
);
return;
}
}

if (_disposed || group.disposed) return;

selectionGroups.promoteNext();

subtreeRoot = newSelection;

applyNewSelection(newSelection);

await _maybeShowNotificationForSelectedNode(
selectedNode: newSelection,
group: group,
);
} catch (error, st) {
if (selectionGroups.next == group) {
_log.shout(error, error, st);
Expand Down Expand Up @@ -818,6 +808,60 @@ class InspectorController extends DisposableController
animateTo(selectedNode.value);
}

static const _implementationWidgetMessage =
'Selected an implementation widget';

static const _notificationDuration = Duration(seconds: 4);

Future<void> _maybeShowNotificationForSelectedNode({
required RemoteDiagnosticsNode? selectedNode,
required ObjectGroup group,
}) async {
if (selectedNode == null ||
!implementationWidgetsHidden.value ||
_selectionIsOutOfDate(selectedNode)) {
return;
}

final possibleImplementationWidget = await group.getSelection(
selectedDiagnostic,
treeType,
);

// Return early if we have a new selected node.
if (_selectionIsOutOfDate(selectedNode)) return;

final isImplementationWidget =
possibleImplementationWidget != null &&
!possibleImplementationWidget.isCreatedByLocalProject;
if (isImplementationWidget) {
final selectedWidgetName = selectedNode.description ?? '';
final implementationWidgetName =
possibleImplementationWidget.description ?? '';

// Return early if we have a new selected node.
if (_selectionIsOutOfDate(selectedNode)) return;

// Show a notification that the user selected an implementation widget,
// e.g. "Selected an implementation widget of Text: RichText."
final messageDetails =
selectedWidgetName.isEmpty
? ''
: ' of $selectedWidgetName${implementationWidgetName.isEmpty ? '' : ': $implementationWidgetName'}';
notificationService.pushNotification(
NotificationMessage(
'$_implementationWidgetMessage$messageDetails.',
duration: _notificationDuration,
),
allowDuplicates: false,
);
}
}

bool _selectionIsOutOfDate(RemoteDiagnosticsNode selected) {
return selected.valueRef != selectedNode.value?.diagnostic?.valueRef;
}

Future<void> _loadPropertiesForNode(InspectorTreeNode? node) async {
final widgetProperties = <RemoteDiagnosticsNode>[];
final renderProperties = <RemoteDiagnosticsNode>[];
Expand Down Expand Up @@ -959,7 +1003,13 @@ class InspectorController extends DisposableController
}
}

void selectionChanged() {
/// Handles updating the widget tree when the selecected widget changes.
///
/// [notifyFlutterInspector] determines whether a request should be sent to
/// the Widget Inspector in the Flutter framework to update the on-device
/// selection. This should only be true if the the selection was changed due
/// to a user action in DevTools (e.g. clicking on a widget in the tree).
void selectionChanged({bool notifyFlutterInspector = false}) {
if (!visibleToUser) {
return;
}
Expand All @@ -975,18 +1025,30 @@ class InspectorController extends DisposableController
setSelectedNode(node);
unawaited(_addNodeToConsole(node));

syncSelectionHelper(selection: selectedDiagnostic);
syncSelectionHelper(
selection: selectedDiagnostic,
notifyFlutterInspector: notifyFlutterInspector,
);
}
}

void syncSelectionHelper({required RemoteDiagnosticsNode? selection}) {
/// Syncs the selection state after a new widgets was selected.
///
/// [notifyFlutterInspector] determines whether a request should be sent to
/// the Widget Inspector in the Flutter framework to update the on-device
/// selection. This should only be true if the the selection was changed due
/// to a user action in DevTools (e.g. clicking on a widget in the tree).
void syncSelectionHelper({
required RemoteDiagnosticsNode? selection,
bool notifyFlutterInspector = false,
}) {
if (selection != null) {
if (selection.isCreatedByLocalProject) {
_navigateTo(selection);
}
}

if (selection != null) {
if (notifyFlutterInspector && selection != null) {
unawaited(selection.setSelectionInspector(true));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,15 +193,20 @@ class InspectorTreeController extends DisposableController
}
}

bool setSelectedNode(InspectorTreeNode? node) {
bool setSelectedNode(
InspectorTreeNode? node, {
bool notifyFlutterInspector = false,
}) {
if (node == _selection) return false;

_selection?.selected = false;
_selection = node;
_selection?.selected = true;
final configLocal = config;
if (configLocal.onSelectionChange != null) {
configLocal.onSelectionChange!();
configLocal.onSelectionChange!(
notifyFlutterInspector: notifyFlutterInspector,
);
}
return true;
}
Expand Down Expand Up @@ -547,7 +552,7 @@ class InspectorTreeController extends DisposableController
}

void onSelectNode(InspectorTreeNode? node) {
setSelectedNode(node);
setSelectedNode(node, notifyFlutterInspector: true);
ga.select(gac.inspector, gac.treeNodeSelection);
final diagnostic = node?.diagnostic;
if (diagnostic != null && diagnostic.groupIsHidden) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
library;

import 'package:devtools_app_shared/ui.dart';
import 'package:flutter/foundation.dart';

import '../../diagnostics/diagnostics_node.dart';
import '../../ui/search.dart';
Expand Down Expand Up @@ -199,7 +198,7 @@ class InspectorTreeConfig {
});

final NodeAddedCallback? onNodeAdded;
final VoidCallback? onSelectionChange;
final void Function({bool notifyFlutterInspector})? onSelectionChange;
final void Function(bool added)? onClientActiveChange;
final TreeEventCallback? onExpand;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ class ObjectGroup extends InspectorObjectGroupBase {
Future<RemoteDiagnosticsNode?> getSelection(
RemoteDiagnosticsNode? previousSelection,
FlutterTreeType treeType, {
bool isSummaryTree = false,
bool restrictToLocalProject = false,
}) async {
// There is no reason to allow calling this method on a disposed group.
assert(!disposed);
Expand All @@ -1035,7 +1035,7 @@ class ObjectGroup extends InspectorObjectGroupBase {
switch (treeType) {
case FlutterTreeType.widget:
newSelection = await invokeServiceMethodReturningNodeInspectorRef(
isSummaryTree
restrictToLocalProject
? WidgetInspectorServiceExtensions.getSelectedSummaryWidget.name
: WidgetInspectorServiceExtensions.getSelectedWidget.name,
null,
Expand Down
3 changes: 2 additions & 1 deletion packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ To learn more about DevTools, check out the
settings to allow auto-refreshing the widget tree after a hot-reload. - [#8483](https://github.com/flutter/devtools/pull/8483)
- Fixed an [issue](https://github.com/flutter/devtools/issues/8487) where the [new Inspector](https://docs.flutter.dev/tools/devtools/release-notes/release-notes-2.40.1#inspector-updates) would not load after a hot-restart in the legacy Inspector. - [#8491](https://github.com/flutter/devtools/pull/8491)
- Fixed an [issue](https://github.com/flutter/devtools/issues/8465) preventing scrolling to an implementation widget in the [new Inspector](https://docs.flutter.dev/tools/devtools/release-notes/release-notes-2.40.1#inspector-updates) after selecting it on the device. - [#8494](https://github.com/flutter/devtools/pull/8494)

- Selecting an implementation widget on the device while implementation widget's are hidden in the [new Inspector's](https://docs.flutter.dev/tools/devtools/release-notes/release-notes-2.40.1#inspector-updates) does not show an error. - [8625](https://github.com/flutter/devtools/pull/8625)

## Performance updates

TODO: Remove this section if there are not any general updates.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,77 @@ void main() {
expect(find.richTextContaining('more widgets...'), findsNothing);
});

// TODO(elliette): Expand into test group for cases when:
// - selected widget is implementation widget and implementation widgets are hidden (this test case)
// - selected widget is implementation widget and implementation widgets are visible
// - selected widget is not implementation widget and implementation widgets are hidden
// - selected widget is not implementation widget and implementation widgets are visible
testWidgetsWithWindowSize('selecting implementation widget', windowSize, (
WidgetTester tester,
) async {
// Load the Inspector.
await _loadInspectorUI(tester);
await tester.pumpAndSettle(inspectorChangeSettleTime);
final state =
tester.state(find.byType(InspectorScreenBody))
as InspectorScreenBodyState;

// Find the first Text diagnostic node.
final diagnostics = state.controller.inspectorTree.rowsInTree.value.map(
(row) => row!.node.diagnostic,
);
final textDiagnostic =
diagnostics.firstWhere((d) => d?.description == 'Text')!;
expect(textDiagnostic.isCreatedByLocalProject, isTrue);

// Tap the "Show Implementation Widgets" button (selected by default).
final showImplementationWidgetsButton = find.descendant(
of: find.byType(DevToolsToggleButton),
matching: find.text('Show Implementation Widgets'),
);
expect(showImplementationWidgetsButton, findsOneWidget);
await tester.tap(showImplementationWidgetsButton);
await tester.pumpAndSettle(inspectorChangeSettleTime);

// Verify the Text diagnostic node is still in the tree.
final diagnosticsNow = state.controller.inspectorTree.rowsInTree.value.map(
(row) => row!.node.diagnostic,
);
expect(
diagnosticsNow.any((d) => d?.valueRef == textDiagnostic.valueRef),
isTrue,
);

// Get the RichText child of the Text diagnostic node.
final service = serviceConnection.inspectorService as InspectorService;
final group = service.createObjectGroup('test-group');
final textSubtree = await group.getDetailsSubtree(textDiagnostic);
final richTextDiagnostic = (await textSubtree!.children)!.firstWhere(
(child) => child.description == 'RichText',
);

// Verify the RichText child is an implementation node that is not in the tree.
expect(richTextDiagnostic.isCreatedByLocalProject, isFalse);
expect(
diagnosticsNow.any((d) => d?.valueRef == richTextDiagnostic.valueRef),
isFalse,
);

// Mimic selecting the RichText diagnostic node with the on-device inspector.
await group.setSelectionInspector(richTextDiagnostic.valueRef, false);
await tester.pumpAndSettle(inspectorChangeSettleTime);

// Verify the Text node is now selected.
final selectedNode = state.controller.selectedNode.value;
expect(selectedNode!.diagnostic!.valueRef, equals(textDiagnostic.valueRef));

// Verify the notification about selecting an implementation widget is displayed.
expect(
find.text('Selected an implementation widget of Text: RichText.'),
findsOneWidget,
);
});

testWidgetsWithWindowSize('can revert to legacy inspector', windowSize, (
WidgetTester tester,
) async {
Expand Down

0 comments on commit 4d41156

Please sign in to comment.