Add some simple side bar functionality with a mock editor environment #6104

DanTup · 2023-07-26T17:43:44Z

This is (another) evolution of #5747 / #5921.

I wanted to make it easy to work on this panel without needing to embed inside VS Code (not just because messing around with connecting WebSockets is clunky, but because it also means having Dart-Code set up for development). I tried hosting the panel inside an iframe (where the outer page could simulate the editor), but hosting Flutter apps inside an iframe seems to break the debugging flow (the inner app fails to start up with some odd javascript errors, but I think the debugger would fail to work with it even with that fixes).

So this version hosts everything in one page by just wrapping the VsCodeFlutterPanel with another widget (currently called VsCodeFlutterPanelMock but it's not a great name) which plays the part of the IDE. It sets up streams (to emulate postMessage, WebSockets, whatever) and logs the data going over it. It provides a mock implementation of the API that VS Code will provide and some buttons to trigger events that would originate in VS Code.

This should allow working on the panel and defining a concrete API (but adding it to the DartApi classes and a mock implementation), as well as making it possible to write tests, before implementing the same API in VS Code.

Recording.2023-07-26.183432.mp4

@kenzieschmoll interested in your thoughts. Right now the bits that are there are very basic (for example the devices passed over only have IDs), but I think it'll be quicker to iterate like this without having to simultaneously provide the VS Code implementation.

DanTup · 2023-08-01T14:08:37Z

@kenzieschmoll I'm reworking this a little bit (to make the API a bit cleaner, support capabilities, and to make the API interface clearer so it can be kept in-sync with VS Code). Feel free to skip looking at this until I'm done (although the general idea is the same, and I'd still like to have a "mock" instance of an editor for easier development (since we'd likely end up with something similar for automated test anyway).

DanTup · 2023-08-02T14:58:59Z

@kenzieschmoll I've pushed some changes here. It's not currently visually appealing, but the basic framework is there. I have a branch of Dart-Code that costs the sidebar and provides the same functionality that the mock environment does here.

This currently uses postMessage inside VS Code and just simple streams in the mock environment - both are just JSON-RPC so I believe should be an easy migration to support DTD in the future.

APIs are implemented in a way that is conditional (so the panel can check which APIs are available) and supports capabilities (so if a whole API is available, we can still check its capabilities to help with the API changing over time). I separated the APIs into an interface and an implementation so that breaking changes should be more obvious and it's clear exactly what needs to be mirrored between here and the other side.

It'll need tests + making look nicer, but I'd like to get some feedback on the general approach first. You can run this using the new launch configuration here (which will load the mock environment, showing the embedded panel and some buttons/log stream):

kenzieschmoll

can you mark which parts of this prototype would become obsolete once we have a Dart Tooling Daemon we can use to pass messages back and forth?

DanTup · 2023-08-03T09:40:18Z

can you mark which parts of this prototype would become obsolete once we have a Dart Tooling Daemon we can use to pass messages back and forth?

I don't actually think much of this will go away, but rather just be changed slightly. For example the DartToolingApiImpl.postMessage() constructor would likely be replaced with one that connects to the DTD instead, and the helpers for sending requests/listening to streams (DartToolingApiImpl.sendRequest and DartToolingApiImpl.events) might change slightly but I think everything else will probably remain (we still want the API classes and JSON handling for type safety - although these are something we might be able to code-gen - but for now they're hand-written).

kenzieschmoll · 2023-08-03T19:46:56Z

packages/devtools_app/lib/src/standalone_ui/api/dart_tooling_api.dart

+
+import 'vs_code_api.dart';
+
+/// An API exposed to Dart tooling extensions.


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".

kenzieschmoll · 2023-08-03T19:47:41Z

packages/devtools_app/lib/src/standalone_ui/api/impl/dart_tooling_api.dart

+
+final _log = Logger('tooling_api');
+
+/// An API for interacting with Dart tooling.


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.

kenzieschmoll · 2023-08-03T19:49:50Z

packages/devtools_app/lib/src/standalone_ui/api/impl/dart_tooling_api.dart

