Take 2 on "Some initial work for FlutterWeb apps" change #3342
Conversation
| @NotNull | ||
| private final List<String> args; | ||
|
|
||
| // TODO(github.com/flutter/flutter-intellij/issues/3293) This is a temporary "<pub-cache>/bin/webdev" that can be provided to test some |
There was a problem hiding this comment.
please update these to bugs related to the problem being solved with the workaround not the feature being implemented.
its fine if the bug descriptions are short but that will make more sense to users viewing the TODO. For example, the bug you linked to will be closed but the TODOs will still exist. In general the bugs for the TODO should not be closed until the TODO is removed.
There was a problem hiding this comment.
Will do, I suppose it will make it more explicit, right now the issue, #3293, has a list of tasks associated with it... I will create some additional more explicit issues to track the specific tasks (4th bullet point, in this case.)
|
|
||
| line.setExePath(FileUtil.toSystemDependentName(sdk.getHomePath() + "/bin/" + FlutterSdkUtil.flutterScriptName())); | ||
| /** | ||
| * Creates a FlutterWeb command line to run. |
There was a problem hiding this comment.
remove this comment as it doesn't add any value beyond the method name.
src/io/flutter/sdk/FlutterSdk.java
Outdated
| args.add("run"); | ||
| args.add("webdev"); | ||
| args.add("daemon"); | ||
| // TODO After debug is supported by webdev, this should be modified to check for debug and add any additional needed flags: |
There was a problem hiding this comment.
add a bug related to this TODO or add your username to the todo.
| + e.toString() + "\n" + | ||
| formatStackTraces(e)); | ||
| return; | ||
| // TODO(github.com/flutter/flutter-intellij/issues/3293, github.com/dart-lang/webdev/issues/233) The following check disables the |
There was a problem hiding this comment.
same as other cases. add a bug for the TODO
There was a problem hiding this comment.
actually in this case only list the one bug related to the issue and not the bug describing the feature you are implementing.
src/io/flutter/run/LaunchState.java
Outdated
| final Project project = getEnvironment().getProject(); | ||
| final FlutterDevice device = DeviceService.getInstance(project).getSelectedDevice(); | ||
|
|
||
| // If the device is null and the project is not a flutter web project, show a message that a device is required and return null. |
There was a problem hiding this comment.
Nit: make the comment explain that flutter web doesn't yet support devices so this tweak is needed. As is the comment explains what happens which is visible from the code but not why that is a good thing.
* Take 2 on "Some initial work for FlutterWeb apps" change that was reverted. * last commetns from @jacob314
3 participants
The fix to the
flutter runregression is in DartVmServiceDebugProcess.javaOriginal, reverted, PR: #3315
Tracking issue: #3293