diff --git a/lib/features/workspace/result_grid_view.dart b/lib/features/workspace/result_grid_view.dart index 22d62cc..16f9f0f 100644 --- a/lib/features/workspace/result_grid_view.dart +++ b/lib/features/workspace/result_grid_view.dart @@ -123,6 +123,11 @@ List computeResultGridColumnWidths({ return widths; } +/// Counts calls to `_GridCell._buildContextMenuItems` — should only fire when +/// a cell's context menu is actually opened (#983), not on every cell build. +@material.visibleForTesting +int gridCellContextMenuItemsBuiltCount = 0; + /// Distributes spare viewport width adaptively to columns that benefit from expansion, /// avoiding artificial stretching of compact columns. List distributeResultGridSpareWidth({ @@ -2794,6 +2799,7 @@ class _GridCell extends material.StatelessWidget { onRevertRow == other.onRevertRow; List _buildContextMenuItems(material.BuildContext context) { + gridCellContextMenuItemsBuiltCount++; final preview = text.length > 24 ? '${text.substring(0, 22)}…' : text; final isNull = text == 'NULL'; final numVal = num.tryParse(text); @@ -3090,7 +3096,10 @@ class _GridCell extends material.StatelessWidget { } }, child: ContextMenu( - items: _buildContextMenuItems(context), + // Lazy: right-clicking a cell is rare relative to how often cells + // get rebuilt (scroll, selection, ...); building the item list + // eagerly on every build was pure waste (#983). + itemsBuilder: _buildContextMenuItems, child: content, ), ), diff --git a/test/features/workspace/result_grid_lazy_context_menu_test.dart b/test/features/workspace/result_grid_lazy_context_menu_test.dart new file mode 100644 index 0000000..37c9950 --- /dev/null +++ b/test/features/workspace/result_grid_lazy_context_menu_test.dart @@ -0,0 +1,73 @@ +import 'package:flutter/gestures.dart'; +import 'package:flutter/material.dart' as material; +import 'package:flutter_test/flutter_test.dart'; +import 'package:querya_desktop/core/theme/querya_theme.dart'; +import 'package:querya_desktop/features/workspace/result_grid_view.dart'; +import 'package:shadcn_flutter/shadcn_flutter.dart'; + +material.Widget _testShell({required material.Widget child}) { + final td = QueryaTheme.darkDefault + .toShadcnThemeData() + .copyWith(platform: () => material.TargetPlatform.linux); + return ShadcnApp( + theme: td, + home: material.Scaffold( + body: child, + ), + ); +} + +Future _secondaryClick(WidgetTester tester, Finder finder) async { + final gesture = await tester.startGesture( + tester.getCenter(finder), + buttons: kSecondaryMouseButton, + ); + await gesture.up(); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 350)); +} + +void main() { + group('VirtualResultGrid lazy context menu items (#983)', () { + testWidgets( + 'scrolling and selecting cells does not build context-menu items; opening one does', + (tester) async { + await tester.pumpWidget( + _testShell( + child: material.SizedBox( + width: 800, + height: 400, + child: VirtualResultGrid( + columns: const ['id', 'name', 'role'], + rows: List.generate( + 50, + (i) => ['$i', 'User $i', 'Admin'], + ), + ), + ), + ), + ); + await tester.pumpAndSettle(); + + final before = gridCellContextMenuItemsBuiltCount; + + // Selecting a cell and scrolling rebuild many cells, but must not + // build any context-menu item list — that's the whole point of #983. + await tester.tap(find.text('User 1')); + await tester.pumpAndSettle(); + await tester.drag( + find.byType(VirtualResultGrid), + const material.Offset(0, -200), + ); + await tester.pumpAndSettle(); + + expect(gridCellContextMenuItemsBuiltCount, before); + + // Right-clicking a cell opens its context menu, which is exactly when + // the (now lazy) item list must be built. + await _secondaryClick(tester, find.text('Admin').first); + + expect(gridCellContextMenuItemsBuiltCount, greaterThan(before)); + }); + }); +} diff --git a/third_party/shadcn_flutter/lib/src/components/menu/context_menu.dart b/third_party/shadcn_flutter/lib/src/components/menu/context_menu.dart index b734cb1..60f540c 100644 --- a/third_party/shadcn_flutter/lib/src/components/menu/context_menu.dart +++ b/third_party/shadcn_flutter/lib/src/components/menu/context_menu.dart @@ -531,8 +531,15 @@ class ContextMenu extends StatefulWidget { /// The child widget that triggers the context menu. final Widget child; - /// Menu items to display in the context menu. - final List items; + /// Menu items to display in the context menu. Exactly one of [items] or + /// [itemsBuilder] must be provided. + final List? items; + + /// Lazily computes menu items right when the menu is about to open, + /// instead of on every build of the widget wrapping [child] — useful when + /// building the item list itself isn't cheap and the menu opens rarely + /// (e.g. once per right-click on a data grid cell, not once per rebuild). + final List Function(BuildContext context)? itemsBuilder; /// How hit testing behaves for the child. final HitTestBehavior behavior; @@ -547,47 +554,66 @@ class ContextMenu extends StatefulWidget { /// /// Parameters: /// - [child] (`Widget`, required): Widget that triggers menu. - /// - [items] (`List`, required): Menu items. + /// - [items] (`List`, required unless [itemsBuilder] is given): Menu items. + /// - [itemsBuilder]: lazy alternative to [items]; see its doc. /// - [behavior] (`HitTestBehavior`, optional): Hit test behavior. /// - [direction] (`Axis`, optional): Menu layout direction. /// - [enabled] (`bool`, optional): Whether menu is enabled. const ContextMenu( {super.key, required this.child, - required this.items, + this.items, + this.itemsBuilder, this.behavior = HitTestBehavior.translucent, this.direction = Axis.vertical, - this.enabled = true}); + this.enabled = true}) + : assert(items != null || itemsBuilder != null, + 'ContextMenu requires either items or itemsBuilder'); @override State createState() => _ContextMenuState(); } class _ContextMenuState extends State { - late ValueNotifier> _children; + ValueNotifier>? _children; @override void initState() { super.initState(); - _children = ValueNotifier(widget.items); + final items = widget.items; + if (items != null) { + _children = ValueNotifier(items); + } } @override void didUpdateWidget(covariant ContextMenu oldWidget) { super.didUpdateWidget(oldWidget); - if (!listEquals(widget.items, oldWidget.items)) { + final items = widget.items; + final oldItems = oldWidget.items; + if (items != null && + oldItems != null && + !listEquals(items, oldItems)) { WidgetsBinding.instance.addPostFrameCallback((timeStamp) { - if (mounted) _children.value = widget.items; + if (mounted) _children?.value = items; }); } } @override void dispose() { - _children.dispose(); + _children?.dispose(); super.dispose(); } + ValueListenable> _resolveChildren(BuildContext context) { + final eager = _children; + if (eager != null) return eager; + // itemsBuilder path: built fresh each time the menu opens, never cached + // across builds of the widget wrapping `child`. + return ValueNotifier(widget.itemsBuilder!(context)); + } + @override Widget build(BuildContext context) { final platform = Theme.of(context).platform; @@ -599,13 +625,13 @@ class _ContextMenuState extends State { onSecondaryTapDown: !widget.enabled ? null : (details) { - _showContextMenu( - context, details.globalPosition, _children, widget.direction); + _showContextMenu(context, details.globalPosition, + _resolveChildren(context), widget.direction); }, onLongPressStart: enableLongPress && widget.enabled ? (details) { - _showContextMenu( - context, details.globalPosition, _children, widget.direction); + _showContextMenu(context, details.globalPosition, + _resolveChildren(context), widget.direction); } : null, child: widget.child,