+    return DartToolingApiImpl.rpc(json_rpc_2.Peer.withoutJson(channel));
+  }
+
+  /// Connects the API over the provided WebSocket.


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.

kenzieschmoll · 2023-08-03T19:52:00Z

packages/devtools_app/lib/src/standalone_ui/api/impl/dart_tooling_api.dart

+  /// Listens for an event '[apiName].[name]' that has a Map for parameters.
+  @protected
+  Stream<Map<String, Object?>> events(String name) {
+    final streamController = StreamController<Map<String, Object?>>.broadcast();


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).

kenzieschmoll · 2023-08-03T19:55:53Z

packages/devtools_app/lib/src/standalone_ui/api/impl/vs_code_api.dart

+  }
+}
+
+class VsCodeDeviceImpl implements VsCodeDevice {


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 :-)

kenzieschmoll · 2023-08-03T20:04:31Z

packages/devtools_app/lib/src/standalone_ui/vs_code/flutter_panel.dart

+              return const Text('');
+            }
+            final deviceEvent = snapshot.data!;
+            return Table(


can we use a ListView or a column instead?

kenzieschmoll · 2023-08-03T20:13:05Z

packages/devtools_app/lib/src/standalone_ui/vs_code/mock_environment/dart_tooling_mock_api.dart

+      emulator: false,
+      emulatorId: null,
+      ephemeral: false,
+      platform: 'dartwin-x64',


darwin or dartwin?

kenzieschmoll · 2023-08-03T20:13:30Z

packages/devtools_app/lib/src/standalone_ui/vs_code/mock_environment/dart_tooling_mock_api.dart

+  ];
+
+  /// The current set of devices being presented to the embedded panel.
+  final List<VsCodeDevice> _devices = [];


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

kenzieschmoll · 2023-08-03T20:16:24Z

...s/devtools_app/lib/src/standalone_ui/vs_code/mock_environment/flutter_panel_mock_editor.dart

+///
+/// This UI interacts with [MockDartToolingApi] to allow triggering events that
+/// would normally be fired by the IDE and also shows a log of recent requests.
+class VsCodeFlutterPanelMockEditor extends StatefulWidget {


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).

kenzieschmoll · 2023-08-03T20:17:56Z

packages/devtools_app/lib/src/standalone_ui/api/impl/vs_code_api.dart

+// found in the LICENSE file.
+
+import 'package:json_rpc_2/json_rpc_2.dart' as json_rpc_2;
+import 'package:meta/meta.dart';


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?

kenzieschmoll · 2023-08-10T22:16:16Z

packages/devtools_app/lib/src/standalone_ui/vs_code/flutter_panel.dart

