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,
inLocalProject: 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.
inLocalProject: implementationWidgetsHidden.value,
Copy link
Member

Choose a reason for hiding this comment

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

if implementationWidgetsHidden.value is false, then the selected widget may or may not be in the local project right? Can we still select a "local project node" if inLocalProject is false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Regardless of whether implementationWidgetsHidden.value is true (only local project widgets in tree) or false (local project and implementation widgets in tree), the node that the user selected may or may not be in the local project. This is because we allow a user to select any widget with the on-device widget selector. Maybe inLocalProject is a bad name (I can change it to inLocalProjectOnly or restrictToLocalProject?). Because yes we can select a local project node if inLocalProject is false.

How on-device selection works (I had to poke around a bit because this was unintuitive to me - still want to write up a one-pager at some point):

  • a user selects a widget on their device
  • the Flutter Widget Inspector sends an "inspect" event
  • DevTools intercepts this "inspect" event and sends a getSelection request
  • DevTools uses the response from getSelection to determine which node to select in the widget tree

So what this change is doing is modifying the request to getSelection to ask for the selected local widget (if implementation widgets are hidden in the tree) because we are unable to show the implementation widget in the tree.

);

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,58 @@ 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) return;
if (!implementationWidgetsHidden.value) return;

// Return early if we have a new selected node.
if (_selectionIsOutOfDate(selectedNode)) return;
elliette marked this conversation as resolved.
Show resolved Hide resolved

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;

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 +1001,7 @@ class InspectorController extends DisposableController
}
}

void selectionChanged() {
void selectionChanged({bool notifyFlutterInspector = false}) {
elliette marked this conversation as resolved.
Show resolved Hide resolved
if (!visibleToUser) {
return;
}
Expand All @@ -975,18 +1017,24 @@ class InspectorController extends DisposableController
setSelectedNode(node);
unawaited(_addNodeToConsole(node));

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

void syncSelectionHelper({required RemoteDiagnosticsNode? selection}) {
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 inLocalProject = 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
inLocalProject
? 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