Inspector Implementation #120

jacob314 · 2019-01-15T04:38:25Z

Sorry about the large size. Good news is a few of the files are just additional clones from flutter and there are tests.

jacob314 · 2019-01-15T16:20:24Z

High level notes:
The important files to review are InspectorController, InspectorTree, and InspectorTreeCanvas.

InspectorController is a port of the InspectorPanel class from the Java code base with refactors to make it a true controller. InspectorController tests can be run with a regular dart vm.

InspectorTree holds most of the functionality for a very inspector specific tree renderer.
InspectorTreeCanvas holds the most important implementation that renders to Canvas
InspectorTreeHtml holds a prototype implementation that renders to HTML. It is generally there to ensure the code is abstracted plausible. In the future we will have and InspectorTreeStatefulWidget as well.

Renamed DiagnosticsNode to RemoteDiagnosticsNode as I wanted to pull in the real local DiagnosticsNode class from Flutter as it is really handy for ascii art string dumps for tests.

devoncarew · 2019-01-15T16:43:53Z

I may take a few passes through this PR, but will defer to @kenzieschmoll for a full review. Can you post some screenshots of the UI in the meantime?

jacob314 · 2019-01-15T17:53:53Z

Clicking around in the tree also works with scrolling behavior matching intellij. Expand and collapse also works.

Auto-collapse toolbar:

Known issues:
Need to suppress line art for the single child + zero property case.
Need to make tooltips hovering over the "Default icon work"
Need to calculate tree width correctly

Screenshot showing the HTML renderer which allows multiline property values. The html renderer is off by default but will be used for displaying errors in the logging panel at minimum.

devoncarew · 2019-01-15T17:59:22Z

👍 screenshots look great

kenzieschmoll · 2019-01-15T18:30:01Z

lib/inspector/diagnostics_node.dart


  /// Reference the actual Dart DiagnosticsNode object this object is referencing.
