Add support for attachRequest in DAP, running "flutter attach" #97652
Conversation
flutter-dashboard
bot
added
the
tool
| @@ -34,236 +36,324 @@ void main() { | |||
| tryToDelete(tempDir); | |||
| }); | |||
|
|
|||
| testWithoutContext('can run and terminate a Flutter app in debug mode', () async { | |||
There was a problem hiding this comment.
this file may be best reviewed ignoring whitespace or side-by-side, as I wrapped a group around the launch tests that makes it seem like almost the entire file was modified.
| /// etc.) will still be available. | ||
| /// | ||
| /// debug/noDebug here refers to the DAP "debug" mode and not the Flutter | ||
| // debug mode (vs Profile/Release). It is possible for the user to "Run" |
There was a problem hiding this comment.
looks like the formatting of the comments on lines 96,98 got messed up, no longer dartdocs plus an extra indent.
| @@ -83,11 +83,41 @@ class FlutterDebugAdapter extends DartDebugAdapter<FlutterLaunchRequestArguments | |||
| @override | |||
| bool get terminateOnVmServiceClose => false; | |||
|
|
|||
| /// Whether or not the user requested to debug (via noDebug). | |||
There was a problem hiding this comment.
Would it also be correct for this to be a getter, like:
bool get debug {
final DartCommonLaunchAttachRequestArguments args = this.args;
if (args is! FlutterAttachRequestArguments) {
return false;
}
return !(args.noDebug ?? false);
}
It took me quite a while to understand what this code was doing (and maybe I still misunderstood).
There was a problem hiding this comment.
Almost, yeah. It would actually be more like:
bool get debug {
final args = this.args;
final noDebug = args is FlutterLaunchRequestArguments
? args.noDebug
: args is FlutterAttachRequestArguments
? args.noDebug
: false; // Should not happen, args is always FlutterLaunchRequestArguments or FlutterAttachRequestArguments
return !(noDebug ?? false);
}It's a bit odd because noDebug is not on the base arguments class because for Dart it's not valid to do noDebug for Attach (because we need a VM Service connection to attach). Flutter actually allows us to attach without a VM Service though, so noDebug ends up on its attach too.
A nicer way might be to add a common case for Flutters args (like DartCommonLaunchAttachRequestArguments). However, looking again at the DAP spec it also does not list noDebug as a valid option for attach anyway, so now I'm unsure whether VS Code would actually honour the difference between Run/Debug when using a launch config with "request": "attach".
Let me do some more testing of this.. if VS Code doesn't support it, then we should remove it from here too which simplifies things a little more (to something like your example, but return true is args is! FlutterLaunchRequestArguments).
It took me quite a while to understand what this code was doing (and maybe I still misunderstood).
Anything in particular? I should make the code and/or comments clearer then :-)
There was a problem hiding this comment.
Anything in particular? I should make the code and/or comments clearer then :-)
Mostly the late initialization of the debug property and the fact that the arg is noDebug but the property is debug. I think if it's refactored to be a getter it will improve the maintainability.
There was a problem hiding this comment.
and the fact that the arg is
noDebugbut the property is debug
Yeah, this is pretty confusing. noDebug comes from DAP so we can't change it.. it's a negative and it's optional, defaulting to false (meaning debug=true). I think wrapping it up in a getter so this inversion is only in a single place makes a lot of sense. I'll do this tomorrow, as well as remove it from attach if it turns out that VS Code doesn't pass it (if it does pass it, I'll file a DAP issue asking for clarification on its support).
There was a problem hiding this comment.
I've removed noDebug from attach now, and added a getter for debug that hopefully makes things clearer. Let me know if not!
…er#97652) * Add support for attachRequest in DAP, which runs "flutter attach" * Update DAP docs for attachRequest * Improve doc comments * Fix comments * Remove noDebug from attach + create a getter for `debug` * Fix indent
This adds support for
attachRequestin the DAP server. This works much likelaunchRequestbut instead of runningflutter runit runsflutter attach(and as such has a restricted set of flags that can be passed).Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.