Dart-code: Feedback on "closing labels" preview feature

[DanTup]

v2.3.1 includes a preview feature enabled with the dart.previewFlutterCloseTagDecorations: true setting. This adds "closing tag" decorations to the closing parens of constructor calls within build methods.

This feature is behind a flag because it's incomplete, has sub-optimal performance and may not work reliably. It is being shipped solely for feedback on the idea and may require analyzer work in order to support in a reliable way.

Please post your feedback below!

Known issues:

These are things that (probably) can't be addressed without support from the analyzer.

• [x] [Perf] Doesn't scale well due to reliance on Outlines, Highlights and repeated getHover calls
• [x] Currently identifies build methods based on return type string (Widget)
• [x] Only shows constructors, doesn't show end of lists (children, actions)

Suggestions:

These are things that can be improved in the preview.

• [x] Support const as well as new
• [x] Prefix with // instead of / so looks a bit like a comment marking the end
• [x] Improve styling
• [x] Fix flicker when typing quickly (or holding keys) caused by not being able relate analyzer notifications with document versions - just keep resetting a timeout and only update after there's been no update for x milliseconds (where x > expected latency for the update)
• [x] Show decorations in any Widget-returning method
• [x] Don't show annotations against characters other than ) to mitigate out-of-sync notifications
• [x] Allow color to be customised rather than reusing an existing style

[sethladd]

Could we recognize "const Icon" ?

[DanTup]

Could we recognize "const Icon" ?

Oops, didn't spot that! Should be easy; currently I'm using the new to detect a constructor, didn't think about const. Obviously with real analyzer support we wouldn't have wonky string checks on keywords, but I'll tweak this in the next release.

[brianegan]

I use IntelliJ, and I'm jealous we don't have this feature! Really cool idea :)

[DanTup]

I can't take credit for it, @sethladd emailed me asking how possible it'd be 😄

I use IntelliJ, and I'm jealous we don't have this feature!

There's a solution to that ;-P

That said; this is behind a flag for a reason.. it has to send many requests to the analyzer to work; it really needs real analyzer support to become reasonable to ship without a flag/warning - whether it gets that will likely depend on the feedback.

[sethladd]

@DanTup rocks for the quick turnaround!

And, @xster was the inspiration.

We have a bunch of VS Code + Flutter users here at Google, we'll ask them to turn on the flag and report feedback. We're also curious if others find this useful. If so, we can make a case to the analyzer team to support (or, help someone land the patch).

Cheers!

[yjbanov]

Another idea, close named arguments with the argument name rather than constructor type (or maybe both?)

Example:

new AlertDialog(
  title: new Text(
    'some'
    'complicated'
    'expression'
  )/title,  // <== this
)

Options:

/title
/title Text

I wonder if parsing Dart code directly using the analyzer API would be faster than sending requests to the analysis server. You shouldn't need resolved AST for this. dartfmt does this and works on a file-by-file basis. Perhaps parse the code (with some debounce strategy), find all NamedExpressions and, if it is multiple lines long, add a closing tag to it.

[yyoon]

How can I install v2.4? I'd love to try this out.

[sethladd]

close named arguments with the argument name rather than constructor type

not a bad idea! That matches how I've used commenting end tags in HTML.

Strawman:

new AlertDialog(
  title: new Text(
    'some'
    'complicated'
    'expression'
  )/Text(title),  // <== this
)

[DanTup]

@yjbanov

Another idea, close named arguments with the argument name rather than constructor type (or maybe both?)

I think both could get noisy, but the argument name could work. Problem is, if it's not named then we'd have to use the type, and then it might be inconsistent. Interested to hear what other people think though!

I wonder if parsing Dart code directly using the analyzer API would be faster than sending requests to the analysis server.

Currently Dart Code is written in TypeScript, so using the analyzer package would likely be non-trivial. I don't think performance is a big deal here; the current prototype seems to perform ok to me, and it's doing TERRIBLE things. If it was made a first-class request in the analyzer, I think it'd easily be fast enough - I mean code-completion talks to the analyzer and works fine (AFAIK) and this is far less time-sensitive than that :-)

@yyoon

How can I install v2.4? I'd love to try this out.

Whoops! I shipped this earlier than planned, so I called it v2.3.1 because there wasn't a lot else in it. The latest version on the marketplace (v2.3.1 - which Code probably already auto-updated to if you have Dart Code installed) has this; just set the setting mentioned at the top. Sorry for the confusion!

[yyoon]

@DanTup I see. Now I can see it working by manually setting it to true. Thanks!

[DanTup]

@yyoon Yep, you need to opt-in to it since it's a bit unfinished and may not perform terribly well for large files. Please let us know what you think when you've tested it a little!

[yyoon]

Found an interesting issue about this feature.

When we have a fairly big build method, it is pretty common for us to define some helper widget builders and use them in the main build method to improve readability / maintainability. For example, as a very simplified example:

Widget _buildHeader(BuildContext context) { ... }

Widget _buildContent(BuildContext context) { ... }

Widget _buildFooter(BuildContext context) { ... }

Widget build(BuildContext context) {
  return new Column(
    children: <Widget>[
      _buildHeader(context),
      _buildContent(context),
      _buildFooter(context),
    ],
  );
}

It looks like the decoration only works within the build method and not in the other helper methods, which seems a bit restrictive. Could we just have this enabled for the entire widget class, or maybe for all the methods returning some kind of Widget?

[xster]

I was gonna say the same thing. Might be too expensive to do but would have loved it to add a closing hint to every object construction that's a subclass of widget.

[DanTup]

@yyoon @xster I've tweaked it to work for any Widget-returning method for the next release. I've also added a 1sec timeout that is reset on each notification to avoid repeated calls (this means there will be a > 1sec delay for updating; but that reduces flickering and at this point I don't think speedy updates are the most important part of testing the idea out).

[DanTup]

v2.3.2 shipped with some tweaks:

• Decorations now appear in Fuchsia projects as well as Flutter projects
• Decorations now appear for any Widget-returning method instead of only methods named build
• Decorations will update less frequently to avoid flickering during quick typing
• Decorations should no longer appear in invalid places (probably)
• Decorations now look more like comments, prefixed with // instead of /
• Decorations now apply to const constructor calls as well as those using new

[xster]

Seth and I just played with it some. It's SUPER useful.
Here's a random piece of code. It would be so much manual labour figuring out what it is otherwise.

Also really impressed that it puts the closing tags in the right places in inline nested parentheses.

Thanks so much for the super quick turn-arounds.

Some unfiltered thoughts:

• Actually really liked that it picked up List constructors too. It would be awesome to get the type parameterization and List literal closing hints too.
• Picking up Widget returning functions is awesome. There's indeed a lot of UI laying-out outside of build() functions. Some are still going to be outside of Widget returning functions too. For instance, widget tests don't return anything but would have big layouts etc. Here's a randomly selected example: https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/tooltip_test.dart. Don't know how feasible it is with the current analyzer. I think ultimately, it would be good to have a closing tag on all Widget subclasses' constructors regardless of where the code is.
• I notice these new hints have an added benefit of helping distinguish, at a glance in big files, UI laying code from other code which is actually really useful in retrospect. Maybe we can help emphasize it with a subtle background highlight on the entire Widget constructing parts of the code (between the constructor and opening parenthesis to the closing one)? There might still be non-UI functions laced inside the Widget constructors but maybe we can just try a simple version for visualization.

[DanTup]

@xster Glad it's working well! Nice to see some big examples too, looks much more useful than in my trivial methods!

Actually really liked that it picked up List constructors too. It would be awesome to get the type parameterization and List literal closing hints too.

Yeah, I'd like to do this but I don't know how feasible it'd be without real analyzer support. I'm currently merging results from outlines and highlights and the data is rather limited. I'll have a scan through this week to see if I can improve though.

Some are still going to be outside of Widget returning functions too. For instance, widget tests don't return anything but would have big layouts etc.

Again, without proper support I think it'll be hard to handle this (short of just doing all constructors everywhere; but performance may suck given we currently have to send a getHover request for each one).

I think ultimately, it would be good to have a closing tag on all Widget subclasses' constructors regardless of where the code is

I agree! If implemented in the analyzer, it probably has really easy access to knowing the real types of things (inc sub-classes, etc.) whereas currently I'm having to do string checks for things like returnType == "Widget" ;(

Maybe we can help emphasize it with a subtle background highlight on the entire Widget constructing parts of the code (between the constructor and opening parenthesis to the closing one)?

If you hover your mouse over a constructor name, it should already highlight the whole region? I don't know if I could show this all the time (it's Code's behaviour based on the region returned from the getHover call) and without some gradiant of colours they'd all just blur into each other when nested?

Keep the feedback coming :-)

[dvdwasibi]

@DanTup I started using this today at work, and it has already made it so much easier to work with large widget trees. Thanks!

One bit of feedback: it would be nice to specify the style (color) of the closing tags.

[DanTup]

One bit of feedback: it would be nice to specify the style (color) of the closing tags.

Agreed! I was trying to reuse existing style names (so they could have nice defaults) but probably that's too restrictive. I think I can just give it a new name and then it can be edited in the usual theming settings for Code.

[yyoon]

Used this for a couple of days, and I find this feature really useful!
Much easier to read / modify our existing flutter code with nested widgets.

As other people mentioned already, being able to see the end of a List of Widgets (very commonly used in any Row, Coloum, ListView, etc.) would be even better.

[DanTup]

Thanks for all of the feedback! I've added support for this in the Analyzer which means much better performance and support for Lists. It will ship in an upcoming Dev SDK and Dart Code will be updated to switch to that implementation when it's available. Initially, it will remain behind a flag to get some more feedback (since it's currently being applied everywhere, not just Flutter widget code).

There's also been some tweaks to the placement - labels will now always appear at the end of the line, stacking up when there are multiple on the same line (like // Row // Container).

I'll post back when there's a Dev SDK and a Dart Code build available for testing.

[DanTup]

@dvdwasibi

it would be nice to specify the style (color) of the closing tags.

I've added this (#408) for the "real" version (coming soon) though I'm hoping to get confirmation from MS that the way I did it is wonky and there's a better way =)

It'll be done by setting a theme colour named dart.closingLabels. It'll default to the colour of tab.inactiveForeground which is the one shown in my last screenshot above (and should have a similarly reasonable default for light themes).

Colours are customised by adding workbench.colorCustomizations to your settings (or using a theme which has it; which is none!):

"workbench.colorCustomizations": {
    "dart.closingLabels": "#ff0000"
}

[sethladd]

Really cool, thanks so much!

[DanTup]

I've shipped Dart Code v2.4.0. This includes an implementation of "closing tags" that uses the analysis server (you must be using at least 1.25.0-dev.11.0 which was released yesterday; flutter is also using it if you update).

It's still behind a flag (though there's a new flag name dart.previewClosingLabels since it's not flutter-specific - though the old flag still works) to get some more feedback (it now appears for all Dart!).

Please try it out :-)

[Skylled]

In its SDK-supported form, I love that the tags show up everywhere, including useful places like setState calls, and inside of functions that don't necessarily return a widget, but do work with widgets. It's one of those subtle tools that you won't realize how helpful it is until its gone.

[xster]

Minor UI issue: noticed it was possible to momentarily type text behind the labels.

[DanTup]

I've been away for two weeks and thought I'd return to some feedback on these labels appearing in twoo many places, but there hasn't been any. For anyone that's used this seriously, does it seem reasonable to drop the flag and ship to everyone?

(I will first investigate @xster's bug, which I've opened #423 for).

[sethladd]

We showed it off at GDD Europe :)

[DanTup]

@sethladd Neat! Was there any feedback? (or was it in a recorded demo?).

@devoncarew You had concerns it might appear in too many places - have you had chance to try it out properly and see if it still feels that way?

[devoncarew]

Hey - just cycling back to this now. We've taken a look locally, and do think that it's slightly heavy in the number of places it shows closing tags. We'd need to spot check more code, but at a first blush, list and method closing tags might be too noisy (constructor closing tags are great). For list closing tags, one thing that may help is if the closing tag looks like the declaration (currently it looks like <Widget>[... and // List<Widget>, which is correct from a semantic POV, but it may be easier to line things up with your eye if we change the closing tag to be // <Widget>[]).

We'll do some more investigation here, and when we're further along run it by UXR.

[xster]

My 2 cents so far:

Thank god you made this!

Perhaps the only tweak is that sometimes it visually blends in with actual comments a bit too much. Could we make it a smaller font or give it some other distinguishing visual treatment?

Either way, low priority. I think it's great as is.

[yjbanov]

This is more of a meta-feedback. I'll definitely file feedback about the labels themselves when I see something that hasn't been discussed yet. My feedback is about the feedback banner itself. Is there a way to disable it, or have it pop up only once in a while (e.g. once a week)? I jump from project to project a lot, and it pops up every time.

Also, joining @xster. I 😍 this feature!

[DanTup]

@devoncarew Thanks for the info. Give me a shout when you have some solid changes you want to make (or if easier for for you make them, feel free). I haven't removed the flag from this yet as I thought 1.25 was near and wanted to do a final test with the release SDK before doing so.

@xster Glad you like it! We can customse things, but smaller fonts might look a bit weird. The colour is customisable already, though I don't know if that's enough. It's possible to add borders/underlines/etc., but I think they'll make it less subtle, and probably more subtle is what you're after? If you have ideas, I can try them out and make screenshots (or if you're comfortable running Dart Code from source you can try it yourself - relevant code is this and Code docs on what's possible are here.

@yjbanov Not currently; but I've been meaning to remove it for a while. I saw someone tweet about uninstalling an extension that was doing something like this and I realised how annoying it is. I should've made it only show once :(

If 1.25 SDK ships soon, I'll remove the flag so it'll go anyway. If not, I'll push an update out to remove it soon. Sorry for the annoyance - I haven't used it much myself (been away for 3 weeks) so it hasn't been on my mind ;(

[DanTup]

@yjbanov

My feedback is about the feedback banner itself. Is there a way to disable it, or have it pop up only once in a while (e.g. once a week)?

v2.4.2 removes this banner (it's the only change). Sorry for the annoyance; I really should've made it only appear now and then (esp. since the extension already has some only-shown-once popups!).

[Skylled]

I've finally found something that isn't absolutely perfect about this feature.

When folding a code section that contains a closing tag, the closing tag is added to the front of the fold.

I'm not sure how useful this behavior is, so please correct me if I'm wrong.
My personal expectation is to have the tag hidden until I decide to unfold.

Screenshot

[DanTup]

Yeah, that looks crap. Can you raise a specific issue for it? Dunno if we can easily fix before MS overhaul decorations though 😔

[DanTup]

Closing this as this is done. We still have #445 which people are directed to if they turn closing labels off. Analytics say 99% of people have it enabled.