Evaluation HoverCard #2746

grouma · 2021-02-26T18:55:03Z

Create new HoverCard class to handle absolutely positioned overlays that automatically remove themself
Update CodeView to support hovering over source code and display an evaluation result

kenzieschmoll · 2021-02-26T20:55:31Z

DBC: Is this the card placement we want? I would think we'd want the card to be centered above the mouse position or below. I think Chrome devtools places the hover card above. IntelliJ places it below like this:

grouma · 2021-02-26T20:57:03Z

DBC: Is this the card placement we want? I would think we'd want the card to be centered above the mouse position or below. I think Chrome devtools places the hover card above. IntelliJ places it below like this:

I'm open to changing it. I was making this consistent with the current hover logic in the memory page. Let me know if you prefer consistency or the Chrome logic.

kenzieschmoll · 2021-02-26T20:58:32Z

DBC: Is this the card placement we want? I would think we'd want the card to be centered above the mouse position or below. I think Chrome devtools places the hover card above. IntelliJ places it below like this:

I'm open to changing it. I was making this consistent with the current hover logic in the memory page. Let me know if you prefer consistency or the Chrome logic.

Yes let's be consistent with chrome devtools or with the IDEs for debugging purposes.

grouma · 2021-02-26T21:17:52Z

Yes let's be consistent with chrome devtools or with the IDEs for debugging purposes.

Done.

kenzieschmoll · 2021-03-02T18:17:14Z

packages/devtools_app/lib/src/debugger/codeview.dart

+  _CodeViewState(this.debuggerController);
+
+  final DebuggerController debuggerController;


access controller via widget.controller instead of passing it into the state object

And actually, since CodeViewState is a stateful widget, we can access the controller from Provider in didChangeDependencies, and we don't need to pass debuggerController into the widget constructor either. See

devtools/packages/devtools_app/lib/src/debugger/debugger_screen.dart

Line 91 in 4456f81

final newController = Provider.of<DebuggerController>(context);

for an example of that

why not just lookup the Provider in the build method?

kenzieschmoll · 2021-03-02T18:21:59Z

packages/devtools_app/lib/src/debugger/codeview.dart

  const LineItem({
    Key key,
    @required this.lineContents,
+    @required this.debuggerController,


access debugger controller from didChangeDependencies instead of requiring it to be a parameter for this stateful widget (see above comment for example)

why didUpdateDependencies instead of the build method?

packages/devtools_app/lib/src/debugger/codeview.dart

kenzieschmoll · 2021-03-02T18:25:17Z

packages/devtools_app/lib/src/debugger/codeview.dart

+      final theme = Theme.of(context);
+      _hoverCard?.remove();
+      final word = wordForHover(
+          event.localPosition.dx, widget.lineContents, theme.fixedFontStyle);


nit: trailing comma

kenzieschmoll · 2021-03-02T18:25:42Z

packages/devtools_app/lib/src/debugger/codeview.dart

+              contents: Material(
+                  child: ExpandableVariable(
+                      debuggerController: widget.debuggerController,
+                      variable: ValueNotifier(variable))),


trailing commas

kenzieschmoll · 2021-03-02T18:25:53Z

packages/devtools_app/lib/src/debugger/codeview.dart

+                      debuggerController: widget.debuggerController,
+                      variable: ValueNotifier(variable))),
+              event: event,
+              width: 250,


put this into a const

kenzieschmoll · 2021-03-02T18:26:00Z

packages/devtools_app/lib/src/debugger/codeview.dart

+              event: event,
+              width: 250,
+              title: word,
+              context: context);


trailing comma

kenzieschmoll · 2021-03-02T18:26:31Z

packages/devtools_app/lib/src/debugger/codeview.dart

-                child:
-                    RichText(text: truncateTextSpan(lineContents, column - 1)),
+                child: RichText(
+                    text: truncateTextSpan(widget.lineContents, column - 1)),


trailing comma

kenzieschmoll · 2021-03-02T18:26:47Z

