Skip to content
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

Don't show an error when selecting an on-device implementation widget #8625

Merged
merged 10 commits into from
Dec 12, 2024
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.',
elliette marked this conversation as resolved.
Show resolved Hide resolved
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}) {
elliette marked this conversation as resolved.
Show resolved Hide resolved
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
Loading