[cocoon] Migrate LatticeScrollView to TableView from two_dimensional_scrollables - #5138
[cocoon] Migrate LatticeScrollView to TableView from two_dimensional_scrollables#5138andywolff wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the LatticeScrollView to use TableView.builder from the two_dimensional_scrollables package, which greatly simplifies the layout, painting, and hit-testing logic. It also introduces caching for lattice cells in _TaskGridState to prevent redundant matrix regeneration and updates tests to accommodate the new caching behavior. The review feedback suggests simplifying the Shift key detection logic in lattice.dart by using HardwareKeyboard.instance.isShiftPressed directly, and refactoring the private helper _getCells in task_grid.dart to remove the redundant widget parameter.
| bool shouldAcceptUserOffset(ScrollMetrics position) { | ||
| final keys = HardwareKeyboard.instance.logicalKeysPressed; | ||
| final isShiftPressed = | ||
| keys.contains(LogicalKeyboardKey.shiftLeft) || | ||
| keys.contains(LogicalKeyboardKey.shiftRight) || | ||
| HardwareKeyboard.instance.isShiftPressed; | ||
|
|
||
| if (isShiftPressed) { | ||
| return false; | ||
| } | ||
| return super.shouldAcceptUserOffset(position); | ||
| } |
There was a problem hiding this comment.
The manual check of logicalKeysPressed for shiftLeft and shiftRight is redundant because HardwareKeyboard.instance.isShiftPressed already checks both of these keys internally. You can simplify this logic to just use HardwareKeyboard.instance.isShiftPressed directly.
@override
bool shouldAcceptUserOffset(ScrollMetrics position) {
if (HardwareKeyboard.instance.isShiftPressed) {
return false;
}
return super.shouldAcceptUserOffset(position);
}| bool get _isShiftPressed { | ||
| final keys = HardwareKeyboard.instance.logicalKeysPressed; | ||
| return keys.contains(LogicalKeyboardKey.shiftLeft) || | ||
| keys.contains(LogicalKeyboardKey.shiftRight) || | ||
| HardwareKeyboard.instance.isShiftPressed; | ||
| } |
There was a problem hiding this comment.
Similarly, the _isShiftPressed getter can be simplified to directly return HardwareKeyboard.instance.isShiftPressed.
bool get _isShiftPressed => HardwareKeyboard.instance.isShiftPressed;| List<List<LatticeCell>> _getCells(TaskGrid widget) { | ||
| if (_cachedCells != null && | ||
| identical(_lastCommitStatuses, widget.commitStatuses) && | ||
| identical(_lastFilter, widget.filter)) { | ||
| return _cachedCells!; | ||
| } | ||
| _lastCommitStatuses = widget.commitStatuses; | ||
| _lastFilter = widget.filter; | ||
| _cachedCells = _processCommitStatuses(widget); | ||
| return _cachedCells!; | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return LatticeScrollView( | ||
| // TODO(ianh): Provide some vertical scroll physics that disable | ||
| // the clamping in the vertical direction, so that you can keep | ||
| // scrolling past the end instead of hitting a wall every time | ||
| // we load. | ||
| // TODO(ianh): Trigger the loading from the scroll offset, | ||
| // rather than the current hack of loading during build. | ||
| cells: _processCommitStatuses(widget), | ||
| cells: _getCells(widget), | ||
| verticalController: verticalController, | ||
| horizontalController: horizontalController, | ||
| ); |
There was a problem hiding this comment.
The _getCells method is a private helper inside _TaskGridState and does not need to take TaskGrid widget as a parameter, since State already provides direct access to the widget property. Removing this parameter simplifies the method signature and call site.
| List<List<LatticeCell>> _getCells(TaskGrid widget) { | |
| if (_cachedCells != null && | |
| identical(_lastCommitStatuses, widget.commitStatuses) && | |
| identical(_lastFilter, widget.filter)) { | |
| return _cachedCells!; | |
| } | |
| _lastCommitStatuses = widget.commitStatuses; | |
| _lastFilter = widget.filter; | |
| _cachedCells = _processCommitStatuses(widget); | |
| return _cachedCells!; | |
| } | |
| @override | |
| Widget build(BuildContext context) { | |
| return LatticeScrollView( | |
| // TODO(ianh): Provide some vertical scroll physics that disable | |
| // the clamping in the vertical direction, so that you can keep | |
| // scrolling past the end instead of hitting a wall every time | |
| // we load. | |
| // TODO(ianh): Trigger the loading from the scroll offset, | |
| // rather than the current hack of loading during build. | |
| cells: _processCommitStatuses(widget), | |
| cells: _getCells(widget), | |
| verticalController: verticalController, | |
| horizontalController: horizontalController, | |
| ); | |
| List<List<LatticeCell>> _getCells() { | |
| if (_cachedCells != null && | |
| identical(_lastCommitStatuses, widget.commitStatuses) && | |
| identical(_lastFilter, widget.filter)) { | |
| return _cachedCells!; | |
| } | |
| _lastCommitStatuses = widget.commitStatuses; | |
| _lastFilter = widget.filter; | |
| _cachedCells = _processCommitStatuses(widget); | |
| return _cachedCells!; | |
| } | |
| @override | |
| Widget build(BuildContext context) { | |
| return LatticeScrollView( | |
| cells: _getCells(), | |
| verticalController: verticalController, | |
| horizontalController: horizontalController, | |
| ); | |
| } |
1 participant
Fixes flutter/flutter#191490
Replaces the custom
RenderBox-basedLatticeScrollViewlayout engine indashboard/lib/widgets/lattice.dartwithTableView.builderfrom the officialtwo_dimensional_scrollables: ^0.3.0package.I configured
pinnedRowCount: 1andpinnedColumnCount: 1to preserve stickyheader rows and author columns. To maintain high-frame-rate scrolling on Flutter
Web across large grids, I introduced custom lightweight render objects:
_LatticeCellBox(LeafRenderObjectWidget) for standard matrix cells (0 childwidgets, direct canvas painting) and
_LatticeCellChildBox(
SingleChildRenderObjectWidget) for header/icon cells.During testing, I observed two interaction bottlenecks:
_processCommitStatuseswas re-instantiating thousands of cell objects onevery scroll frame. I added identity-based caching in
_TaskGridStateusingidentical(_lastCommitStatuses, widget.commitStatuses).scroll position was less than maximum extent. I added
_LatticeVerticalScrollPhysicsto explicitly reject vertical offsets whenShift is held down (
isShiftPressed == true), enabling instant horizontalShift-scrolling.
RenderBox(LatticeScrollView)TableViewLeafRenderObjectWidgetcanvas painting_getCells)_LatticeVerticalScrollPhysicsAdditionally, I updated
_handleTapUpto compute coordinates relative to pinnedheader extents and current scroll offsets (
scrollX/scrollY), and added supportfor
--dart-define=USE_PRODUCTION_SERVICE=falseindashboard/lib/main.dartfor local release mode testing.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.