Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a "Wrap with builder" assist that works like "Wrap with widget" #1890

Closed
amreniouinnovent opened this issue Jul 24, 2019 · 18 comments
Closed
Labels
fixed in dart / flutter in editor Relates to code editing or language features in flutter Relates to running Flutter apps in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Milestone

Comments

@amreniouinnovent
Copy link

Use case

The refactor menu in VSCode has basic widgets.
I want to be able to wrap my widget in a BlocBuilder widget directly or any other widget other than the default widgets

Proposal

I think there should a JSON format or similar way for adding more widgets to the refactor menu and asks for an array of parameters like bloc state class, bloc event class, etc.

image

@DanTup
Copy link
Member

DanTup commented Jul 24, 2019

Is the request here just to be able to run Wrap with new widget but replace the placeholder widget text with a specific widgets name, or are you trying to also customise the code that is inserted?

@amreniouinnovent
Copy link
Author

amreniouinnovent commented Jul 24, 2019

@DanTup Here is a sample of what I need is having a JSON snippet which has a placeholder like {child}

Theme(
      data:""
          return **{child}**;
        );
BlocBuilder<LoginBloc, LoginState>(
            builder: (BuildContext context, LoginState loginState) {
          return **{child}**;
        });

@DanTup
Copy link
Member

DanTup commented Jul 24, 2019

Ok, I think that complicates things.. When we do "Wrap with new widget" in VS Code, we don't really understand which parts of the code are the child widget - we just send the command off to the analysis server (which ships in the Dart SDK) and it provides us with the edits to apply to the file that will do the wrapping. To achieve what you want, we'd need to know how to extract exactly the child widget in order to merge it with your template (or, we'd need to some potentially brittle string manipulation).

How often do you need to wrap existing code with BlocBuilder, versus writing it from the start? Would a snippet work better?

@DanTup DanTup added the awaiting info Requires more information from the customer to progress label Jul 24, 2019
@amreniouinnovent
Copy link
Author

@DanTup Yes using snippets could do the trick too.
You have used the right term "Template".
If you feel this proposal is too complex you could close the issue... I don't know a lot about the Dart SDK.
I don't often use it to BlocBuilder but it could be a better solution than snippets solution.
Thank you for hardworking in Dart support to VSCode.

@DanTup
Copy link
Member

DanTup commented Jul 24, 2019

I don't often use it to BlocBuilder but it could be a better solution than snippets solution.

What I meant, was would it be more useful to have a snippet that inserts the template you gave above, rather than a refactor? I'm not sure which is more common - writing new code that uses BlocBuilder, or trying to wrap it around existing code. It seems like wrapping it might only be done when converting a project to use Bloc, whereas writing new code you'd probably write it using the BlocBuilder in the first place? (I'm not familiar with Bloc, so I might not understand correctly).

If so, snippets may add more value, and the complexity of this might be less worthwhile. I don't have a good enough understanding to have a solid opinion of that yet though :-)

@amreniouinnovent
Copy link
Author

amreniouinnovent commented Jul 24, 2019

@DanTup wrapping is more common than snippets
when you work on a page you focus on the UI then you add the business logic later
So once the login button is created I will wrap it in a BlocBuilder.

so the sequence is building UI >then wrapping the required parts into builders >then business logic

this solution will work for a lot of cases like:
StreamBuilders "used for firebase wrapping", FutureBuilder, etc builders

Builders are for rendering a specific widget only when an event occurs so it improves the performance.

Sample for the stream builder:
https://api.flutter.dev/flutter/widgets/StreamBuilder-class.html
Sample for the future builder:
https://api.flutter.dev/flutter/widgets/FutureBuilder-class.html

@DanTup
Copy link
Member

DanTup commented Jul 24, 2019

when you work on a page you focus on the UI then you add the business logic later

Right, makes more sense now :-)

It's possible the Flutter Outline data gives us enough to be able to do this on the client reliably, though it'd need some experimenting to be sure we can do it in a fairly generic way. I'll leave this open in the backlog so people can add 👍's to it (or if someone wants to have a go as a PR) and maybe have a look when I have a little time.

@DanTup DanTup added in editor Relates to code editing or language features in flutter Relates to running Flutter apps is enhancement and removed awaiting info Requires more information from the customer to progress labels Jul 24, 2019
@DanTup DanTup added this to the Backlog milestone Jul 24, 2019
@amreniouinnovent
Copy link
Author

@DanTup Actually wrapping with a StreamBuilder is already exist I think any other builder will be the same functionality.

@DanTup
Copy link
Member

DanTup commented Jul 25, 2019

I'm not sure what that means - do you mean you don't need this, or do you just mean there's one that's similar?

These refactors are done in the server, and it just gives us raw edits back. Customising the text that comes back is tricky because we don't know which part of the text corresponds to things like the class name (StreamBuilder) versus is the child.

I can't say for sure that we have enough info in the client to handle this reliable, it'd need a little investigation.

@amreniouinnovent
Copy link
Author

@DanTup I mean a similar implementation..Take your time.

@venkatd
Copy link

venkatd commented Aug 20, 2019

I have a related proposal but not sure how complex (or possible) such an implementation would be. I constantly find myself wrapping/unwrapping widgets in my workflow. Would be interesting to allow arbitrary wrapping.

Idea:

  • Build an index of all widgets that extend Widget and have a Widget child or List<Widget> children property
  • Expose a "wrap widget" refactor which prompts the user with an auto-complete list to type the name of a widget. The available items in the list would only widgets that can wrap other widgets.
  • When the widget is selected, wrap the widget using the child or children property.

Thoughts?

@amreniouinnovent
Copy link
Author

@venkatd Making dynamic autocomplete is a good suggestion
Also having widgets to wrap a list of widgets in children:[inner widget] is a good suggestion too.

@DanTup
Copy link
Member

DanTup commented Aug 20, 2019

That's an interesting idea, though the VS Code extension doesn't have all the information required to build such an index, so it'd need support in the server.

That said, it doesn't feel like the experience today is that far off. If you use "Wrap with new Widget", it nested inside a widget and selects the word widget for you to type the name of the widget you want. That code-completion filters as you type, so if you start typing the widget you want it should be quickly selectable. Is this not working that way for you?

You can also keybind refactors like "Wrap in new widget" - see #1931 (comment) (though it's broken in the current stable release, and fixed for the next one :-))

@DanTup DanTup added the in lsp/analysis server Something to be fixed in the Dart analysis server label Jun 1, 2020
@jagmit
Copy link

jagmit commented Apr 19, 2021

@DanTup I think the situation was not explained clear enough yet here so I'll try again:

The generic option "Wrap with widget..." works really good for wrapping a widget as a child with any arbitrary widget.

Another class of use cases is where you want to wrap a widget with some sort of builder, where the widget is not just going to be a regular child but part of a builder function.
There are a lot of different custom types of builders (LayoutBuilder, FutureBuilder, BlocBuilder etc.). For these we only have the specific option to generate a StreamBuilder, but not a generic option.

The generic "Wrap with widget..." is great for a custom widget, because from there you only need to add stuff manually in order to turn the generated generic output into something "custom".
For a builder you only have the very specific "Wrap with StreamBuilder" option, so you need to remove stuff first to turn the generated StreamBuilder into a generic form and then you can add your custom stuff (add/change the generic type, add a child or any other field, change the signature of the builder function etc.).

This extra step for a builder does not sound like much, but it quickly start to get annoying if you have to do this a lot every day :)