packages/devtools_app/lib/src/debugger/codeview.dart

+                widget.lineContents,
+                scrollPhysics: const NeverScrollableScrollPhysics(),
+                maxLines: 1,
+              )),


trailing comma

kenzieschmoll · 2021-03-02T18:27:05Z

packages/devtools_app/lib/src/debugger/codeview.dart

+            widget.lineContents,
+            scrollPhysics: const NeverScrollableScrollPhysics(),
+            maxLines: 1,
+          ));


kenzieschmoll · 2021-03-02T18:27:53Z

packages/devtools_app/lib/src/debugger/codeview.dart

-        scrollPhysics: const NeverScrollableScrollPhysics(),
-        maxLines: 1,
-      );
+      child = MouseRegion(


this is duplicated code. Can you pull out into a local var and use in both places?

kenzieschmoll · 2021-03-02T18:29:25Z

packages/devtools_app/lib/src/debugger/variables.dart


  @override
  Widget build(BuildContext context) {
    return ValueListenableBuilder<Variable>(
      valueListenable: variable,
      builder: (context, variable, _) {
-        final controller = Provider.of<DebuggerController>(context);
+        final controller =
+            debuggerController ?? Provider.of<DebuggerController>(context);


Is this because we don't have a debugger controller available in the context of the hover card overlay?

correct. Overlay doesn't play great with provider. We might be able to resolve if we moved our providers to the absolute root of the tree but that would cause problems for the snapshot app case.

kenzieschmoll · 2021-03-02T18:30:22Z

packages/devtools_app/lib/src/debugger/hover.dart

+/// Merges words linked by a `.`.
+///
+/// For example, hovering over `bar` in `foo.bar.baz` would return
+/// `foo.bar.baz`.


nit: should this be foo.bar instead of foo.bar.baz?

No this is correct.

can you explain that rationale to me? If a user hovers over bar, wouldn't they want to look at the value of bar, not baz?

I thought Chrome's behavior was to treat all things linked by a . as a single "block".

I just tested and that doesn't seem to be the case:

I'll update the logic.

kenzieschmoll · 2021-03-02T18:33:46Z

packages/devtools_app/lib/src/debugger/hover.dart

+/// For example, hovering over `bar` in `foo.bar.baz` would return
+/// `foo.bar.baz`.
+String _mergedLinkedWords(String word, int index, TextSpan line) {
+  var left = index - 1;


how is the index = 1 case handled? If we are hovering over bar in foo.bar, don't we want to evaluate foo.bar? This looks like we only merge the words if there are two previous values. We should be able to change this to chain previous values one at a time instead of 2 at a time.

same comment for the right case

We want to evaluate all words linked by a .. This is consistent with Chrome's behavior.

index in this case would point to bar and foo would be merged. There are test cases covering this.

I updated the logic and the tests. If we hover over bar in foo.bar.baz we want to evaluate foo.bar to be consistent with Chrome. There will always be at least two items because one of the items will be .. Otherwise it won't be linked.

kenzieschmoll · 2021-03-02T18:35:29Z

packages/devtools_app/lib/src/debugger/hover.dart

+                _hasMouseEntered = true;
+              },
+              child: Container(
+                padding: const EdgeInsets.fromLTRB(8, 8, 8, 8),


use const EdgeInsets.all(denseSpacing);

kenzieschmoll · 2021-03-02T18:36:39Z

packages/devtools_app/lib/src/debugger/hover.dart

+                    color: focusColor,
+                    width: _hoverCardBorderWidth,
+                  ),
+                  borderRadius: BorderRadius.circular(10.0),


put this into a const or use the defaultBorderRadius used elsewhere in devtools

kenzieschmoll · 2021-03-02T18:37:20Z

packages/devtools_app/lib/src/debugger/hover.dart

+                child: Column(
+                  crossAxisAlignment: CrossAxisAlignment.start,
+                  children: [
+                    Container(


use SizedBox instead since width is the only property

kenzieschmoll · 2021-03-02T18:37:38Z

packages/devtools_app/lib/src/debugger/hover.dart

+                    Container(
+                      width: width,
+                      child: Text(
+                        title,


what if the title is long? should this ellipsize?

kenzieschmoll · 2021-03-02T18:37:53Z

packages/devtools_app/lib/src/debugger/hover.dart

+                    contents,
+                  ],
+                ),
+              )));


trailing commas

packages/devtools_app/lib/src/debugger/hover.dart

kenzieschmoll

Done with first round of comments

jacob314 · 2021-03-02T21:49:56Z

packages/devtools_app/lib/src/debugger/codeview.dart

@@ -39,10 +43,14 @@ class CodeView extends StatefulWidget {
  final void Function(ScriptRef scriptRef, int line) onSelected;

  @override
-  _CodeViewState createState() => _CodeViewState();
+  _CodeViewState createState() => _CodeViewState(controller);


https://dart-lang.github.io/linter/lints/no_logic_in_create_state.html
will avoid this issue without code review. lets add the lint to devtools.

jacob314 · 2021-03-02T21:59:31Z

packages/devtools_app/lib/src/debugger/hover.dart

+    final prev = line.children[left].toPlainText();
+    final prevprev = line.children[left - 1].toPlainText();
+    if (prev == '.') {
+      word = '$prevprev$prev$word';


nit: one way to write this logic a little bit more simply is to create a
List that is the components of the line.
Then you can just track the the start and end indexes and use String.join to get the output string when you are done.

grouma · 2021-03-02T23:47:38Z

This should be ready to review again. PTAL.

kenzieschmoll · 2021-03-03T16:23:46Z

grouma added 3 commits February 26, 2021 10:46

Evaluation HoverCard

cc2701c

Merge branch 'master' into hover-card

d44d75e

copyright

52954a2

grouma requested a review from jacob314 February 26, 2021 18:55

lint

fc7a922

grouma added 2 commits February 26, 2021 13:14

center hover

222a784

remove newline

d92ab25

grouma requested a review from kenzieschmoll March 2, 2021 18:14

kenzieschmoll reviewed Mar 2, 2021

View reviewed changes

packages/devtools_app/lib/src/debugger/codeview.dart Show resolved Hide resolved

kenzieschmoll reviewed Mar 2, 2021

View reviewed changes

packages/devtools_app/lib/src/debugger/hover.dart Outdated

contents,

],

),

)));

Copy link

Member

kenzieschmoll Mar 2, 2021

Choose a reason for hiding this comment

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

trailing commas

kenzieschmoll reviewed Mar 2, 2021

View reviewed changes

packages/devtools_app/lib/src/debugger/hover.dart Show resolved Hide resolved

kenzieschmoll reviewed Mar 2, 2021

View reviewed changes

jacob314 reviewed Mar 2, 2021

View reviewed changes

grouma added 2 commits March 2, 2021 15:08

review comments

5fb818c

fix linked logic

a6322cb

trailing

045db58

kenzieschmoll approved these changes Mar 3, 2021

View reviewed changes

grouma merged commit f8e219a into master Mar 3, 2021

grouma deleted the hover-card branch March 3, 2021 16:41

		_CodeViewState(this.debuggerController);

		final DebuggerController debuggerController;

Evaluation HoverCard #2746

Evaluation HoverCard #2746

Conversation

grouma commented Feb 26, 2021

kenzieschmoll commented Feb 26, 2021

grouma commented Feb 26, 2021

kenzieschmoll commented Feb 26, 2021

grouma commented Feb 26, 2021

Choose a reason for hiding this comment

kenzieschmoll Mar 2, 2021 • edited Loading

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

grouma Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll Mar 2, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grouma commented Mar 2, 2021

kenzieschmoll commented Mar 3, 2021

kenzieschmoll Mar 2, 2021 •

edited

Loading

grouma Mar 2, 2021 •

edited

Loading

kenzieschmoll Mar 2, 2021 •

edited

Loading