-  InspectorInstanceRef getDartDiagnosticRef() {
+  InspectorInstanceRef get dartDiagnosticRef {
    return InspectorInstanceRef(json['objectId']);


Can you inline this?

InspectorInstanceRef get dartDiagnosticRef => InspectorInstanceRef(json['objectId']);

I wanted to but the line length is above 80 and => blocks shouldn't wrap.

lib/inspector/diagnostics_node.dart

lib/inspector/inspector.dart

devoncarew

This is really, really fantastic! I made a quick pass through; comments in-line.

devoncarew · 2019-01-15T18:01:24Z

lib/inspector/diagnostics_node.dart

@@ -725,12 +657,12 @@ class InspectorSourceLocation {
  final Map<String, Object> json;
  final InspectorSourceLocation parent;

-  String getPath() {
+  String get path {


devoncarew · 2019-01-15T18:02:13Z

lib/inspector/inspector.dart

+      : super(
+          name: 'Inspector',
+          id: 'inspector',
+          iconClass: 'octicon-device-mobile',


no trailing comma may make this particular instance format better?

Without the trailing comma, dartfmt generates

InspectorScreen() : super( name: 'Inspector', id: 'inspector', iconClass: 'octicon-device-mobile');

which I feel makes the end paren less obvious.
As an aside, I would personally be a fan of setting the line length to 100 for this code base
in case this line would just be a natural one liner.

devoncarew · 2019-01-15T18:35:34Z

lib/inspector/inspector.dart

+          div()..flex(),
+          div(c: 'btn-group collapsible')
+            ..add(<CoreElement>[
+              ServiceExtensionButton(extensions.performanceOverlay).button,


possibly put these into ~2 separate button groups, instead of having 7 buttons in a row?

done. Split into 3 groups and got a little clever about having the overflow buttons shrink down to just icons before the more important icons

devoncarew · 2019-01-15T18:37:37Z

lib/inspector/inspector.dart

+    inspectorContainer.add(elements);
+    splitterSubscription = flexSplitBidirectional(elements);
+
+    // TODO(jacobr): update visibility based on whether the screen is visible.


When you do get ready to do this, we have entering() and exiting() calls you can override for Screens.

devoncarew · 2019-01-15T18:39:34Z

lib/inspector/inspector_service.dart

+      await group.dispose();
+      return null;
+    }
+    // TODO(jacobr): use the list of loaded scripts to determine the appropriate


I think Isolate.rootLib is what you want - I don't think you'll need the full list of scripts (it's also available now however, as part of the new getScripts() call).

I've updated the todo to use Isolate.rootLib. TLDR; I need to cleanup the --track-widget-creation transformer to stop using absolute paths.
@terrylucas maybe you are interested in helping fix that in the transformer as you need to tweak the transformer code for some memory tracking items.

devoncarew · 2019-01-15T18:40:43Z

lib/inspector/inspector_tree_canvas.dart

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+library inspector_tree_canvas;


I'm curious if we need library directives? I normally only add them if I need someplace to hang dartdoc comments off of.

no idea. I've been adding them out of habit. Removed the low value ones.

devoncarew · 2019-01-15T18:45:48Z

web/index.html

@@ -21,7 +21,7 @@
    <link rel="stylesheet" href="styles.css">
    <link rel="stylesheet" href="packages/devtools/debugger/debugger.css">
    <link rel="stylesheet" href="packages/devtools/logging/logging.css">
-
+    <link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons">


Note that all the rest of our deps are in-lined into the package - that they could be served w/o an internet connection (including the octicons icons). Perhaps open an issue to in-line the material icons font?

I can't seem to make a comment down there, but right after , can you add a place for the Inspector page, and remove the one for the Device page? We have an approximation of the final UI in static html so that there's less flashing and re-layout when the app launches.

Good catch. I'd thought that part of static HTML was cruft to cleanup rather than server side rendering./

Done. Loaded the flutter icons locally.

devoncarew · 2019-01-15T18:47:21Z

web/styles.css

@@ -232,6 +232,35 @@ input:disabled+label {
    opacity: 0.5;
 }

+.inspector-tree {


I'd create an inspector specific style sheet, lib/inspector/inspector.css, and move things into there that won't be shared. We do this for the debugging and logging pages, just so this css file doesn't grow unbounded.

devoncarew · 2019-01-15T18:48:04Z

web/styles.css

@@ -651,6 +680,26 @@ nav.menu li {
    color: #ccc;
 }

+/*
+We have to do something with the scrollbars as they flicker due to DOM


Do we want this for all of our scrollbars or just the inspector ones?

this impacts the infinite scrolling table as well so yes.
We can tweak this style so we still fade in and out on hover but I think this is at least a non-flickery default.

devoncarew · 2019-01-15T18:48:16Z

web/styles.css

@@ -686,3 +735,23 @@ nav.menu li {
    bottom: 30px;
    z-index: 10;
 }
+
+@media all and (max-width: 1280px) {


👍 this is great

devoncarew

More comments (around testing)!

devoncarew · 2019-01-15T18:58:51Z

lib/debugger/debugger.dart

@@ -1041,6 +1041,7 @@ class BreakpointsView {

  Stream<Breakpoint> get onDoubleClick => _items.onDoubleClick;

+  @override


👍 ; there are ~4 more places in this file I believe where we could make the same change

Great. I've updated them all to implement the CoreElementView interface.

devoncarew · 2019-01-15T19:02:22Z

test/inspector_controller_test.dart

+      expect(
+        detailsTree.toStringDeep(),
+        equalsIgnoringHashCodes(
+          '▼[S] Scaffold  <-- selected\n'


Should we keep this golden output in a txt file?

I'm torn about this. On one hand a text file makes rebaselining easy but on the other hand inline makes the intent of the test much clearer. As a middle ground I plan to fixup the output here so most of these tests are truncated output only highlighting the relevant details so rebaselining won't be needed much or at all and the forest is easier to see from the trees.
note that there are a number of inline golden style text output tests in flutter proper and they rarely break.

devoncarew · 2019-01-15T19:02:54Z

test/inspector_controller_test.dart

+      expect(
+        detailsTree.toStringDeep(),
+        equalsIgnoringHashCodes('▼[S] Scaffold \n'
+            '│   ▼state: ScaffoldState#00000(tickers: tracking 1 ticker)\n'


perhaps this output as well (in a separate file), given the size?

this dump will be made a pleasant size by the tweaks alluded to above.

correction. added reasonably nice golden file and rebaseline goldens functionality and used it i all the details cases. ended up adding a more verbose golden format that include styles as well and used it to catch a bug where we were emitting some pointless styled empty blocks.

devoncarew · 2019-01-15T19:04:46Z

test/matchers/fake_flutter_matchers.dart

@@ -0,0 +1,438 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.


For some of these files, are there cooresponding Java file in the flutter-intellij repo (or dart files in the flutter repo) that we should be keeping them up-to-date with? If so, we should call that out in comments in order to help future maintainers.

devoncarew · 2019-01-15T19:05:26Z

test/inspector_service_test.dart

+import 'dart:async';
+import 'dart:io';
+
+import 'package:devtools/inspector/diagnostics_node.dart';


organize imports?

devoncarew · 2019-01-15T19:05:51Z

test/inspector_controller_test.dart

+import 'dart:async';
+import 'dart:io';
+
+import 'package:devtools/inspector/flutter_widget.dart';


organize imports?

devoncarew · 2019-01-15T19:07:04Z

test/inspector_service_test.dart

+        expect(
+          treeToDebugString(nodeInDetailsTree),
+          equalsIgnoringHashCodes('MaterialApp\n'
+              ' │ state: _MaterialAppState#00000\n'


have a separate golden master file for this test expectation?

devoncarew · 2019-01-15T19:08:10Z

test/inspector_service_test.dart

+import 'package:devtools/inspector/flutter_widget.dart';
+import 'package:test/test.dart';
+
+import 'package:devtools/globals.dart';


It looks like some portion of the devtools codebase will need to run in the VM (not an issue, just noting it). I expect that the tests on travis will keep us honest, if we start referencing dart:html code from portions of the app that need to be able to run on the cli.

yes. these tests along with the one @kenzieschmoll already added keep us honest about not adding dart:html imports to our business logic code which is a great thing. When I'd first added the inspector code but let a few dart:html tests sneak in, @kenzieschmoll's tests started failing locally with complains about my deart:html deps.

devoncarew · 2019-01-15T19:13:20Z

lib/inspector/inspector.dart

+      detailsInspectorTree.element.element
+    ];
+    inspectorContainer.add(elements);
+    splitterSubscription = flexSplitBidirectional(elements);


I would use defaultSplitterWidth for the splitter width. I see that this version of the method doesn't take preferred ratios for the child sizes, but if we can add it you may try out a 40/60 or 33/66 split (see if giving more weight to the RHS looks good).

added default size options for the bidirectional splitter which is a bit tricky as it can be both horizontal and vertical depending on the window size.
Agreed that a 60/40 split looks much better. Use 65/35 split in the other direction in the vertical layout case as in that case the details panel should be smaller not larger.

lib/inspector/inspector.dart

lib/inspector/inspector_controller.dart

lib/inspector/inspector_tree_canvas.dart

lib/ui/fake_flutter/diagnosticable.dart

kenzieschmoll · 2019-01-15T22:11:51Z

lib/ui/fake_flutter/diagnosticable.dart

+      case DiagnosticsTreeStyle.truncateChildren:
+        return null;
+    }
+    return null;


Could put this in a default case.

this code isn't for review here. it is just forked from package flutter so ignoring your valid comments :)

kenzieschmoll · 2019-01-15T22:24:13Z

lib/ui/fake_flutter/print.dart

+      _debugPrintBuffer.isNotEmpty) {
+    final String line = _debugPrintBuffer.removeFirst();
+    _debugPrintedCharacters +=
+        line.length; // TODO(ianh): Use the UTF-8 byte length instead


Probably remove this todo for ianh@?

I think there is value for us just porting files wholesale instead of trying to cleanup as these files will be ported more cleanly at some later point for hummingbird. all we want for them now is that we can reasonably bring in new versions from package:flutter without odd diffs.

test/inspector_controller_test.dart

kenzieschmoll

LGTM

this matches intellij ui and solves problem where render objects in tests had different dimensions than on the bots.

DanTup · 2019-01-21T16:14:26Z

fureweb-com · 2019-01-21T21:14:50Z

wow!

jacob314 requested review from devoncarew and kenzieschmoll January 15, 2019 04:38

kenzieschmoll reviewed Jan 15, 2019

View reviewed changes

lib/inspector/diagnostics_node.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Jan 15, 2019

View reviewed changes

lib/inspector/diagnostics_node.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Jan 15, 2019

View reviewed changes

lib/inspector/inspector.dart Outdated Show resolved Hide resolved

devoncarew reviewed Jan 15, 2019

View reviewed changes

kenzieschmoll reviewed Jan 15, 2019

View reviewed changes

jacob314 force-pushed the inspector_review branch from 6419b3c to 4bda7f0 Compare January 16, 2019 04:42

jacob314 added 5 commits January 16, 2019 11:26

Inspector Implementation

f13f333

Code review comment fixes.

1275bc5

Code review comment fixes.

142e0cf

Code review comment fixes.

75149f1

Remaining code review comment fixes.

9927ced

kenzieschmoll approved these changes Jan 16, 2019

View reviewed changes

jacob314 added 2 commits January 16, 2019 11:31

Fix build.

844d0de

Fix tests.

de15da8

jacob314 force-pushed the inspector_review branch from 7717d57 to de15da8 Compare January 17, 2019 04:05

jacob314 added 3 commits January 16, 2019 20:49

More fixes.

cb0515c

fix bug where render object nodes were expanded

e13f3ec

this matches intellij ui and solves problem where render objects in tests had different dimensions than on the bots.

Strip out controller test hack that isn't needed anymore.

61e129a

jacob314 merged commit e7f6f36 into flutter:master Jan 17, 2019

jacob314 deleted the inspector_review branch January 17, 2019 17:17

		@@ -1041,6 +1041,7 @@ class BreakpointsView {

		Stream<Breakpoint> get onDoubleClick => _items.onDoubleClick;

		@override

		@@ -0,0 +1,438 @@
		// Copyright 2019 The Chromium Authors. All rights reserved.

Inspector Implementation #120

Inspector Implementation #120

Conversation

jacob314 commented Jan 15, 2019

jacob314 commented Jan 15, 2019

devoncarew commented Jan 15, 2019

jacob314 commented Jan 15, 2019

devoncarew commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll left a comment

Choose a reason for hiding this comment

DanTup commented Jan 21, 2019

fureweb-com commented Jan 21, 2019