Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/features/workspace/result_grid_view.dart
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ List<double> 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<double> distributeResultGridSpareWidth({
Expand Down Expand Up @@ -2794,6 +2799,7 @@ class _GridCell extends material.StatelessWidget {
onRevertRow == other.onRevertRow;

List<MenuItem> _buildContextMenuItems(material.BuildContext context) {
gridCellContextMenuItemsBuiltCount++;
final preview = text.length > 24 ? '${text.substring(0, 22)}…' : text;
final isNull = text == 'NULL';
final numVal = num.tryParse(text);
Expand Down Expand Up @@ -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,
),
),
Expand Down
73 changes: 73 additions & 0 deletions test/features/workspace/result_grid_lazy_context_menu_test.dart
Original file line number Diff line number Diff line change
@@ -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<void> _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));
});
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<MenuItem> items;
/// Menu items to display in the context menu. Exactly one of [items] or
/// [itemsBuilder] must be provided.
final List<MenuItem>? 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<MenuItem> Function(BuildContext context)? itemsBuilder;

/// How hit testing behaves for the child.
final HitTestBehavior behavior;
Expand All @@ -547,47 +554,66 @@ class ContextMenu extends StatefulWidget {
///
/// Parameters:
/// - [child] (`Widget`, required): Widget that triggers menu.
/// - [items] (`List<MenuItem>`, required): Menu items.
/// - [items] (`List<MenuItem>`, 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<ContextMenu> createState() => _ContextMenuState();
}

class _ContextMenuState extends State<ContextMenu> {
late ValueNotifier<List<MenuItem>> _children;
ValueNotifier<List<MenuItem>>? _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<List<MenuItem>> _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;
Expand All @@ -599,13 +625,13 @@ class _ContextMenuState extends State<ContextMenu> {
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,
Expand Down
Loading