Debugger panel text selection improvements #6020
Conversation
| // browser's native context menu on secondary-click, and instead use the | ||
| // menu provided by Flutter: | ||
| if (kIsWeb) { | ||
| unawaited(BrowserContextMenu.enableContextMenu()); |
There was a problem hiding this comment.
do we really want to do this at the app-wide level? Will this break any flutter context menu actions we use elsewhere? Perhaps we could enable this menu more local to the console / debugger.
There was a problem hiding this comment.
I'm pretty sure this is a no-op (the context menu is already globally enabled) - what we really want to do is disable it, but currently are blocked by flutter/flutter#129692. I can switch this to disabling and just comment it out.
There was a problem hiding this comment.
What about disabling the context menu closer to the source of where we have actually implemented our own context menus? Like in this example how the browser context menu is disabled and reenabled within the lifecycle of this widget: https://github.com/flutter/flutter/blob/e12167d98c44ad1977934399757a4351821f4f95/examples/api/lib/material/context_menu/context_menu_controller.0.dart#L34-L50
There was a problem hiding this comment.
I considered doing that, though I'm not sure where the best place would be. Would we want it on the entire debugger panel (and if so, would it be weird that that one panel has different context menus then the rest of DevTools)? Or on each selectable text region (which could get us into a weird state, eg context menu is disabled when codeview inits its state, then when the variable tree inits its state, then the variable tree is removed and the context menu is enabled, even though we still want it for codeview)?
I think we probably want the Flutter context menus everywhere. This matches the behavior for macos, and so ideally means the experience during development matches the experience in release-mode.
| @@ -75,10 +53,10 @@ class DisplayProvider extends StatelessWidget { | |||
| } | |||
|
|
|||
| final hasName = variable.name?.isNotEmpty ?? false; | |||
| return DevToolsTooltip( | |||
There was a problem hiding this comment.
is there a reason we removed the tooltip here?
There was a problem hiding this comment.
Because this was initially branched off of #6001 (which hasn't been submitted yet)
| final menuButtons = <ContextMenuButtonItem>[]; | ||
| if (variable.isRerootable) { | ||
| menuButtons.add( | ||
| ContextMenuButtonItem( | ||
| onPressed: () { | ||
| ContextMenuController.removeAny(); | ||
| final ref = variable.ref; | ||
| serviceManager.consoleService.appendBrowsableInstance( | ||
| instanceRef: variable.value as InstanceRef?, | ||
| isolateRef: ref?.isolateRef, | ||
| heapSelection: ref?.heapSelection, | ||
| ); | ||
| }, | ||
| label: 'Reroot', | ||
| ), | ||
| ); | ||
| } | ||
| if (serviceManager.inspectorService != null && variable.isRoot) { | ||
| menuButtons.add( | ||
| ContextMenuButtonItem( | ||
| onPressed: () { | ||
| ContextMenuController.removeAny(); | ||
| _handleInspect(context); | ||
| }, | ||
| label: 'Inspect', | ||
| ), | ||
| ); | ||
| } | ||
| return menuButtons; |
There was a problem hiding this comment.
this can be condensed:
return [
if (variable.isRerootable)
// reroot button
if (serviceManager.inspectorService != null && variable.isRoot)
// inspect button
];
Fixes #6019
Work towards #5963
Note: Includes TODOS for #6018, which requires flutter/flutter#129692 to be fixed first
Demos (after):
Demos (before):