One option to solve this would be to add a generic "Wrap with builder..." option to dart code and the analysis server, that maybe outputs something like

Builder<Object> {
    builder: (context) {
        return SomeWidgetThatWasAlreadyPreviouslyThere(
        ...
        )
    }
}

with Builder or Object being selected.

Another option that is mentioned here would be to invent some type of template format, which could be defined once somewhere and then would show up in the refactor context menu. In the template you could then define where the widget, on which this menu was opened, would be placed and which parts of the generated code snippet would be selected after running the option to quickly make changes.

I hope explanation makes it more clear.

@DanTup DanTup changed the title Configurable refactor menu Add a "Wrap with builder" assist that works like "Wrap with widget" Apr 20, 2021
@DanTup DanTup modified the milestones: Backlog, On Deck Apr 20, 2021
@DanTup
Copy link
Member

DanTup commented May 13, 2021

Looks like this was implemented by @b-stime in dart-lang/sdk#45656 and will be available in a future SDK release :-)

May-13-2021 15-11-55

@DanTup DanTup closed this as completed May 13, 2021
@II11II
Copy link

II11II commented Jun 25, 2021

Hi @DanTup 🖐
Can we access it beforehand?

@DanTup
Copy link
Member

DanTup commented Jun 25, 2021

@II11II the fix is in the SDK, so you'd likely need to be using the dev (or master) channel of the Flutter SDK to get it for now.

@shane99a
Copy link

shane99a commented Jul 2, 2021

Am I crazy or do I not see the Wrap with Builder option in the released version. Using version: v3.24.0. Edit: Nvm, I understand now you have to change channels to get it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in dart / flutter in editor Relates to code editing or language features in flutter Relates to running Flutter apps in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Projects
None yet
Development

No branches or pull requests

6 participants