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

Conversation

elliette
Copy link
Member

@elliette elliette commented Dec 11, 2024

Work towards #8626

In #8494, we added an error notification when a user selects an implementation widget on their device, but implementation widgets are hidden. This is not the most friendly UI.

Instead, with this PR we select the implementation widget's ancestor in the tree, and show a notification (not an error) that an implementation widget has been selected.

selecting_implementation_widget

@elliette elliette changed the title [DRAFT] Don't show an error when selecting an on-device implementation widget Don't show an error when selecting an on-device implementation widget Dec 12, 2024
@elliette elliette marked this pull request as ready for review December 12, 2024 19:16
@elliette elliette requested a review from a team as a code owner December 12, 2024 19:16
@elliette elliette requested review from kenzieschmoll and removed request for a team December 12, 2024 19:16
@@ -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.

@@ -1035,7 +1035,7 @@ class ObjectGroup extends InspectorObjectGroupBase {
switch (treeType) {
case FlutterTreeType.widget:
newSelection = await invokeServiceMethodReturningNodeInspectorRef(
inLocalProject
inLocalProjectOnly
Copy link
Member

Choose a reason for hiding this comment

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

I actually like the 'restrictToLocalProject' name that you suggested. I think this makes it more clear that the response is going to be restricted to a specific set of widgets.

Copy link
Member Author

Choose a reason for hiding this comment

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

SG! Switch to restrictToLocalProject

@elliette elliette merged commit 4d41156 into flutter:master Dec 12, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants