Dart-code: Wrap with column missing in flutter IDEs

[ryaninja]

I've tried this in both Android Studio and VSCode, but the context menu to wrap in column simply doesn't appear for me. I'm trying to follow a layout tutorial, but I get this error at pretty much the first hurdle. Has this feature been removed/replaced? Anyone got any ideas?

Here's what the guide looks like:
https://i.stack.imgur.com/fpKV2.jpg

And here's what I see:
https://i.stack.imgur.com/yZu5M.jpg

[ryaninja]

Ok, so it turns out you need to right-click and not highlight the widget and then right click. Wrap in column showed as soon as I did this.

[DanTup]

@bwilkerson @scheglov does this seem like a bug? When invoking the assist menu one character from the end of a widget class, Wrap in Row/Column show up. However if the cursor is after the last character, they do not (however the others do). I can't think of a reason for this (nor can I see an obvious reason from the code why this happens):

[bwilkerson]

I can't think of any valid reason for the behavior being different between these two selection locations, so I'd consider this to be a bug.

Note that "Wrap with Row" is also missing. Both it and "Wrap with Column" are implemented in a single method. That method uses SelectionAnalyzer to convert a selection range to a set of AST nodes, and my guess (though I haven't dug into it enough to know for sure) is that that class selects an AST node that the method in question doesn't handle correctly, and that that's the source of the bug.

[DanTup]

Ok, took a quick look at this. When the cursor is inside the name of the widget, analyzer.coveringNode from the SelectionAnalzyer correctly identifies the widget expression. However, when it's like this:

WidgetFoo^('test')

The selection analyzer picks up the argument list ('test') as the innermost covering node (since it starts at that offset), and flutter.identifyWidgetExpression exits early (without walking up parents) when it finds argument lists:

https://github.com/dart-lang/sdk/blob/93313eb2449099e20ade80d4760f76a325a4e176/pkg/analysis_server/lib/src/utilities/flutter.dart#L325-L327

Removing node is ArgumentList from that condition causes this case to work, however it breaks some other tests (for example test_identifyWidgetExpression_node_instanceCreation which is explicitly checking for this behaviour).

I'm not sure what a good fix is. If feels like the cursor being to the left of a paren in an argument list maybe should not be treated as being in the argument list, but that also feels like a weird exception.

[bwilkerson]

A possible short-term work around would be to check in the assists to see whether the covering node is an argument list whose parent is a widget expression. If we add a comment explaining why we're doing it, then we'd have some context in case we decide to change SelectionAnalyzer in the future.

@scheglov I'm interested in your thoughts on this.

[DanTup]

@scheglov do you have any additional thoughts on this? If not, I might have a go at doing what @bwilkerson suggested above. Thanks!

[bwilkerson]

A note on implementation.

In order to better support being able to apply multiple fixes simultaneously, we changed the way we're implementing both fixes and assists. They now use instances of a subclass of CorrectionProducer to build the actual edits. The fixes have all been converted at this point, but most of the assists have not been.

Prior to these changes, fixes and assists each had a slightly different way of computing the node used as the starting point for creating the edits. Usually they produced the same node, but in a few cases they differed. In the new architecture they are consistent. That has meant that sometimes when converting a fix or assist we've had to update the code slightly to adjust for the change in starting node.

So, if you're interested in looking into this, it would be good if you first converted the existing assist to use the CorrectionProducer pattern. It's possible that doing so would impact the way the bug needs to be fixed, and it would be nice not to have to do the conversion after the fix is applied. That said, I won't say to no to getting the bug fixed without the extra work!

[scheglov]

It is mostly a matter of covering reasonable cases.
It is reasonable to propose Wrap in this case Widget^(...).
But we might have to do differently for Widget(..., child^).

[DanTup]

So, if you're interested in looking into this, it would be good if you first converted the existing assist to use the CorrectionProducer pattern.

Looks like you beat me to this :-)

It is reasonable to propose Wrap in this case Widget^(...).
But we might have to do differently for Widget(..., child^).

I think we could specifically handle the case where the caret is at exactly the offset of the argument list (eg. the caret is to the left of the open paren) - coveringNode is ArgumentList && coveringNode.offset == selectionOffset? I've opened a change that does that here:

https://dart-review.googlesource.com/c/sdk/+/152521/

[bwilkerson]

lgtm