devtools

Add some simple side bar functionality with a mock editor environment

#6104

Merged

nit: replace "extensions" with "surfaces". This could be confusing with DevTools Extensions.

Done! Although I'm not sure these terms are completely clear to me (surfaces vs extensions).

I was thinking "Dart Tooling Extensions" would mean any extension that could consume the new APIs (via DTD), and that "DevTools Extensions" would be a subset of "Dart Tooling Extensions".

what exactly is "Dart tooling" here? DevTools, Dart-Code VS code extension?

I changed this to:

/// An API used by Dart tooling surfaces to interact with Dart tools that expose
/// APIs such as Dart-Code and the LSP server.

Is that clearer? I couldn't come up with a (non-vague) term to describe the components that will generally provide APIs.

can you add a comment describing when we would want to use the websocket connection vs the post message connection.

I've removed this as it was unused. It was originally used to let you connect to VS Code without being embedded inside it (as a way to simplify debugging when interacting with VS Code), however I think that's somewhat superseded by the mock environment - and if we do want to support it in future we'll just use the DTD.

does this stream controller need to be created in the context of the class so that it can be disposed once it is done being used?

I've added a line here to automatically close it when rpc closes which I think does what you wanted (but also handles the connection going away without dispose being called explicitly).

what is the benefit of having an interface for all these classes? Can we just have one class instead of having an interface and an impl?

The goal was to make the API much clearer so it can more easily be mirrored/compared to VS Code. Without interfaces it might also be quite easy to make an accidental breaking change to the interface (and the mock environment) without VS Code.

I added some more comments to the interface classes to make this clearer, although I'm not married to the idea - if you'd prefer not to have them (and aren't concerned about accidental breakage), I'm happy to remove :-)

put these strings into consts so that we can reuse them here and in toJson below

Done! I put them in the API interface class for now (since they're really part of the API) but if we combine them they'll come back here. FWIW I'd really prefer to code-gen this because there's a lot of boilerplate (that would be copy/pasted and easy to have mistakes), but probably something for later.

same here, please make these static consts for re-use below

Done!

put this and 'selectDevice' into a const class member

Done!

should there also be a capability for 'selectDevice'

Good spot, fixed!

I don't think this needs to be a stateful widget

You're right, fixed!

can we remove this empty text widget?

I replaced this with const SizedBox(height: 20), (it was to add some padding to prevent the first item in the column touching the top of the window), but I'm not sure if that's the best way to do this?

return SizedBox.shrink() for a placeholder instead

this text can be part of the TextButton child Text widget above.

Text(
  '${device.name}'
  '${device.id == deviceEvent.selectedDeviceId ? ' (selected)' : ''}',
)

That would include it in the button so the styling would change - this was deliberately separate:

Although, this "design" is just a placeholder and we definitely don't want to ship it looking like this (we probably want to show more than just the name of a device for ex.).

Fine to land as is and iterate with some ux input later

can we use a ListView or a column instead?

darwin or dartwin?

nit: put type on right hand side:
final _devices = <VsCodeDevice>[]

I wonder if this should be implemented as a stager app that lives in our test/ directory, that way it doesn't have to get shipped with our production code. We have stager apps for several DevTools screens that can be used for simplified development and testing.

Oh that might be better. I'm not very familiar with stager though - I'll take a look at changing it this week. It would definitely be nicer if we could keep development screen out of the main app. Thanks!

I've done this, so the mock API + mock environment now live in test/. I need to figure out why the theming doesn't work though (it seems to always run in light mode even when the toggle is set to dark).

do we need to add a dep on meta? I remember an effort to remove this dependency from devtools a while back, though I can't recall why. Probably because we were using it for @required and some other signatures that are now provided by flutter out of the box.

I added it to use @protected to try and avoid some of the base methods in the API classes showing up to the widget code. Unfortunately code completion still shows them (dart-lang/sdk#35449), though it does generate a warning if you use them.

It was mostly just to enforce everything going through methods (eg. don't call sendRequest('someString') directly from a widget), but it's a fairly small benefit. If there's a reason to avoid the dependency (and it wasn't just removed because it was unnecessary), we could remove it. I tracked the change back to #3484 but I don't understand the reason - wdyt?

nit use defaultSpacing so that we are consistent with other devtools spacing

nit: do we need to manually specify a splitter here? is the DefaultSplitter that is used when splitters is null not sufficient?

Not really. I was trying to make it look a little more like VS Code (so it gave a better idea of how it'd look) so this was to set the colour and make it narrower. It didn't work particularly well though, so I've removed it for now.