Add DTD version of sidebar #7919

DanTup · 2024-06-12T14:02:53Z

This is a work in progress to add a DTD implementation of the sidebar (while still maintaining a postMessage-compatible version for some transition period). There's still work to do here but I'm opening a Draft PR as a reference for what I'm working towards as I try to land smaller PRs:

This adds a DTD version of the sidebar alongside the current postMessage version. They implement a common interface so we can (mostly) have one implementation of the sidebar widgets while supporting both so that we can initially test the sidebar with a flag and then switch over once we're sure it's good.

Although it's mostly done, there are still a few things outstanding before this is mergable:

Merge latest DevTools master into this PR
Publish a new package:dtd with the latest changes and update the version here (we rely on some new constructors and type changes in package:dtd)
Ensure the DTD changes to support the Service stream have rolled into Flutter and that version of Flutter has rolled into DevTools (current is 3.5.0-293.0.dev but we need something with 23de3198faaa624c68cce347574a531854699af5 which looks like 3.6.0-16.0.dev)
Move the spec for the editor service to the SDK repo (in the doc @helin24 started)
Re-test both the simulated environment and in VS Code using dart.customDevTools (without dependency overrides etc., just using the pinned version of Flutter)

DanTup · 2024-07-11T16:11:07Z

@kenzieschmoll this is mostly ready now (I don't have any more changes planned, so it's at least ready for a round of feedback). I can load this version of the sidebar in VS Code and all of the functionality from the previous version works (and as a bonus of not using postMessage, we can open the sidebar outside of VS Code and it's still fully functional!).

It adds a new endpoint /editorSidebar for the DTD-based version (named more generically) which just wraps the same sidebar but providing a DTD connection instead of a postMessage API (they both implement the same interface). It also adds a new stager app for this DTD-based version (and some VS Code tasks/launch config to start a DTD connection for it).

Initially the Dart-Code changes will be landed behind a flag ("dart.previewDtdSidebar": true,) to allow some additional testing (and selection of the SDK version we want to gate it on).

There will be a fair bit of code we can clean up/merge once we get rid of the postMessage version, but we need to support both for now to allow us to switch over gradually (and be able to switch back in VS Code if we find issues).

There's a temporary markdown file here that will be moved to the SDK, but I'll wait for @helin24's change to land first because it'll go into the same file.

DanTup · 2024-07-11T16:12:35Z

(the bot failure is because we need a newer version of DTD that isn't published yet - there have still been some other changes ongoing in that, so it's easier to release once they're all done - that will need doing before we can merge this ofc)

packages/devtools_app/lib/src/service/editor/api_classes.dart

kenzieschmoll · 2024-07-12T16:29:05Z

packages/devtools_app/lib/src/service/editor/editor_client.dart

+  //  is removed.
+
+  DtdEditorClient(this._dtd) {
+    unawaited(initialized); // Trigger async initialization.


can we just call unawaited(_initialize()) and get rid of the initialized variable?

We can't remove the variable because it's part of the external interface (the sidebar will fetch the initial set of devices/debug sessions once this completes).

kenzieschmoll · 2024-07-12T16:32:29Z

packages/devtools_app/lib/src/service/editor/editor_client.dart

+///
+/// Changes made to the editor services/events should be considered carefully to
+/// ensure they are not breaking changes to already-shipped editors.
+class DtdEditorClient extends EditorClient {


mixin DisposerMixin so that you can add the listeners on _dtd with the addAutoDisposeListener or addAutoDisposeStreamSubscription. And then you'll probably need to call dispose() from the close method. Looking at the code, you'll probably want to do the same for the post message client

DisposerMixin doesn't seem to have a dispose() method. I see one in AutoDisposeControllerMixin that calls cancelStreamSubscriptions, cancelListeners, cancelFocusNodes but it's not clear if I should use that here (it adds some additional stuff that doesn't seem relevant.

(I could just duplicate that dispose() method into my class, but it doesn't feel like the best thing)

if EditorClient extends DisposableController, then you can mixin AutoDisposeControllerMixin here. After looking at this a little bit, I think the autodispose code could use a little cleanup.

Ah, done in 1bd6ef8

kenzieschmoll · 2024-07-12T16:33:23Z

packages/devtools_app/lib/src/service/editor/editor_client.dart

+      if (method == EditorMethod.getDevices.name) {
+        _supportsGetDevices = isRegistered;
+      } else if (method == EditorMethod.getDebugSessions.name) {
+        _supportsGetDebugSessions = isRegistered;
+      } else if (method == EditorMethod.selectDevice.name) {
+        _supportsSelectDevice = isRegistered;
+      } else if (method == EditorMethod.hotReload.name) {
+        _supportsHotReload = isRegistered;
+      } else if (method == EditorMethod.hotRestart.name) {
+        _supportsHotRestart = isRegistered;
+      } else if (method == EditorMethod.openDevToolsPage.name) {
+        _supportsOpenDevToolsPage = isRegistered;
+        _supportsOpenDevToolsForceExternal =
+            capabilities?['supportsForceExternal'] == true;
+      } else {
+        return;
+      }


use a switch statement here instead

Unfortunately EditorMethod.hotRestart.name is not constant (name is an extension from the SDK). I did try doing this a few other ways but they all felt a little messy too (for example adding an explicit name to the enum values means every enum value has to have its name duplicated as a string in the constructor call). I can change if you have any preference between them though.

packages/devtools_app/lib/src/service/editor/editor_client.dart

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

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

kenzieschmoll · 2024-07-12T20:17:24Z

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

+    createLoggedWebSocketChannel(Uri wsUri) async {
+  final logController = StreamController<String>();
+
+  final rawChannel = WebSocketChannel.connect(wsUri);


does this channel need to be closed at some point by the caller of createLoggedWebSocketChannel? Otherwise we might be leaking resources

I believe this is the same as above. The stream gets passed to the DartToolingDaemon class and it will handle closing them (which is handled by the fake editor if you click the disconnect button, but in this test app we don't really have another way to close it - eg. you can't navigate away from the mock editor scene when you run this app).

kenzieschmoll · 2024-07-12T20:19:59Z

Whoops didn't mean to approve, just meant to comment. Undo LGTM.

…tters+fields

…lient.editorServiceChanged See flutter#7919 (comment)

See flutter#7919 (comment)

…provements, remove experiment flag, unused PerformanceController

…ding between buttons

…he simulated editor See flutter#7919 (comment)

…ditor"s

DanTup · 2024-07-16T14:48:13Z

@kenzieschmoll thanks! I've been through and addressed (or responded to) all of your points now (sorry for the spam).

I've also edited in a checklist to the top comment here with some final things that will need doing before this can merge (the bots will continue to be red until some more of those are done).

packages/devtools_app/lib/src/service/editor/editor_client.dart

packages/devtools_app/lib/src/shared/analytics/constants/_editor_sidebar_constants.dart

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

kenzieschmoll · 2024-07-16T22:00:31Z

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

 import 'mock_editor_widget.dart';

 /// To run, use the "standalone_ui/vs_code" launch configuration with the
 /// `devtools/packages/` folder open in VS Code, or run:
 ///
-///   flutter run -t test/test_infra/scenes/standalone_ui/vs_code.stager_app.g.dart --dart-define=enable_experiments=true -d chrome
+///   flutter run -t test/test_infra/scenes/standalone_ui/vs_code.stager_app.g.dart -d chrome


remove leading space

Done in 98d75b8

kenzieschmoll · 2024-07-16T22:02:21Z

packages/devtools_app_shared/lib/src/utils/list_queue.dart

+/// This class is not a full implementation of the [ListQueue] class, but it
+/// exposes the majority of [ListQueue] methods that are useful for how this
+/// class is used in DevTools. New methods can be added to this class as needed.
+final class ListQueueValueNotifier<T> extends ChangeNotifier


this should have tests. To not block this PR, I recommend putting this class under devtools_app/ and then adding a matching test file devtools_app/test/shared/ with a TODO and a tracking issue. Once this class is well tested we can consider adding to the public package devtools_app_shared/ for shared use.

Done in 98d75b8

See flutter#7919 (comment)

DanTup · 2024-07-17T09:55:45Z

@kenzieschmoll thanks! I've addressed/responded to your latest comments. I also cleared your review status because I think it was your accidental approve rather than a new one.

I'll try to progress the other things in the checklist above that this PR depends on.

kenzieschmoll

looks good!

This is going in the SDK at https://dart-review.googlesource.com/c/sdk/+/375080

DanTup · 2024-07-31T13:04:13Z

I've merged latest master into this and the bots have gone green. I've removed the temp spec file here and opened https://dart-review.googlesource.com/c/sdk/+/378141 to put it in the SDK with the other common service interfaces. I also made a minor tweak to support null deviceIds for selectDevice (VS Code already supported it and we had it in the event, but not the request).

I've also tested this latest version again with the Dart-Code branch that uses the DTD sidebar and everything is working:

@kenzieschmoll any final comments before I merge this? I will merge the Dart-Code work after tomorrows release so it will become available behind a flag in the pre-release extension tomorrow so it'll be easy for others to test (and we can figure out when to remove the flag and just gate on SDK version when we're sure it's all good).

kenzieschmoll · 2024-07-31T19:29:53Z

Let's hold off until the 2.38.0 release of DevTools is complete (working on that today). Will ping this PR or just merge it once the release is out. Thanks!

kenzieschmoll · 2024-08-01T16:26:56Z

The 2.38.0 DevTools release into the SDK is complete. Good to merge this now.

DanTup mentioned this pull request Jun 12, 2024

Refactor VS Code sidebar to make more generic and align with new DTD APIs #7918

Merged

DanTup force-pushed the add-dtd-sidebar branch 3 times, most recently from 6b81422 to 0ef8f1c Compare June 17, 2024 11:38

DanTup force-pushed the add-dtd-sidebar branch 6 times, most recently from f9f8028 to ef8b233 Compare July 11, 2024 15:27

Add DTD version of sidebar

f9a7b13

DanTup force-pushed the add-dtd-sidebar branch from ef8b233 to f9a7b13 Compare July 11, 2024 16:03

DanTup added the release-notes-not-required label Jul 11, 2024

DanTup marked this pull request as ready for review July 11, 2024 16:03

DanTup requested a review from a team as a code owner July 11, 2024 16:03

DanTup requested review from elliette and kenzieschmoll and removed request for a team July 11, 2024 16:03

kenzieschmoll approved these changes Jul 12, 2024

View reviewed changes

kenzieschmoll self-requested a review July 12, 2024 20:18

DanTup added 7 commits July 16, 2024 15:21

Review tweaks: backticks in docs, switch expression event, reorder ge…

77f796e

…tters+fields

Include more information about registration/unregistration in EditorC…

83fd3e7

…lient.editorServiceChanged See flutter#7919 (comment)

Replace some raw strings with constants

958878a

See flutter#7919 (comment)

Split analytics IDs for legacy + DTD sidebars

1011260

See flutter#7919 (comment)

Replace strings with constants, trailing commas, timeout TODO, doc im…

b65f2bb

…provements, remove experiment flag, unused PerformanceController

Fix some casts + typo in GetDebugSessions call

60d3577

Switch SingleChildScrollViews to ListView.builders() and add some pad…

e07fca2

…ding between buttons

DanTup added 2 commits July 16, 2024 15:21

Add ListQueueValueNotifier and switch to it for the logging view of t…

d50c807

…he simulated editor See flutter#7919 (comment)

Rename many "FakeEditor" and "EditorServer" classes to be "SimulatedE…

60707d7

…ditor"s

kenzieschmoll reviewed Jul 16, 2024

View reviewed changes

DanTup added 3 commits July 17, 2024 10:47

Use AutoDisposeControllerMixin for EditorClient implementations

1bd6ef8

See flutter#7919 (comment)

Minor review tweaks

98d75b8

Move ListQueueValueNotifier out of shared + add TODO for tests

02d343a

DanTup requested a review from kenzieschmoll July 17, 2024 09:54

Update devtools_app to DTD 2.3

41b766f

kenzieschmoll approved these changes Jul 19, 2024

View reviewed changes

DanTup added 3 commits July 31, 2024 13:14

Merge branch 'master' into add-dtd-sidebar

3d4d210

Handle nullable deviceId for selectDevice to match the event

a47ec6f

Remove editor service spec

16c08a3

This is going in the SDK at https://dart-review.googlesource.com/c/sdk/+/375080

DanTup merged commit faeb2a3 into flutter:master Aug 1, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DTD version of sidebar #7919

Add DTD version of sidebar #7919

DanTup commented Jun 12, 2024 •

edited

Loading

DanTup commented Jul 11, 2024

DanTup commented Jul 11, 2024

kenzieschmoll Jul 12, 2024

DanTup Jul 16, 2024

kenzieschmoll Jul 12, 2024

DanTup Jul 16, 2024

kenzieschmoll Jul 16, 2024 •

edited

Loading

DanTup Jul 17, 2024

kenzieschmoll Jul 12, 2024

DanTup Jul 16, 2024

kenzieschmoll Jul 12, 2024

DanTup Jul 16, 2024

kenzieschmoll commented Jul 12, 2024

DanTup commented Jul 16, 2024

kenzieschmoll Jul 16, 2024

DanTup Jul 17, 2024

kenzieschmoll Jul 16, 2024

DanTup Jul 17, 2024

DanTup commented Jul 17, 2024

kenzieschmoll left a comment

DanTup commented Jul 31, 2024

kenzieschmoll commented Jul 31, 2024

kenzieschmoll commented Aug 1, 2024

Add DTD version of sidebar #7919

Add DTD version of sidebar #7919

Conversation

DanTup commented Jun 12, 2024 • edited Loading

DanTup commented Jul 11, 2024

DanTup commented Jul 11, 2024

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 Jul 16, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll commented Jul 12, 2024

DanTup commented Jul 16, 2024

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 commented Jul 17, 2024

kenzieschmoll left a comment

Choose a reason for hiding this comment

DanTup commented Jul 31, 2024

kenzieschmoll commented Jul 31, 2024

kenzieschmoll commented Aug 1, 2024

DanTup commented Jun 12, 2024 •

edited

Loading

kenzieschmoll Jul 16, 2024 •

edited

Loading