You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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?
@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):
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.
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:
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.
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.
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!
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^).
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:
@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):
Yes, that looks like a bug. The question is: does the version of the Flutter SDK that you're using include the fix. It sometimes takes a long time for a fix to make it into a build. Unfortunately, I can't remember how to determine that information, even if I knew which version of the Flutter SDK you're using.
I can reproduce this, but it's slightly different to the original report/fix - in that case the cursor was at the end of ModelBinding (the highlight is just VS code highlighting the references) and that works. However if you select the widget name, Wrap with Column is still missing.
With dart-lang/sdk@8480657, these actions will also work if the whole widget name is selected. The fix is in the SDK so will ship with a future Flutter SDK release (rather than the Dart extension for VS Code).
Activity
ryaninja commentedon May 25, 2020
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 commentedon May 25, 2020
@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 commentedon May 25, 2020
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
SelectionAnalyzerto 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 commentedon May 26, 2020
Ok, took a quick look at this. When the cursor is inside the name of the widget,
analyzer.coveringNodefrom theSelectionAnalzyercorrectly identifies the widget expression. However, when it's like this:The selection analyzer picks up the argument list
('test')as the innermost covering node (since it starts at that offset), andflutter.identifyWidgetExpressionexits 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 ArgumentListfrom that condition causes this case to work, however it breaks some other tests (for exampletest_identifyWidgetExpression_node_instanceCreationwhich 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 commentedon May 26, 2020
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
SelectionAnalyzerin the future.@scheglov I'm interested in your thoughts on this.
DanTup commentedon Jun 11, 2020
@scheglov do you have any additional thoughts on this? If not, I might have a go at doing what @bwilkerson suggested above. Thanks!
bwilkerson commentedon Jun 11, 2020
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
CorrectionProducerto 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
CorrectionProducerpattern. 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 commentedon Jun 11, 2020
It is mostly a matter of covering reasonable cases.
It is reasonable to propose
Wrapin this caseWidget^(...).But we might have to do differently for
Widget(..., child^).DanTup commentedon Jun 25, 2020
Looks like you beat me to this :-)
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/
3 remaining items
mich040m commentedon Feb 23, 2022
Still happening as described here
bwilkerson commentedon Feb 23, 2022
Yes, that looks like a bug. The question is: does the version of the Flutter SDK that you're using include the fix. It sometimes takes a long time for a fix to make it into a build. Unfortunately, I can't remember how to determine that information, even if I knew which version of the Flutter SDK you're using.
DanTup commentedon Feb 24, 2022
I can reproduce this, but it's slightly different to the original report/fix - in that case the cursor was at the end of
ModelBinding(the highlight is just VS code highlighting the references) and that works. However if you select the widget name, Wrap with Column is still missing.[analysis_server] Allow Flutter "Wrap" assists to work with construct…
DanTup commentedon Feb 24, 2022
With dart-lang/sdk@8480657, these actions will also work if the whole widget name is selected. The fix is in the SDK so will ship with a future Flutter SDK release (rather than the Dart extension for VS Code).