Skip to content

Pass 'valueId' (Diagnosticable) instead of 'objectId' (DiagnosticsNode) to inspector API. #5918

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jun 21, 2023
Merged

Pass 'valueId' (Diagnosticable) instead of 'objectId' (DiagnosticsNode) to inspector API. #5918

merged 11 commits into from
Jun 21, 2023

Conversation

polina-c
Copy link
Contributor

@polina-c polina-c commented Jun 14, 2023 •
edited
Loading

Prerequisite PRs that update APIs to take Diagnosticable:

flutter/flutter#128833
getChildrenSummaryTree

flutter/flutter#128897
getProperties

flutter/flutter#128962
getSelectedSummaryWidget
getSelectedWidget
getChildrenSummaryTree
getChildrenDetailsSubtree
getDetailsSubtree
getLayoutExplorerNode

Tests should start passing after switching to this version of Flutter:

Flutter 3.12.0-4.0.pre.106 • channel master • [email protected]:flutter/flutter.git
Framework • revision 5a7ab5a147 (15 minutes ago) • 2023-06-16 15:43:02 -0400
Engine • revision aefe104cd0
Tools • Dart 3.1.0 (build 3.1.0-213.0.dev) • DevTools 2.24.0

@polina-c polina-c mentioned this pull request Jun 14, 2023
Closed
@polina-c polina-c changed the title Stop using 'objectId' in inspector. Stop using 'objectId' in DevTools inspector. Jun 14, 2023
@polina-c polina-c changed the title Stop using 'objectId' in DevTools inspector. Pass 'valueId' (Diagnosticable) instead of 'objectId' (DiagnosticsNode) to inspector API. Jun 15, 2023
@polina-c polina-c marked this pull request as ready for review June 16, 2023 17:17
@polina-c polina-c requested review from CoderDake and a team as code owners June 16, 2023 17:17
jacob314
jacob314 reviewed Jun 16, 2023
View reviewed changes
packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart
@@ -603,16 +603,6 @@ class InspectorController extends DisposableController
_refreshRateLimiter.scheduleRequest();
}

bool identicalDiagnosticsNodes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Fyi @incendial, why wasn't this method detected as dead code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, will check.

Copy link
Contributor

@incendial incendial Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think I was running the command without the --monorepo option. And since inspector_controller is in the package exports, any unused code is skipped.

With the option it shows a bunch of things:

lib/src/screens/inspector/inspector_controller.dart:
    ⚠ unused field highlightNodesShownInBothTrees
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:237:3
    ⚠ unused method getTreeType
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:254:3
    ⚠ unused method highlightShowFromNodeInstanceRef
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:285:3
    ⚠ unused method getSubtreeRootValue
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:493:3
    ⚠ unused method getTreeNode
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:574:3
    ⚠ unused method maybeUpdateValueUI
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:578:3
    ⚠ unused method identicalDiagnosticsNodes
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:602:3
    ⚠ unused method treeTypeDisplayName
      at /Users/dimannich/Desktop/code/devtools/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart:922:3

Copy link
Contributor

@incendial incendial Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

And in other files as well.

Can create a PR if the team is ready for another unused code removal round 🙂.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be amazing. Thank you!!!

Copy link
Contributor

@incendial incendial Jun 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I was running the command on a stale fork, so some of these reports have already been removed 😅

jacob314
jacob314 reviewed Jun 16, 2023
View reviewed changes
packages/devtools_app/lib/src/screens/inspector/layout_explorer/ui/layout_explorer_widget.dart
@@ -122,7 +122,7 @@ abstract class LayoutExplorerWidgetState<W extends LayoutExplorerWidget,

void updateHighlighted(L? newProperties);

String? id(RemoteDiagnosticsNode? node) => node?.dartDiagnosticRef.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to verify that the layout explorer still works after this change. For example, try changing flex values for widgets in a row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2023-06-16.at.1.47.59.PM.mov

jacob314
jacob314 approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

lgtm
Thank you for cleaning up this tech debt! This should make the inspector more performant (half as many object ids created) as well as leak less memory.

@polina-c polina-c mentioned this pull request Jun 16, 2023
Merged
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 16, 2023
@polina-c
Update objectToDiagnosticsNode to stop failing. (#129027)
9690394 
Prerequisite for flutter/devtools#5918
@polina-c polina-c merged commit 3ce878c into flutter:master Jun 21, 2023
@polina-c polina-c deleted the inspector branch June 21, 2023 14:51
@incendial incendial mentioned this pull request Jun 25, 2023
Merged
4 tasks
This was referenced Jun 21, 2023
Closed
Merged
CoderDake added a commit to CoderDake/devtools that referenced this pull request Jul 5, 2023
@CoderDake
Revert "Pass 'valueId' (Diagnosticable) instead of 'objectId' (Diagno…
ec834bc 
…sticsNode) to inspector API. (flutter#5918)"

This reverts commit 3ce878c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incendial incendial incendial left review comments

@jacob314 jacob314 jacob314 approved these changes

@CoderDake CoderDake Awaiting requested review from CoderDake

Assignees
No one assigned
Labels
release-notes-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@polina-c @jacob314 @incendial