Add additional properties callback in Inspector Serialization Delegate by albertusdev · Pull Request #45531 · flutter/flutter

albertusdev · 2019-11-25T09:09:28Z

Description

Added callback to add additional experimental serialization properties.
This callback can be used to customize the serialization of DiagnosticsNode objects for experimental features. For example, Dart DevTools can evaluate the following expression to register a VM Service API with a custom serialization to experiment with visualizing layouts.

Related Issues

Tests

I added the following tests:

InspectorSerializationDelegate addAdditionalPropertiesCallback

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

fluttergithubbot · 2019-11-25T09:09:30Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jacob314

Add a test to widget_inspector_test.dart
verifying the callback works as expected. You will find some existing code that tests the service extensions. You could write a test that simulates registering a custom service extension like the one you need to register from DevTools. In widget_inspector_test.dart, the service extension logic is stubbed out so it is easy to trigger service extensions and validate the responses.

…dd test

albertusdev · 2019-11-25T22:41:36Z

@jacob314 ready for your another round of review before requesting review from Flutter team, might need your suggestion on the docs as well since we make the SerializationDelegate public now

jacob314 · 2019-11-26T00:42:57Z

+  ///
+  /// Example usage:
+  ///
+  /// Map<String, Object> getDetailsSubtreeWithRenderObject(


wrap this code example with
"```dart"

jacob314 · 2019-11-26T00:46:07Z

+  /// the following expression to register a VM Service API with a custom serialization
+  /// to experiment with visualizing layouts.
+  ///
+  /// Example usage:


Replace "Example usage:" with
The following code samples demonstrates adding the RenderObject associated with an Element to the
serialized data for all elements in the tree.

jacob314 · 2019-11-26T00:47:38Z

+  /// Callback to add additional experimental serialization properties.
+  ///
+  /// This callback can be used to customize the serialization of DiagnosticsNode
+  /// objects for experimental features. For example, Dart DevTools can evaluate


