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

Make inspector polyfill compatible with both null safe and legacy Flutter #2387

Merged
merged 1 commit into from
Sep 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/devtools_app/analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@ analyzer:
exclude:
- lib/generated_plugin_registrant.dart
- macos/
# Enable this option if you need to test whether the inspector_polyfill_script is compatible with
# versions of flutter that require null safety.
# enable-experiment:
# - non-nullable
153 changes: 85 additions & 68 deletions packages/devtools_app/assets/scripts/inspector_polyfill_script.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@
// additional imports to this file.
// ignore_for_file: unused_import

// This code needs to execute both in null safe and legacy versions of Dart
// so it needs to be written in a way that is null safe without depending on any
// of the new null safety syntax. To test that this script works with null safe
// dart, edit the analysis_options file to enable null safety.
// If supporting null safe and legacy Dart at the same time becomes too
// difficult, we can fork this file and provide two polyfill scripts one for the
// legacy and one for the null safe versions of Dart.
// We must suppress unnecessary cast lints as the casts are necessary in
// null safe dart but the devtools_app library has not yet been migrated.
// ignore_for_file: unnecessary_cast

// Code from this class is executed within the context of the
// widget_inspector.dart library so using protected members is fine.
// ignore_for_file: invalid_use_of_visible_for_testing_member
Expand Down Expand Up @@ -70,8 +81,8 @@ import 'package:flutter/src/widgets/widget_inspector.dart';
// WidgetInspectorService show up as in scope as far as the analysis server is
// concerned.
extension WidgetInspectorServicePrivateMethods on WidgetInspectorService {
Map<String, Object> _nodeToJson(
DiagnosticsNode node,
Map<String, Object /*?*/ > /*?*/ _nodeToJson(
Copy link
Contributor

Choose a reason for hiding this comment

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

This and below would it be easier to find the null safety code by using a TODO or NNBD type of line comment e.g.,
// NNBD (jacob):
// Map<String, Object?>?_nodeToJson(
// DiagnosticsNode? node,
// Pre-NNBD(jacob):
Map<String, Object> _nodeToJson(
DiagnosticsNode node,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that is a bit cleaner. I'll add that refactor in the morning.

DiagnosticsNode /*?*/ node,
InspectorSerializationDelegate delegate,
) {
throw 'Dummy extension method to make the code type check when it calls private members';
Expand All @@ -94,43 +105,45 @@ String addServiceExtensions() {
return entry;
}
}
return null;
throw 'Enum value $name not found';
}

Future<Map<String, dynamic>> getLayoutExplorerNode(
Map<String, String> parameters) {
final String id = parameters['id'];
final int subtreeDepth = int.parse(parameters['subtreeDepth']);
final String groupName = parameters['groupName'];
Map<String, Object> result = {};
final id = parameters['id'];
final subtreeDepth = int.parse(parameters['subtreeDepth'] as String);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

writing as Foo is equivalent to writing !

final groupName = parameters['groupName'];
var result = <String, dynamic>{};
Copy link
Member

Choose a reason for hiding this comment

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

why the change from Object to dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has to be dynamic because that is now the type on the Flutter side. Before null safety, Object and dynamic basically meant the same thing so you could use them interchangeably in generics without getting warnings. With null safety they are different because
Object? and Object are different.

final instance = WidgetInspectorService.instance;
final root = instance.toObject(id);
if (root == null) {
result = null;
} else {
result = instance._nodeToJson(
root,
InspectorSerializationDelegate(
groupName: groupName,
summaryTree: true,
subtreeDepth: subtreeDepth,
service: instance,
addAdditionalPropertiesCallback: (node, delegate) {
final Map<String, Object> additionalJson = <String, Object>{};
final Object value = node.value;
if (value is Element) {
final renderObject = value.renderObject;
return Future.value(<String, dynamic>{
'result': result,
});
}
result = instance._nodeToJson(
root as DiagnosticsNode,
InspectorSerializationDelegate(
groupName: groupName,
summaryTree: true,
subtreeDepth: subtreeDepth,
service: instance,
addAdditionalPropertiesCallback: (node, delegate) {
final Map<String, Object> additionalJson = <String, Object>{};
final Object value = node.value as Object;
if (value is Element) {
final renderObject = value.renderObject;
if (renderObject != null) {
Copy link
Member

Choose a reason for hiding this comment

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

should this be checking that renderObject.toDiagnosticsNode != null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a typo in your question? renderObject.toDiagnosticsNode is a Function.
Do you mean checking that the result of toDiagnosticsNode() is not null? It is now enforced as null safe in Flutter and has always been null safe by convention. The previous check was overly paranoid and triggered a lint error with null safety.

additionalJson['renderObject'] =
renderObject.toDiagnosticsNode()?.toJsonMap(
renderObject.toDiagnosticsNode().toJsonMap(
delegate.copyWith(
subtreeDepth: 0,
includeProperties: true,
),
);
final Constraints constraints = renderObject.constraints;
if (constraints != null) {
final Map<String, Object> constraintsProperty =
<String, Object>{
) as Object;
try {
final constraints = renderObject.constraints;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the constraints property is now non-nullable but could throw an exception if it is accessed at the wrong time..

final constraintsProperty = <String, Object>{
'type': constraints.runtimeType.toString(),
'description': constraints.toString(),
};
Expand All @@ -143,88 +156,94 @@ String addServiceExtensions() {
});
}
additionalJson['constraints'] = constraintsProperty;
} catch (e) {
// Not laid out yet.
}
if (renderObject is RenderBox) {
additionalJson['size'] = <String, Object>{
'width': renderObject.size.width.toString(),
'height': renderObject.size.height.toString(),
};

final ParentData parentData = renderObject.parentData;
final ParentData parentData =
renderObject.parentData as ParentData;
Copy link
Contributor

Choose a reason for hiding this comment

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

final ParentData and as ParentData don't these do the same? Only need one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. these types don't need to be on the LHS but they do need to be on the RHS. This code matches flutter style of putting types everywhere so it is easier to merge into package:flutter.

if (parentData is FlexParentData) {
additionalJson['flexFactor'] = parentData.flex;
additionalJson['flexFactor'] = parentData.flex as int;
additionalJson['flexFit'] =
describeEnum(parentData.fit ?? FlexFit.tight);
}
}
}
return additionalJson;
}),
);
}
return Future.value(<String, Object>{
}
return additionalJson;
}),
) as Map<String, dynamic>;
return Future.value(<String, dynamic>{
Copy link
Member

Choose a reason for hiding this comment

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

again, why the change from Object to dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same answer as above.

'result': result,
});
}

Future<Map<String, dynamic>> setFlexFit(Map<String, String> parameters) {
final String id = parameters['id'];
final FlexFit flexFit =
toEnumEntry<FlexFit>(FlexFit.values, parameters['flexFit']);
final String id = parameters['id'] as String;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question final String and as String only one needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

answered lower down.

final parameter = parameters['flexFit'] as String;
final FlexFit flexFit = toEnumEntry<FlexFit>(FlexFit.values, parameter);
final dynamic object = WidgetInspectorService.instance.toObject(id);
if (object == null) return null;
final render = object.renderObject;
final parentData = render.parentData;
bool succeed = false;
if (parentData is FlexParentData) {
parentData.fit = flexFit;
render.markNeedsLayout();
succeed = true;
if (object != null) {
final render = object.renderObject;
final parentData = render.parentData;
if (parentData is FlexParentData) {
parentData.fit = flexFit;
render.markNeedsLayout();
succeed = true;
}
}
return Future.value(<String, Object>{
'result': succeed,
});
}

Future<Map<String, dynamic>> setFlexFactor(Map<String, String> parameters) {
final String id = parameters['id'];
final String flexFactor = parameters['flexFactor'];
final int factor = flexFactor == 'null' ? null : int.parse(flexFactor);
final String id = parameters['id'] as String;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question on these two too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parameters['id'] might be null.
In null safe dart the ideal way to write this is
final id = parameters['id']!;
but you can't use ! and make the code compatible with legacy dart.

final String flexFactor = parameters['flexFactor'] as String;
final factor = flexFactor == 'null' ? null : int.parse(flexFactor);
final dynamic object = WidgetInspectorService.instance.toObject(id);
if (object == null) return null;
final render = object.renderObject;
final parentData = render.parentData;
bool succeed = false;
if (parentData is FlexParentData) {
parentData.flex = factor;
render.markNeedsLayout();
succeed = true;
if (object != null) {
final render = object.renderObject;
final parentData = render.parentData;
if (parentData is FlexParentData) {
parentData.flex = factor;
render.markNeedsLayout();
succeed = true;
}
}
return Future.value({'result': succeed});
}

Future<Map<String, dynamic>> setFlexProperties(
Map<String, String> parameters) {
final String id = parameters['id'];
final String id = parameters['id'] as String;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question String and as String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parameters['id'] might be null.
In null safe dart the ideal way to write this is
final id = parameters['id']!;
but you can't use ! and make the code compatible with legacy dart.

final MainAxisAlignment mainAxisAlignment = toEnumEntry<MainAxisAlignment>(
MainAxisAlignment.values,
parameters['mainAxisAlignment'],
parameters['mainAxisAlignment'] as String,
);
final CrossAxisAlignment crossAxisAlignment =
toEnumEntry<CrossAxisAlignment>(
CrossAxisAlignment.values,
parameters['crossAxisAlignment'],
parameters['crossAxisAlignment'] as String,
);
final dynamic object = WidgetInspectorService.instance.toObject(id);
if (object == null) return null;
final render = object.renderObject;
bool succeed = false;
if (render is RenderFlex) {
render.mainAxisAlignment = mainAxisAlignment;
render.crossAxisAlignment = crossAxisAlignment;
render.markNeedsLayout();
render.markNeedsPaint();
succeed = true;
if (object != null) {
final render = object.renderObject;
if (render is RenderFlex) {
render.mainAxisAlignment = mainAxisAlignment;
render.crossAxisAlignment = crossAxisAlignment;
render.markNeedsLayout();
render.markNeedsPaint();
succeed = true;
}
}
return Future.value(<String, Object>{'result': succeed});
}
Expand All @@ -248,8 +267,6 @@ String addServiceExtensions() {
registerHelper('setFlexFit', setFlexFit);
registerHelper('setFlexFactor', setFlexFactor);
registerHelper('setFlexProperties', setFlexProperties);
return failures.isNotEmpty
? WidgetInspectorService.instance._safeJsonEncode(failures)
: null;
return WidgetInspectorService.instance._safeJsonEncode(failures);
// INSPECTOR_POLYFILL_SCRIPT_END
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,11 @@ $script''',
final encodedResult =
await group.inspectorLibrary.retrieveFullValueAsString(result);
if (encodedResult != null) {
final Map<String, Object> decodedError = json.decode(encodedResult);
if (encodedResult != null) {
for (String name in decodedError.keys) {
log(
"Unable to add service extension '$name' due to error:\n${decodedError[name]}",
);
}
final Map<String, Object> errors = json.decode(encodedResult);
for (String name in errors.keys) {
log(
"Unable to add service extension '$name' due to error:\n${errors[name]}",
);
}
}
}
2 changes: 1 addition & 1 deletion packages/devtools_app/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ version: 0.9.3-dev.1
homepage: https://github.com/flutter/devtools

environment:
sdk: '>=2.6.0 <3.0.0'
sdk: '>=2.10.0-56.0.dev <3.0.0'
# The flutter desktop support interacts with build scripts on the Flutter
# side that are not yet stable, so it requires a very recent version of
# Flutter. This version will increase regularly as the build scripts change.
Expand Down