selection sheet redesign #6975
selection sheet redesign #6975
Conversation
|
Your Name seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
PR #6975 - Selection Sheet Redesign: Comprehensive Code ReviewOverviewThis PR introduces a significant redesign of the selection sheet with 3,622 additions and 1,723 deletions across 48 files. The main changes include:
🔴 Critical Issues1. PageController Memory Leak (High Priority)Files affected: 19 files including Issue: // Line 94 in file_selection_actions_widget.dart
final PageController _pageController = PageController();The Impact: Memory leaks will accumulate as users navigate through galleries, potentially causing performance degradation and crashes on low-memory devices. Fix Required: @override
void dispose() {
_progressController.close();
_pageController.dispose(); // ADD THIS
widget.selectedFiles.removeListener(_selectFileChangeListener);
super.dispose();
}Affected Files:
2. Timer Usage Anti-Pattern (High Priority)Files affected: 19 files (all gallery pages) Issue: // Line 73 in collection_page.dart
_selectionTimer = Timer(const Duration(milliseconds: 10), () {});The timer is created with an empty callback and serves no purpose except to check Problem:
Better Approach: // Instead of using a timer, use a simple timestamp
int? _lastSelectionChangeTime;
void _onSelectionChanged() {
final hasSelection = _selectedFiles.files.isNotEmpty;
if (hasSelection && !_hasFilesSelected) {
setState(() {
_isCollapsed = false;
_hasFilesSelected = true;
_lastSelectionChangeTime = DateTime.now().millisecondsSinceEpoch;
});
} else if (!hasSelection && _hasFilesSelected) {
setState(() {
_hasFilesSelected = false;
_isCollapsed = false;
_lastSelectionChangeTime = null;
});
}
}
// In scroll notification:
final shouldAllowCollapse = _lastSelectionChangeTime == null ||
(DateTime.now().millisecondsSinceEpoch - _lastSelectionChangeTime! > 100);This eliminates the need for timer disposal and is more straightforward. 3. Potential Race Condition in Scroll HandlingFiles affected: All gallery pages with scroll notifications Issue: // Line 113-120 in collection_page.dart
Future.delayed(const Duration(milliseconds: 10), () {
if (mounted && _hasFilesSelected) {
setState(() {
_isCollapsed = true;
_hasCollapsedOnce = true;
});
}
});Problems:
Fix: // Add a Timer field for the collapse delay
Timer? _collapseTimer;
// In the scroll notification:
if (shouldAllowCollapse && ...) {
_collapseTimer?.cancel();
_collapseTimer = Timer(const Duration(milliseconds: 100), () {
if (mounted && _hasFilesSelected) {
setState(() {
_isCollapsed = true;
_hasCollapsedOnce = true;
});
}
});
}
// In dispose:
_collapseTimer?.cancel();
|
Pull Request Review: Selection Sheet RedesignI have completed a comprehensive review of PR #6975. Overall, the UI redesign implementation looks solid with good attention to animation and user experience. However, there are several important issues that need to be addressed. Critical Issues1. Useless Timer Pattern (Multiple Files) Timer is created with empty callback just to check .isActive later. This is an anti-pattern. Replace with a simpler boolean flag or timestamp-based approach. 2. Race Condition with Future.delayed Future.delayed is not awaited or cancelled, which can cause state updates after widget disposal and multiple delayed callbacks stacking up during rapid scrolling. Store the timer/future and cancel it in dispose or when new scroll events arrive. 3. Inconsistent Collapse Logic The condition ScrollDirection.forward OR ScrollDirection.reverse catches ALL scroll directions (idle is the only other option). This makes the check meaningless. Code Quality Issues4. Redundant State Management 5. Missing Null Safety Force unwrapping primaryDelta with ! can crash if null. Use proper null checking. 6. Hardcoded Color Values Violates the project design system guideline (CLAUDE.md:184-190). Colors should come from getEnteColorScheme(context). 7. Incomplete Comment Known bug in production code with TODO comment. Should either be fixed or tracked properly. Performance Considerations8. Excessive setState Calls 9. AnimatedSize Durations Positive Aspects
Security ConsiderationsNo security issues found. The changes are UI-only and do not affect authentication, encryption, file access permissions, or API security. Recommendations SummaryBefore Merge:
Nice to Have: Pre-Merge ChecklistBefore merging, please ensure:
Let me know if you would like me to help implement any of these fixes! |
…/ente into selection-sheet-redesign
No description provided.