@@ -73,41 +65,43 @@ class _VsCodeConnectedPanelState extends State<_VsCodeConnectedPanel> {
  Widget build(BuildContext context) {
    return Column(
      children: [
+        const SizedBox(height: 20),


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

kenzieschmoll · 2023-08-10T22:19:02Z

packages/devtools_app/test/test_infra/scenes/standalone_ui/vs_code_mock_editor.dart

+            PreferredSize(
+              preferredSize: const Size.fromHeight(1),
+              child: SizedBox(
+                height: 1,
+                child:
+                    ColoredBox(color: editorTheme.editorTerminalSplitterColor),
+              ),
+            ),
+          ],


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.

kenzieschmoll

This is looking good. I'm happy we were able to move the mock environment into a stager app so that it is not part of our production code. We'll likely need some clean up and iteration on the UI, but okay to land and iterate while we build these embedded features out.

- Add comments - Use more constants - StatelessWidget where state not required - Fix typos - Add `selectDevice` capability - ...

DanTup · 2023-08-21T09:18:40Z

The changes above this comment were just a rebase with no changes (because it's been a while since I opened this). Seems like a few things have changed in master and there are some errors, so the changes after this comment will be real changes to fix them.

DanTup · 2023-08-21T10:07:31Z

Turned out to just be needing imports for package:devtools_app_shared/ui.dart and no other changes, so merging.

This was referenced Jul 26, 2023

Add experimental VS Code extension prototype #5747

Closed

Connect to VS Code using a JSON-RPC API over postMessage or WebSocket #5921

Closed

DanTup force-pushed the new-sidebar-with-mock branch from e667664 to d9ee815 Compare August 2, 2023 14:47

DanTup changed the title ~~WIP: Add some simple side bar functionality with a mock editor environment~~ Add some simple side bar functionality with a mock editor environment Aug 2, 2023

kenzieschmoll reviewed Aug 2, 2023

View reviewed changes

kenzieschmoll reviewed Aug 3, 2023

View reviewed changes

DanTup force-pushed the new-sidebar-with-mock branch from 4b573ce to 8dd4eaa Compare August 7, 2023 15:04

kenzieschmoll reviewed Aug 10, 2023

View reviewed changes

DanTup added run-dcm-workflow release-notes-not-required labels Aug 13, 2023

DanTup marked this pull request as ready for review August 13, 2023 11:48

DanTup requested a review from a team as a code owner August 13, 2023 11:49

DanTup requested review from CoderDake and removed request for a team August 13, 2023 11:49

kenzieschmoll approved these changes Aug 17, 2023

View reviewed changes

DanTup added 7 commits August 21, 2023 09:56

Add some simple side bar functionality with a mock editor environment

34b5a8d

Add additional comments

32bb16d

Minor review comments

c1db587

- Add comments - Use more constants - StatelessWidget where state not required - Fix typos - Add `selectDevice` capability - ...

Move mock environment to test/stager scene

9d90ccc

Fix "Dark Theme" toggle in VS Code panel in stager scene

e560948

Use defaultSpacing + remove custom split widgets

b32f765

Remove unused key parameter

7aaf25f

DanTup force-pushed the new-sidebar-with-mock branch from ef9cb40 to 7aaf25f Compare August 21, 2023 08:56

Update imports

074eb47

DanTup merged commit 6440a75 into flutter:master Aug 21, 2023
15 checks passed

polina-c mentioned this pull request Aug 29, 2023

Releasing from 2.27.0-dev.17 to 2.27.0 polina-c/devtools#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some simple side bar functionality with a mock editor environment #6104

Add some simple side bar functionality with a mock editor environment #6104

DanTup commented Jul 26, 2023

DanTup commented Aug 1, 2023

DanTup commented Aug 2, 2023

kenzieschmoll left a comment

DanTup commented Aug 3, 2023

kenzieschmoll Aug 3, 2023

DanTup Aug 6, 2023

kenzieschmoll Aug 3, 2023

DanTup Aug 6, 2023

kenzieschmoll Aug 3, 2023

DanTup Aug 6, 2023

kenzieschmoll Aug 3, 2023

DanTup Aug 6, 2023

kenzieschmoll Aug 3, 2023

DanTup Aug 6, 2023

kenzieschmoll Aug 3, 2023

kenzieschmoll Aug 3, 2023

kenzieschmoll Aug 3, 2023

kenzieschmoll Aug 3, 2023

DanTup Aug 6, 2023

DanTup Aug 7, 2023

kenzieschmoll Aug 3, 2023

DanTup Aug 6, 2023 •

edited

Loading

kenzieschmoll Aug 10, 2023

kenzieschmoll Aug 10, 2023

DanTup Aug 13, 2023

kenzieschmoll left a comment •

edited

Loading

DanTup commented Aug 21, 2023

DanTup commented Aug 21, 2023


		import 'vs_code_api.dart';

		/// An API exposed to Dart tooling extensions.


		final _log = Logger('tooling_api');

		/// An API for interacting with Dart tooling.

Add some simple side bar functionality with a mock editor environment #6104

Add some simple side bar functionality with a mock editor environment #6104

Conversation

DanTup commented Jul 26, 2023

DanTup commented Aug 1, 2023

DanTup commented Aug 2, 2023

kenzieschmoll left a comment

Choose a reason for hiding this comment

DanTup commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanTup Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll left a comment • edited Loading

Choose a reason for hiding this comment

DanTup commented Aug 21, 2023

DanTup commented Aug 21, 2023

DanTup Aug 6, 2023 •

edited

Loading

kenzieschmoll left a comment •

edited

Loading