dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.58k forks source link

Missing `Remove this widget` case and wrongly removing const #57072

Open FMorschel opened 1 week ago

FMorschel commented 1 week ago

Repro:

import 'package:flutter/material.dart';

Widget f() {
  return const Center(
    child: Scaffold(
      body: Column(
        children: [
          Text('Hello World!'),
        ],
      ),
    ),
  );
}

Wrongs:

dart-github-bot commented 1 week ago

Summary: The "Remove this widget" feature in Flutter's widget inspector is inconsistent. It disappears when removing a child widget from a Column, and incorrectly removes the const keyword when applied to a Center widget.

bwilkerson commented 1 week ago

If you remove Text the option to Remove this widget is gone from Column

How often does this situation arise? I wouldn't think it happens very often, so it doesn't seem like a high priority issue, but if you know differently ...

What should the result be of removing Column in that case? I ask because as a general principle we don't support refactorings / assists when they would produce diagnostics that didn't exist before. Removing Column seems like it would have that characteristic, which probably explains why we don't support it.

At Center you have the option to Remove this widget but it does remove const

That seems like a clear bug.

FMorschel commented 1 week ago

With this you also can't remove the Text:

Center(
  child: Text(''),
)

What should the result be of removing Column in that case?

Or the Text in this case.

I would suggest the assist to look if the parameter is positional (not optional) or required to decide this and then if it can be safely removed, simply remove the full parameter.

How often does this situation arise?

I'm not sure. I do get to this some of the time when I have custom widgets in different places on my screen and I'd like to remove it from one of those places.

I wouldn't think it happens very often, so it doesn't seem like a high priority issue

I do agree this is definitely not a high-priority issue. I just think it would be nice to have the assist do this for us. Some of the time there can be lots of inner parameters and this would lessen the job of selecting the full range of lines.

bwilkerson commented 1 week ago

I just think it would be nice to have the assist do this for us.

I agree, but we need to focus on the higher priority issues first.