replace
"Dart Devtools"
with
[Dart Devtools](https://github.com/flutter/devtools)

jacob314 · 2019-11-26T00:51:58Z


-class _SerializationDelegate implements DiagnosticsSerializationDelegate {
-  _SerializationDelegate({
+/// A delegate that configures how a hierarchy of [DiagnosticsNode]s should be


Add a @visibleForTesting annotation.

Tweak the comment to
/// A delegate that configures how a hierarchy of [DiagnosticsNode]s are
/// serialized by the Flutter Inspector.

jacob314 · 2019-11-26T00:55:48Z

+                    delegate.copyWith(subtreeDepth: 0),
+                  );
+              }
+              additionalJson['callbackIsCalled'] = true;


nit: callbackIsCalled -->
callbackExecuted

jacob314 · 2019-11-26T00:57:01Z

+      expect(json['renderObject'], isA<Map<String, dynamic>>());
+      final Map<String, dynamic> renderObjectJson = json['renderObject'];
+      expect(renderObjectJson['description'], startsWith('RenderFlex'));
+      final InspectorSerializationDelegate unsafeDelegate =


nit: add a line break before lin 2652.

jacob314 · 2019-11-26T00:57:47Z

+      expect(json['renderObject'], isA<Map<String, dynamic>>());
+      final Map<String, dynamic> renderObjectJson = json['renderObject'];
+      expect(renderObjectJson['description'], startsWith('RenderFlex'));
+      final InspectorSerializationDelegate unsafeDelegate =


nit: unsafeDelegate to emptyDelegate.

jacob314

a few minor nits.
This functionality will be really useful for us to experiment with new Flutter Inspector features without landing code changes before the features are proven to be useful!

goderbauer

Looks good. Just some doc nits.

goderbauer · 2019-11-26T18:30:18Z

+  /// can evaluate the following expression to register a VM Service API
+  /// with a custom serialization to experiment with visualizing layouts.
+  ///
+  /// The following code samples demonstrates adding the RenderObject associated


[RenderObject]

goderbauer · 2019-11-26T18:30:24Z

+  /// with a custom serialization to experiment with visualizing layouts.
+  ///
+  /// The following code samples demonstrates adding the RenderObject associated
+  /// with an Element to the serialized data for all elements in the tree:


goderbauer · 2019-11-26T18:30:33Z

+  ///
+  /// The following code samples demonstrates adding the RenderObject associated
+  /// with an Element to the serialized data for all elements in the tree:
+  /// ```dart


Add a blank line before this.

goderbauer · 2019-11-26T18:31:21Z

+  /// Service used by GUI tools to interact with the [WidgetInspector].
  final WidgetInspectorService service;
+
+  /// The `groupName` parameter is not needed but is specified to regularize the


This doesn't really document what this property is for.

Alright, I agree.

goderbauer · 2019-11-26T18:31:36Z

+
  final List<DiagnosticsNode> _nodesCreatedByLocalProject = <DiagnosticsNode>[];

+  /// Whether the Inspector is interactive or not


Can you document what it means to be interactive?

goderbauer · 2019-11-26T18:31:52Z

    // that also exist in the summary tree. This ensures that every time
    // you expand a node in the details tree, you expand the entire subtree
-    // up until you reach the next nodes shared with the summary tree.
+    // up until you reach the next nodes shared with the sum mary tree.


revert this?

Thank you for catching this!

goderbauer · 2019-11-26T18:33:11Z

+  /// ) {
+  ///   return _nodeToJson(
+  ///     root,
+  ///     InspectorSerializationDelegate(


Where would I run this code? InspectorSerializationDelegate is marked as visibleForTesting, so would this only be in test code?

Debugging tools such as Dart DevTools can access it even if it was still private via the VM Service protocol. It is marked @visibleForTesting so we can test it from widget_inspector_test.dart

This code is run through the eval in the Dart DevTools.
But currently in the flutter framework yes it is only in the test code.

albertusdev · 2019-11-26T19:17:50Z

@goderbauer ready for another round of review after improving the docs :-)

goderbauer

LGTM

Add additional properties callback in Inspector Serialization Delegate

5cccc8e

albertusdev requested a review from jacob314 November 25, 2019 09:09

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 25, 2019

googlebot added the cla: yes label Nov 25, 2019

albertusdev mentioned this pull request Nov 25, 2019

[Story of Your Layout] Remove dependency to experimental flag flutter/devtools#1396

Merged

jacob314 reviewed Nov 25, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/widget_inspector.dart Outdated

jacob314 reviewed Nov 25, 2019

View reviewed changes

Piinks added the work in progress; do not review label Nov 25, 2019

Albertus Angga Raharja added 2 commits November 25, 2019 14:10

Rename _SerializationDelegate to InspectorSerializationDelegate and a…

7e94e2c

…dd test

Fix indentation

aba22ee

albertusdev changed the title ~~[WIP] Add additional properties callback in Inspector Serialization Delegate~~ Add additional properties callback in Inspector Serialization Delegate Nov 25, 2019

Remove trailing whitespace

6d46ed1

albertusdev force-pushed the add-additional-properties-callback-widget-inspector-delegate branch from 4f5b9aa to 6d46ed1 Compare November 25, 2019 22:32

Handle case when addAdditionalPropertiesCallback returns null

63441dc

albertusdev removed the work in progress; do not review label Nov 25, 2019

jacob314 reviewed Nov 26, 2019

View reviewed changes

Improve docs and minor renames

c8234ac

jacob314 requested a review from goderbauer November 26, 2019 01:08

Improve docs

9a1ffcd

goderbauer reviewed Nov 26, 2019

View reviewed changes

Improve documentation

dc92bc1

goderbauer approved these changes Nov 26, 2019

View reviewed changes

albertusdev merged commit e1512b4 into flutter:master Nov 26, 2019

github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021


		final List<DiagnosticsNode> _nodesCreatedByLocalProject = <DiagnosticsNode>[];

		/// Whether the Inspector is interactive or not

Conversation

albertusdev commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Nov 25, 2019

Uh oh!

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

albertusdev commented Nov 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertusdev commented Nov 26, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

albertusdev commented Nov 25, 2019 •

edited

Loading