Allow wrapping container with container

[Kavantix]

Currently the Wrap with Container refactoring works on all widgets except the Container widget itself.
Could we allow this to be done?
There are valid cases for doing this
I'm currently using Wrap with Container in vim in favor of Wrap with widget... since that inserts widget( which is seen as an error since widget is not a valid function or class

[mkustermann]

@Kavantix This sounds like a flutter specific issue. Could you maybe file this bug in the flutter repository instead: flutter/flutter ?

Or is this related to some editor / analyzer refactoring tools?

[Kavantix]

Well yes it is flutter specific, but as far as I could tell this repo is the one implementing the refactoring (also for flutter).
If you still feel I should open it there I would gladly do so

[mkustermann]

/cc @devoncarew Maybe you could correctly triage this issue?

[devoncarew]

Yup, this is the right repo :)

We don't show Wrap with Container on a container itself as I believe that's an anti-pattern - the attributes for the two could just be collapsed down into a single container. Can you talk about your uses cases?

[Kavantix]

Well the most important one for me personally is that I use it to wrap a widget with any other widget since wrap with widget gives me an error until I put the actual widget there.

As for a valid usecase of two containers, I agree that 99% of the time you should simply collapse them into one but I could imagine someone to for instance do some color, padding and alignment on the outer container and a different color and some constraints on the inner container.

So TLDR the usecases are extremely limited, I would just like to have it so I can wrap a container with a widget without the analysis server showing me an error because widget is not a function.

[devoncarew]

Ah, ok.

I'm currently using Wrap with Container in vim in favor of Wrap with widget... since that inserts widget( which is seen as an error since widget is not a valid function or class

I think this is a limitation of the current emacs / analysis server integration. For the refactoring, we pass the text to insert as well as a selection range after the insertion is complete. For other IDEs (VS Code, IntelliJ), the widget text will be selected, and you'll be able to type over the text with the actual name that you want.

Can you re-file this issue as an enhancement against whichever emacs / dart integration you're using?

[Kavantix]

I’m not using emacs, I use coc.nvim in nvim and it does also select the text which I can type over just like vscode and such.
Also I believe I noticed this in vscode with previewLsp as well but I’ll have to test that tomorrow to be sure

If adding wrap with container on a container won’t happen (which I feel like you’re inferring?) then indeed somehow we need to be able to ignore the error until the user entered the name of the widget.
Do you perhaps know if the vscode and intellij plugins explicitely handle this?

Btw, I just wanna say as well that it’s really nice that the lsp version is getting some more love lately, enabling things like the completeFunctionCalls snippet in vim 🎉

[Kavantix]

As you can see in the image in vscode on the previewLsp setting it also shows an error because widget is not a function (or actually even worse dynamic cannot be assigned to type widget.
So this is not isolated to the integrations I would say and also for vscode it is an issue.

Btw I also noticed that this one in fact is not given as a snippet so it does not allow you to put the name of the widget in (both in vscode and the vim extension). I probably confused it with another autocomplete.
@devoncarew shall I create another issue here to address the fact that the refactor is not a snippet? (this seems to be the case for all refactors, so also for extract widget and such, and thus also doesn't work in vscode in lsp mode)

[devoncarew]

I’m not using emacs, I use coc.nvim in nvim

Oops, sorry, I read vim but typed emacs.

it does not allow you to put the name of the widget in (both in vscode and the vim extension)

Hmm, I do see the widget text selected when I try this feature, for both IntelliJ and VS Code.

What version of flutter are you using? I'm running against the latest from master. I think the error after completion is expected - widget isn't defined, and the user will need to enter a widget name to remove the error. I think that you're hitting a problem we've since addressed, which is that the error range reported is for the entire multi-line expression instead of the single widget token. That makes the analysis error around the text 'widget' much more of a UX problem.

[Kavantix]

I'm also using the latest master and both the big multi-line error and the widget being selected don't work

It does work if vscode is not in previewLsp mode, but when it uses the lsp what I posted before happens and the widget part does not get selected

@DanTup FYI, maybe you also have some info on this?

[DanTup]

There's a limitation in the LSP spec that prevents us from pre-selected text from code-actions. This means the widget text is not selected when using LSP (and the same will be true for other clients using the LSP server).

There are some open issues requesting support in LSP here that you can 👍

• Text Snippets not available for CodeAction microsoft/language-server-protocol#592
• Allow CodeActions to specify cursor position microsoft/language-server-protocol#724

There are comments saying that VS Code doesn't support this, which is why LSP doesn't (things are only added to LSP when there is a client that can use them). In VS Code when LSP is disabled, we used a different way of applying edits that uses a command that could call editor.insertSnippet so not being able to do it directly from code actions wasn't an issue.

It could be handled via a custom command, but it would need specific support in each client so that's really a last-resort option. Having something added to the LSP spec would be much better because it wouldn't require editors to implement Dart-lsp-specific commands.

I added a note on microsoft/language-server-protocol#724 (comment) to see whether it's likely there will be movement on it, and also suggesting a possible simple way of providing a selection-range without needing full snippet support.

As for the long range, it sounds like #44598 but that should be fixed on Flutter master. Can you confirm the exact error message and error code being reported when you see this?

[Kavantix]

Yes you're right the long range is indeed improved on master, not sure why I didn't see it before.

I'll thumbs up the other issues.

But as for this exact issue, it having giving the error would simply be solved if we could wrap every widget with a container since that's never an error.
So then I come back to my original question, can we re-enable wrapping container with container (even maybe with a setting for the server or something would be fine)

[DanTup]

@Kavantix I'm not sure I entirely understand the request. If you're planning on wrapping with a different widget, what's the value in using Wrap with Container over Wrap with widget? You mentioned this causes an analysis error, but if I understand correctly, that's only temporary until you type the actual widget name in (although even using Container there will be a temporary error as you start replacing its name until you've fully-typed the other widgets class name).

I think it would be preferable to improve Wrap with widget than enable Wrap Container with Container. Although LSP doesn't support this, it turns out the Rust team have implemented their own snippet extension to LSP which is also supported by a few editors now, so might be a bad idea to support in the Dart LSP server as long as it can be in a way that wouldn't break if LSP officially adopted it in future.

[Kavantix]

Well yeah, the short period in which it shows an error is not needed and bothers me personally, and in vim the analysis only updates when I leave inset mode so if I’m swapping out Container for another widget name I never see any error.

I do agree though that improving wrap with widget is another good and perhaps better option.
Before jumping to something like the codeaction snippet extension we should probably first consider a similar approach as the extract actions (specifying the name of the widget before applying the action)

[DanTup]

This means the widget text is not selected when using LSP (and the same will be true for other clients using the LSP server).

FWIW, the Dart LSP server (and VS Code extension) implement an experimental custom extension to LSP proposed by the Rust team to now select the "widget" text after using the Wrap assist:

https://github.com/rust-analyzer/rust-analyzer/blob/b35559a2460e7f0b2b79a7029db0c5d4e0acdb44/docs/dev/lsp-extensions.md#snippet-textedit

It's not officially part of LSP, but it's possible it would be merged in the future and it's used by at least Dart and Rust, so it might be something to propose for the LSP client you're using.