From 4a20615842b8ef967b9d660b92cf5ad731245357 Mon Sep 17 00:00:00 2001 From: Fabian Freund Date: Fri, 29 May 2026 08:17:08 +0200 Subject: [PATCH] ui refactorings --- .../browser/presentation/screens/browser.dart | 61 ++-- .../browser_modules/bottom_app_bar.dart | 109 ++++--- .../widgets/tab_view/tab_tree_view.dart | 272 +++++++++--------- .../widgets/tab_view/tab_view_header.dart | 7 +- .../screens/container_draft_suggestions.dart | 114 +++++--- .../widgets/selectable_chips.dart | 7 + 6 files changed, 320 insertions(+), 250 deletions(-) diff --git a/apps/weblibre/lib/features/geckoview/features/browser/presentation/screens/browser.dart b/apps/weblibre/lib/features/geckoview/features/browser/presentation/screens/browser.dart index e15eb1f2..c64769eb 100644 --- a/apps/weblibre/lib/features/geckoview/features/browser/presentation/screens/browser.dart +++ b/apps/weblibre/lib/features/geckoview/features/browser/presentation/screens/browser.dart @@ -950,7 +950,11 @@ class BrowserScreen extends HookConsumerWidget { ), ) : Consumer( - builder: (context, ref, _) { + // _TabBar is passed via `child` so it is built once and + // reused across toolbar show/hide toggles; only the + // `visible` flag fed to _AnimatedToolbar depends on the + // watched provider. + builder: (context, ref, child) { final toolbarState = ref.watch( toolbarVisibilityControllerProvider(selectedTabId), ); @@ -961,21 +965,21 @@ class BrowserScreen extends HookConsumerWidget { return _AnimatedToolbar( position: TabBarPosition.bottom, visible: visible, - child: _TabBar( - tabBarPosition: TabBarPosition.bottom, - showMainToolbar: - tabBarPosition == TabBarPosition.bottom, - showContextualToolbar: showContextualToolbar, - showQuickTabSwitcherBar: - displayQuickTabSwitcherBar, - isSmallWebMode: false, - pointerMoveEvents: - tabBarPosition == TabBarPosition.bottom - ? pointerMoveEventsController.stream - : null, - ), + child: child!, ); }, + child: _TabBar( + tabBarPosition: TabBarPosition.bottom, + showMainToolbar: + tabBarPosition == TabBarPosition.bottom, + showContextualToolbar: showContextualToolbar, + showQuickTabSwitcherBar: displayQuickTabSwitcherBar, + isSmallWebMode: false, + pointerMoveEvents: + tabBarPosition == TabBarPosition.bottom + ? pointerMoveEventsController.stream + : null, + ), ), ), @@ -986,7 +990,11 @@ class BrowserScreen extends HookConsumerWidget { right: 0, top: 0, child: Consumer( - builder: (context, ref, _) { + // _TabBar is passed via `child` so it is built once and + // reused across toolbar show/hide toggles; only the + // `visible` flag fed to _AnimatedToolbar depends on the + // watched provider. + builder: (context, ref, child) { final toolbarState = ref.watch( toolbarVisibilityControllerProvider(selectedTabId), ); @@ -997,19 +1005,20 @@ class BrowserScreen extends HookConsumerWidget { return _AnimatedToolbar( position: TabBarPosition.top, visible: visible, - child: _TabBar( - tabBarPosition: TabBarPosition.top, - showMainToolbar: true, - showContextualToolbar: showContextualToolbar, - showQuickTabSwitcherBar: displayQuickTabSwitcherBar, - isSmallWebMode: isSmallWebActive, - enableGestures: !isSmallWebActive, - pointerMoveEvents: isSmallWebActive - ? null - : pointerMoveEventsController.stream, - ), + child: child!, ); }, + child: _TabBar( + tabBarPosition: TabBarPosition.top, + showMainToolbar: true, + showContextualToolbar: showContextualToolbar, + showQuickTabSwitcherBar: displayQuickTabSwitcherBar, + isSmallWebMode: isSmallWebActive, + enableGestures: !isSmallWebActive, + pointerMoveEvents: isSmallWebActive + ? null + : pointerMoveEventsController.stream, + ), ), ), diff --git a/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/browser_modules/bottom_app_bar.dart b/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/browser_modules/bottom_app_bar.dart index 55d9a837..b7c953bd 100644 --- a/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/browser_modules/bottom_app_bar.dart +++ b/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/browser_modules/bottom_app_bar.dart @@ -501,6 +501,57 @@ class QuickTabSwitcherItem with FastEquatable { this.depth = 0, }); + /// Builds a switcher entry for an open tab. [sandboxSourceUri] is the + /// canonical source URL when the tab is a sandbox capture (otherwise null), + /// so the bar shows the real site instead of the loopback capture URL. + factory QuickTabSwitcherItem.tab( + TabStateWithContainer state, { + required String? selectedTabId, + required Set pinnedTabIds, + required Map tabDepthById, + required Uri? sandboxSourceUri, + }) { + final (tab, container) = state; + + return QuickTabSwitcherItem( + color: container?.color, + useCustomColor: container?.metadata.useCustomColor ?? false, + id: tab.id, + isActive: tab.id == selectedTabId, + title: sandboxSourceUri != null && tab.title.isEmpty + ? sandboxSourceUri.authority + : tab.titleOrAuthority, + tabMode: tab.tabMode, + isHistory: false, + isPinned: pinnedTabIds.contains(tab.id), + isSandbox: sandboxSourceUri != null, + depth: tabDepthById[tab.id] ?? 0, + url: sandboxSourceUri ?? tab.url, + avatar: TabIcon(tabState: tab, iconSize: 20), + ); + } + + /// Builds a switcher entry for a history suggestion (shown only when there + /// are no open tabs in the active mode). + factory QuickTabSwitcherItem.history({ + required String url, + required String? title, + }) { + final parsedUrl = Uri.parse(url); + + return QuickTabSwitcherItem( + color: null, + id: url, + isActive: false, + title: title ?? parsedUrl.authority, + tabMode: TabMode.regular, + isHistory: true, + isPinned: false, + url: parsedUrl, + avatar: UrlIcon([parsedUrl], iconSize: 20), + ); + } + @override List get hashParameters => [ color, @@ -587,45 +638,25 @@ class QuickTabSwitcher extends HookConsumerWidget { ); final reorderEnabled = effectiveMode == QuickTabSwitcherMode.containerTabs && canManualReorder; - final tabItems = tabStates.value.map((state) { - final sandboxSourceUri = parseSandboxSource( - sandboxCaptureMap[state.$1.id], - ); - final displayUrl = sandboxSourceUri ?? state.$1.url; - final displayTitle = sandboxSourceUri != null && state.$1.title.isEmpty - ? sandboxSourceUri.authority - : state.$1.titleOrAuthority; - return QuickTabSwitcherItem( - color: state.$2?.color, - useCustomColor: state.$2?.metadata.useCustomColor ?? false, - id: state.$1.id, - isActive: state.$1.id == selectedTabId, - title: displayTitle, - tabMode: state.$1.tabMode, - isHistory: false, - isPinned: pinnedTabIds.contains(state.$1.id), - isSandbox: sandboxSourceUri != null, - depth: tabDepthById[state.$1.id] ?? 0, - url: displayUrl, - avatar: TabIcon(tabState: state.$1, iconSize: 20), - ); - }).toList(); - final historyItems = (historySuggestions ?? []).map(( - state, - ) { - final url = Uri.parse(state.url); - return QuickTabSwitcherItem( - color: null, - id: state.url, - isActive: false, - title: state.title ?? url.authority, - tabMode: TabMode.regular, - isHistory: true, - isPinned: false, - url: url, - avatar: UrlIcon([url], iconSize: 20), - ); - }).toList(); + final tabItems = tabStates.value + .map( + (state) => QuickTabSwitcherItem.tab( + state, + selectedTabId: selectedTabId, + pinnedTabIds: pinnedTabIds, + tabDepthById: tabDepthById, + sandboxSourceUri: parseSandboxSource( + sandboxCaptureMap[state.$1.id], + ), + ), + ) + .toList(); + final historyItems = (historySuggestions ?? []) + .map( + (visit) => + QuickTabSwitcherItem.history(url: visit.url, title: visit.title), + ) + .toList(); final availableItems = [...tabItems, ...historyItems]; final activeItem = availableItems.isEmpty diff --git a/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_tree_view.dart b/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_tree_view.dart index 49432ab9..7320cbfc 100644 --- a/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_tree_view.dart +++ b/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_tree_view.dart @@ -192,138 +192,9 @@ class ViewTabTreesWidget extends HookConsumerWidget { children: [ TabViewHeader(onClose: onClose, tabsViewMode: TabsViewMode.tree), Expanded( - child: HookConsumer( - builder: (context, ref, child) { - final screenWidth = MediaQuery.of(context).size.width; - final disableAnimations = MediaQuery.disableAnimationsOf( - context, - ); - - final containerId = ref.watch(selectedContainerProvider); - - final filteredTabEntities = ref.watch( - seamlessFilteredTabEntitiesProvider( - searchPartition: TabSearchPartition.preview, - // ignore: document_ignores using fast equatable - // ignore: provider_parameters - containerFilter: ContainerFilterById( - containerId: containerId, - ), - groupTrees: true, - ), - ); - - final activeTab = ref.watch(selectedTabProvider); - - final crossAxisCount = useMemoized(() { - final calculatedCount = calculateCrossAxisItemCount( - screenWidth: screenWidth, - horizontalPadding: 4.0, - crossAxisSpacing: 8.0, - ); - - return math.max( - math.min( - calculatedCount, - filteredTabEntities.value.length, - ), - 2, - ); - }, [screenWidth, filteredTabEntities.value.length]); - - final itemSize = useMemoized( - () => calculateItemSize( - screenWidth: screenWidth, - childAspectRatio: 0.75, - horizontalPadding: 4.0, - crossAxisSpacing: 8.0, - crossAxisCount: crossAxisCount, - mainAxisSpacing: 8.0, - ), - [screenWidth, crossAxisCount], - ); - - useEffect(() { - WidgetsBinding.instance.addPostFrameCallback((_) { - if (!scrollController.hasClients || activeTab == null) { - return; - } - - final index = filteredTabEntities.value.indexWhere( - (entity) => entity.tabId == activeTab, - ); - - if (index < 0) return; - - final row = index ~/ 2; - final tabStart = row * itemSize.height; - final viewportDimension = - scrollController.position.viewportDimension; - - final targetOffset = - (tabStart - - viewportDimension / 2 + - itemSize.height / 2) - .clamp( - 0.0, - scrollController.position.maxScrollExtent, - ); - - if (targetOffset == scrollController.offset) return; - - if (disableAnimations) { - scrollController.jumpTo(targetOffset); - } else { - unawaited( - scrollController.animateTo( - targetOffset, - duration: const Duration(milliseconds: 200), - curve: Curves.easeInOut, - ), - ); - } - }); - - return null; - }, [filteredTabEntities, activeTab]); - - final tabs = useMemoized(() { - return filteredTabEntities.value - .whereType() - .map((entity) { - return _TabTreePreview( - entity: entity, - activeTabId: activeTab, - onClose: onClose, - stackPadding: const Offset(8, 8), - ); - }) - .toList(); - }, [filteredTabEntities, activeTab]); - - return Padding( - padding: const EdgeInsets.symmetric(horizontal: 4.0), - child: FadingScroll( - controller: scrollController, - fadingSize: 5, - builder: (context, controller) { - return GridView.builder( - controller: controller, - padding: const EdgeInsets.only(bottom: 56), - gridDelegate: SliverGridDelegateWithFixedCrossAxisCount( - //Sync values for itemHeight calculation _calculateItemHeight - childAspectRatio: 0.75, - mainAxisSpacing: 8.0, - crossAxisSpacing: 8.0, - crossAxisCount: crossAxisCount, - ), - itemCount: tabs.length, - itemBuilder: (context, index) => tabs[index], - ); - }, - ), - ); - }, + child: _TabTreesGrid( + scrollController: scrollController, + onClose: onClose, ), ), ], @@ -353,3 +224,140 @@ class ViewTabTreesWidget extends HookConsumerWidget { ); } } + +/// Scrollable grid of tab-tree previews. +/// +/// Extracted into its own [HookConsumerWidget] (rather than an inline +/// `HookConsumer`) so the heavy `seamlessFilteredTabEntitiesProvider` watch +/// and the layout/scroll-sync hook state live in a stable, dedicated element +/// instead of an anonymous builder closure. This keeps the surrounding +/// [TabViewHeader] and FAB out of this subtree's rebuild scope. +class _TabTreesGrid extends HookConsumerWidget { + final ScrollController scrollController; + final VoidCallback onClose; + + const _TabTreesGrid({required this.scrollController, required this.onClose}); + + @override + Widget build(BuildContext context, WidgetRef ref) { + final screenWidth = MediaQuery.of(context).size.width; + final disableAnimations = MediaQuery.disableAnimationsOf(context); + + final containerId = ref.watch(selectedContainerProvider); + + final filteredTabEntities = ref.watch( + seamlessFilteredTabEntitiesProvider( + searchPartition: TabSearchPartition.preview, + // ignore: document_ignores using fast equatable + // ignore: provider_parameters + containerFilter: ContainerFilterById(containerId: containerId), + groupTrees: true, + ), + ); + + final activeTab = ref.watch(selectedTabProvider); + + final crossAxisCount = useMemoized(() { + final calculatedCount = calculateCrossAxisItemCount( + screenWidth: screenWidth, + horizontalPadding: 4.0, + crossAxisSpacing: 8.0, + ); + + return math.max( + math.min(calculatedCount, filteredTabEntities.value.length), + 2, + ); + }, [screenWidth, filteredTabEntities.value.length]); + + final itemSize = useMemoized( + () => calculateItemSize( + screenWidth: screenWidth, + childAspectRatio: 0.75, + horizontalPadding: 4.0, + crossAxisSpacing: 8.0, + crossAxisCount: crossAxisCount, + mainAxisSpacing: 8.0, + ), + [screenWidth, crossAxisCount], + ); + + useEffect(() { + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!scrollController.hasClients || activeTab == null) { + return; + } + + final index = filteredTabEntities.value.indexWhere( + (entity) => entity.tabId == activeTab, + ); + + if (index < 0) return; + + final row = index ~/ 2; + final tabStart = row * itemSize.height; + final viewportDimension = + scrollController.position.viewportDimension; + + final targetOffset = + (tabStart - viewportDimension / 2 + itemSize.height / 2).clamp( + 0.0, + scrollController.position.maxScrollExtent, + ); + + if (targetOffset == scrollController.offset) return; + + if (disableAnimations) { + scrollController.jumpTo(targetOffset); + } else { + unawaited( + scrollController.animateTo( + targetOffset, + duration: const Duration(milliseconds: 200), + curve: Curves.easeInOut, + ), + ); + } + }); + + return null; + }, [filteredTabEntities, activeTab]); + + final tabs = useMemoized(() { + return filteredTabEntities.value + .whereType() + .map((entity) { + return _TabTreePreview( + entity: entity, + activeTabId: activeTab, + onClose: onClose, + stackPadding: const Offset(8, 8), + ); + }) + .toList(); + }, [filteredTabEntities, activeTab]); + + return Padding( + padding: const EdgeInsets.symmetric(horizontal: 4.0), + child: FadingScroll( + controller: scrollController, + fadingSize: 5, + builder: (context, controller) { + return GridView.builder( + controller: controller, + padding: const EdgeInsets.only(bottom: 56), + gridDelegate: SliverGridDelegateWithFixedCrossAxisCount( + //Sync values for itemHeight calculation _calculateItemHeight + childAspectRatio: 0.75, + mainAxisSpacing: 8.0, + crossAxisSpacing: 8.0, + crossAxisCount: crossAxisCount, + ), + itemCount: tabs.length, + itemBuilder: (context, index) => tabs[index], + ); + }, + ), + ); + } +} diff --git a/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_view_header.dart b/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_view_header.dart index 6c3b7155..fa990e28 100644 --- a/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_view_header.dart +++ b/apps/weblibre/lib/features/geckoview/features/browser/presentation/widgets/tab_view/tab_view_header.dart @@ -1142,12 +1142,7 @@ class TabViewHeader extends HookConsumerWidget { ), ), const Divider(), - if (showContainerUi) - Consumer( - builder: (context, ref, child) { - return _TabFilters(tabsViewMode: tabsViewMode); - }, - ), + if (showContainerUi) _TabFilters(tabsViewMode: tabsViewMode), const SizedBox(height: 8), ], ), diff --git a/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_draft_suggestions.dart b/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_draft_suggestions.dart index aa51b2d9..41524974 100644 --- a/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_draft_suggestions.dart +++ b/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_draft_suggestions.dart @@ -52,6 +52,24 @@ class ContainerDraftSuggestionsScreen extends HookConsumerWidget { ? selectedContainerTabs.value[selectedContainer.value!] : null; + final screenWidth = MediaQuery.of(context).size.width; + + final crossAxisCount = useMemoized(() { + final calculatedCount = calculateCrossAxisItemCount( + screenWidth: screenWidth, + horizontalPadding: 4.0, + crossAxisSpacing: 8.0, + ); + + return math.max( + math.min( + calculatedCount, + selectedContainer.value?.tabIds.length ?? 0, + ), + 2, + ); + }, [screenWidth, selectedContainer.value?.tabIds.length]); + return Scaffold( appBar: AppBar(title: const Text('Draft Containers')), body: SafeArea( @@ -60,24 +78,6 @@ class ContainerDraftSuggestionsScreen extends HookConsumerWidget { child: suggestionsAsync.when( skipLoadingOnReload: true, data: (suggestions) { - final screenWidth = MediaQuery.of(context).size.width; - - final crossAxisCount = useMemoized(() { - final calculatedCount = calculateCrossAxisItemCount( - screenWidth: screenWidth, - horizontalPadding: 4.0, - crossAxisSpacing: 8.0, - ); - - return math.max( - math.min( - calculatedCount, - selectedContainer.value?.tabIds.length ?? 0, - ), - 2, - ); - }, [screenWidth, selectedContainer.value?.tabIds.length]); - return Column( children: [ SizedBox( @@ -88,35 +88,10 @@ class ContainerDraftSuggestionsScreen extends HookConsumerWidget { itemId: (container) => container, itemAvatar: (container) => const Icon(MdiIcons.creation, size: 20), - itemLabel: (container) => HookConsumer( - builder: (context, ref, child) { - final items = useListenableSelector( - selectedContainerTabs, - () => selectedContainerTabs.value[container], - ); - - final topic = items.isNotEmpty - ? ref.watch( - tabsTopicProvider(EquatableValue(items!)), - ) - : AsyncValue.data(container.topic); - - return topic.when( - skipLoadingOnReload: true, - data: (topic) => Text(topic ?? 'Untitled'), - error: (error, stackTrace) { - logger.e( - 'Failed predicting selected tabs topic', - error: error, - stackTrace: stackTrace, - ); - - return Text(container.topic ?? 'Untitled'); - }, - loading: () => - const Skeletonizer(child: Text('Untitled')), - ); - }, + itemLabel: (container) => _SuggestedContainerLabel( + key: ValueKey(container), + container: container, + selectedContainerTabs: selectedContainerTabs, ), itemBadgeCount: (container) => container.tabIds.length, availableItems: suggestions!, @@ -237,3 +212,48 @@ class ContainerDraftSuggestionsScreen extends HookConsumerWidget { ); } } + +/// Label for a single suggested-container chip. +/// +/// Extracted into its own [HookConsumerWidget] (rather than an inline +/// `HookConsumer` in `itemLabel`) so its hook state is bound to a stable +/// element identity per container, and the heavy `tabsTopicProvider` watch +/// is scoped to just this chip. +class _SuggestedContainerLabel extends HookConsumerWidget { + final SuggestedContainer container; + final ValueNotifier>> + selectedContainerTabs; + + const _SuggestedContainerLabel({ + required this.container, + required this.selectedContainerTabs, + super.key, + }); + + @override + Widget build(BuildContext context, WidgetRef ref) { + final items = useListenableSelector( + selectedContainerTabs, + () => selectedContainerTabs.value[container], + ); + + final topic = items.isNotEmpty + ? ref.watch(tabsTopicProvider(EquatableValue(items!))) + : AsyncValue.data(container.topic); + + return topic.when( + skipLoadingOnReload: true, + data: (topic) => Text(topic ?? 'Untitled'), + error: (error, stackTrace) { + logger.e( + 'Failed predicting selected tabs topic', + error: error, + stackTrace: stackTrace, + ); + + return Text(container.topic ?? 'Untitled'); + }, + loading: () => const Skeletonizer(child: Text('Untitled')), + ); + } +} diff --git a/apps/weblibre/lib/presentation/widgets/selectable_chips.dart b/apps/weblibre/lib/presentation/widgets/selectable_chips.dart index a4c3711f..b920ee08 100644 --- a/apps/weblibre/lib/presentation/widgets/selectable_chips.dart +++ b/apps/weblibre/lib/presentation/widgets/selectable_chips.dart @@ -145,6 +145,11 @@ class SelectableChips extends StatelessWidget { final GlobalKey? activeItemKey; final double? cacheExtent; + /// Key applied to the underlying scroll view. Supply a [PageStorageKey] to + /// preserve the scroll offset across rebuilds/remounts (e.g. when a host + /// widget is torn down and recreated by a bottom sheet open/close). + final Key? scrollKey; + final K Function(S item) itemId; final Widget Function(T item) itemLabel; final Widget? Function(T item)? itemAvatar; @@ -187,6 +192,7 @@ class SelectableChips extends StatelessWidget { this.scrollController, this.activeItemKey, this.cacheExtent = 0, + this.scrollKey, super.key, }); @@ -296,6 +302,7 @@ class SelectableChips extends StatelessWidget { builder: (context, controller) { if (onReorder == null) { return ListView.builder( + key: scrollKey, controller: controller, scrollCacheExtent: cacheExtent.mapNotNull( (extent) => ScrollCacheExtent.pixels(extent),