Make inspector polyfill compatible with both null safe and legacy Flutter #2387
Conversation
| final String groupName = parameters['groupName']; | ||
| Map<String, Object> result = {}; | ||
| final id = parameters['id']; | ||
| final subtreeDepth = int.parse(parameters['subtreeDepth'] as String); |
There was a problem hiding this comment.
writing as Foo is equivalent to writing !
| <String, Object>{ | ||
| ) as Object; | ||
| try { | ||
| final constraints = renderObject.constraints; |
There was a problem hiding this comment.
the constraints property is now non-nullable but could throw an exception if it is accessed at the wrong time..
| @@ -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. Te test that this script works with null safe | |||
| final id = parameters['id']; | ||
| final subtreeDepth = int.parse(parameters['subtreeDepth'] as String); | ||
| final groupName = parameters['groupName']; | ||
| var result = <String, dynamic>{}; |
There was a problem hiding this comment.
why the change from Object to dynamic?
There was a problem hiding this comment.
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 Object value = node.value as Object; | ||
| if (value is Element) { | ||
| final renderObject = value.renderObject; | ||
| if (renderObject != null) { |
There was a problem hiding this comment.
should this be checking that renderObject.toDiagnosticsNode != null?
There was a problem hiding this comment.
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.
| return additionalJson; | ||
| }), | ||
| ) as Map<String, dynamic>; | ||
| return Future.value(<String, dynamic>{ |
There was a problem hiding this comment.
again, why the change from Object to dynamic?
There was a problem hiding this comment.
same answer as above.
|
Reverted the pubspec.lock update to avoid a deprecated method issue with the latest vm_service package version. |
| @@ -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( | |||
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
I agree that is a bit cleaner. I'll add that refactor in the morning.
| } | ||
| 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; |
There was a problem hiding this comment.
final ParentData and as ParentData don't these do the same? Only need one?
There was a problem hiding this comment.
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.
| final String id = parameters['id']; | ||
| final FlexFit flexFit = | ||
| toEnumEntry<FlexFit>(FlexFit.values, parameters['flexFit']); | ||
| final String id = parameters['id'] as String; |
There was a problem hiding this comment.
Same question final String and as String only one needed?
There was a problem hiding this comment.
answered lower down.
| 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; |
There was a problem hiding this comment.
Same question on these two too.
There was a problem hiding this comment.
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.
| } | ||
| 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; |
There was a problem hiding this comment.
Same question String and as String?
There was a problem hiding this comment.
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.
|
|
No description provided.