diff --git a/lib/screens/profile/profile_switch_screen.dart b/lib/screens/profile/profile_switch_screen.dart index 74f4ce11..21cc50dd 100644 --- a/lib/screens/profile/profile_switch_screen.dart +++ b/lib/screens/profile/profile_switch_screen.dart @@ -55,32 +55,27 @@ class _ProfileSwitchScreenState extends State with MountedS bool _focusRequested = false; bool _switching = false; Stream? _viewStream; - StorageService? _viewStreamStorage; - StorageService? _storage; - Future? _storageFuture; @override void didChangeDependencies() { super.didChangeDependencies(); _ensureViewStream(); - if (_storage == null) { - unawaited( - (_storageFuture ??= StorageService.getInstance()).then((s) { - setStateIfMounted(() => _storage = s); - }), - ); - } } + /// Built exactly once. Resolving [StorageService] asynchronously here used + /// to rebuild the stream a microtask after first paint, and because storage + /// is what supplies profile recency the second view arrived re-sorted — the + /// reorder then rebound the tiles' focus nodes and dropped the D-pad + /// highlight onto the enclosing scope (#1792). Storage is already resolved + /// before any route exists, so read it from the provider graph instead. void _ensureViewStream() { - if (_viewStream != null && identical(_viewStreamStorage, _storage)) return; - _viewStreamStorage = _storage; + if (_viewStream != null) return; _viewStream = watchProfilesView( profiles: context.read(), profileConnections: context.read(), connections: context.read(), plexHome: context.read(), - storage: _storage, + storage: context.read(), ); } @@ -220,71 +215,87 @@ class _ProfileSwitchScreenState extends State with MountedS } SliverList _profileList(List profiles, ProfilesView view, String? activeId, {required bool autofocusFirst}) { + // The tile keys and `findChildIndexCallback` below are one mechanism, not + // two independent safeguards. A refreshed profile source can re-sort this + // list after first paint; the key stops the sliver handing a tile's + // Element the next profile's focus node, and the lookup lets it map that + // key to its new index. Without the lookup the tile is destroyed and + // re-inflated instead of moved, which keeps primary focus but resets + // FocusableWrapper's chrome — a focused tile with no highlight (#1792). return SliverList( - delegate: SliverChildBuilderDelegate((context, index) { - final profile = profiles[index]; - final isActive = profile.id == activeId; - final tokensRef = tokens(context); - final tileRadii = groupItemRadii(context, index, profiles.length); - final isFirstSelectable = autofocusFirst && index == 0; - final profileFocusNode = _profileFocusNode(profile); - final menuFocusNode = _profileMenuFocusNode(profile); - final menuKey = _profileMenuKey(profile); - // All tile actions are disabled while a switch is binding: the - // overlay's barrier blocks pointers but not DPAD key events, and a - // Manage/Delete flow racing the in-flight switch corrupts state - // (e.g. a delete confirmation left open when the switch settles). - final actionsEnabled = !widget.requireSelection && !_switching; - final onManage = actionsEnabled ? () => _manageProfile(profile) : null; - final onDelete = profile.isLocal && actionsEnabled ? () => _deleteProfile(profile) : null; - final onSignOut = profile.isPlexHome && profile.parentConnectionId != null && actionsEnabled - ? () => _signOutPlexAccount(profile) - : null; - final hasMenu = onManage != null || onDelete != null || onSignOut != null; + delegate: SliverChildBuilderDelegate( + (context, index) { + final profile = profiles[index]; + final isActive = profile.id == activeId; + final tokensRef = tokens(context); + final tileRadii = groupItemRadii(context, index, profiles.length); + final isFirstSelectable = autofocusFirst && index == 0; + final profileFocusNode = _profileFocusNode(profile); + final menuFocusNode = _profileMenuFocusNode(profile); + final menuKey = _profileMenuKey(profile); + // All tile actions are disabled while a switch is binding: the + // overlay's barrier blocks pointers but not DPAD key events, and a + // Manage/Delete flow racing the in-flight switch corrupts state + // (e.g. a delete confirmation left open when the switch settles). + final actionsEnabled = !widget.requireSelection && !_switching; + final onManage = actionsEnabled ? () => _manageProfile(profile) : null; + final onDelete = profile.isLocal && actionsEnabled ? () => _deleteProfile(profile) : null; + final onSignOut = profile.isPlexHome && profile.parentConnectionId != null && actionsEnabled + ? () => _signOutPlexAccount(profile) + : null; + final hasMenu = onManage != null || onDelete != null || onSignOut != null; - if (isFirstSelectable && !_focusRequested) { - _focusRequested = true; - WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted) profileFocusNode.requestFocus(); - }); - } + if (isFirstSelectable && !_focusRequested) { + _focusRequested = true; + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted) profileFocusNode.requestFocus(); + }); + } - return Padding( - padding: EdgeInsets.fromLTRB(16, index == 0 ? 4 : tokensRef.groupGap, 16, 0), - child: FocusableWrapper( - autofocus: isFirstSelectable, - focusNode: profileFocusNode, - disableScale: true, - borderRadii: tileRadii, - enableLongPress: hasMenu, - onLongPress: hasMenu ? () => _openProfileMenu(profile) : null, - onNavigateRight: hasMenu ? () => menuFocusNode.requestFocus() : null, - onSelect: _switching || (isActive && !widget.requireSelection) ? null : () => _switchTo(profile), - child: Card( - shape: RoundedRectangleBorder(borderRadius: tileRadii), - clipBehavior: Clip.antiAlias, - child: _ProfileTile( - borderRadius: tileRadii, - profile: profile, - avatarUrl: view.avatarUrlByProfile[profile.id], - isActive: isActive && !widget.requireSelection, - chips: _chipsFor(profile, view), - onTap: () => _switchTo(profile), - onLongPress: hasMenu ? () => _openProfileMenu(profile) : null, - // Manage available for any profile — adding/removing - // borrowed connections is supported on plex_home too. Delete - // stays local-only (Plex Home users are owned by Plex). - onManage: onManage, - onDelete: onDelete, - onSignOut: onSignOut, - menuFocusNode: menuFocusNode, - menuKey: menuKey, - onMenuNavigateLeft: () => profileFocusNode.requestFocus(), + return Padding( + key: ValueKey(profile.id), + padding: EdgeInsets.fromLTRB(16, index == 0 ? 4 : tokensRef.groupGap, 16, 0), + child: FocusableWrapper( + autofocus: isFirstSelectable, + focusNode: profileFocusNode, + disableScale: true, + borderRadii: tileRadii, + enableLongPress: hasMenu, + onLongPress: hasMenu ? () => _openProfileMenu(profile) : null, + onNavigateRight: hasMenu ? () => menuFocusNode.requestFocus() : null, + onSelect: _switching || (isActive && !widget.requireSelection) ? null : () => _switchTo(profile), + child: Card( + shape: RoundedRectangleBorder(borderRadius: tileRadii), + clipBehavior: Clip.antiAlias, + child: _ProfileTile( + borderRadius: tileRadii, + profile: profile, + avatarUrl: view.avatarUrlByProfile[profile.id], + isActive: isActive && !widget.requireSelection, + chips: _chipsFor(profile, view), + onTap: () => _switchTo(profile), + onLongPress: hasMenu ? () => _openProfileMenu(profile) : null, + // Manage available for any profile — adding/removing + // borrowed connections is supported on plex_home too. Delete + // stays local-only (Plex Home users are owned by Plex). + onManage: onManage, + onDelete: onDelete, + onSignOut: onSignOut, + menuFocusNode: menuFocusNode, + menuKey: menuKey, + onMenuNavigateLeft: () => profileFocusNode.requestFocus(), + ), ), ), - ), - ); - }, childCount: profiles.length), + ); + }, + childCount: profiles.length, + findChildIndexCallback: (key) { + final id = (key as ValueKey).value; + final index = profiles.indexWhere((profile) => profile.id == id); + return index < 0 ? null : index; + }, + ), ); } diff --git a/test/screens/profile/profile_switch_screen_test.dart b/test/screens/profile/profile_switch_screen_test.dart index 07819f7e..6b6ea9ee 100644 --- a/test/screens/profile/profile_switch_screen_test.dart +++ b/test/screens/profile/profile_switch_screen_test.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:drift/native.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; @@ -5,6 +7,8 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:plezy/connection/connection.dart'; import 'package:plezy/connection/connection_registry.dart'; import 'package:plezy/database/app_database.dart'; +import 'package:plezy/focus/focusable_wrapper.dart'; +import 'package:plezy/focus/input_mode_tracker.dart'; import 'package:plezy/i18n/strings.g.dart'; import 'package:plezy/profiles/active_profile_provider.dart'; import 'package:plezy/profiles/plex_home_service.dart'; @@ -16,6 +20,7 @@ import 'package:plezy/profiles/profile_registry.dart'; import 'package:plezy/screens/profile/profile_switch_screen.dart'; import 'package:plezy/services/storage_service.dart'; import 'package:plezy/theme/mono_theme.dart'; +import 'package:plezy/utils/platform_detector.dart'; import 'package:provider/provider.dart'; import '../../test_helpers/prefs.dart'; @@ -58,6 +63,7 @@ void main() { TranslationProvider( child: MultiProvider( providers: [ + Provider.value(value: storage), Provider.value(value: profiles), Provider.value(value: profileConnections), Provider.value(value: connections), @@ -121,6 +127,7 @@ void main() { TranslationProvider( child: MultiProvider( providers: [ + Provider.value(value: storage), Provider.value(value: profiles), Provider.value(value: profileConnections), Provider.value(value: connections), @@ -188,6 +195,7 @@ void main() { TranslationProvider( child: MultiProvider( providers: [ + Provider.value(value: storage), Provider.value(value: profiles), Provider.value(value: profileConnections), Provider.value(value: connections), @@ -216,6 +224,177 @@ void main() { await tester.pumpWidget(const SizedBox.shrink()); await tester.pump(); }); + + testWidgets('paints the recency order on the first frame that shows profiles', (tester) async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + final profiles = _FakeProfileRegistry(db, [ + Profile.local(id: 'local-owner', displayName: 'Owner', createdAt: DateTime(2026, 1, 1)), + Profile.local(id: 'local-kids', displayName: 'Kids', createdAt: DateTime(2026, 1, 2)), + ]); + final connections = _FakeConnectionRegistry(db); + final profileConnections = _FakeProfileConnectionRegistry(db); + final storage = await StorageService.getInstance(); + await storage.markProfileUsed('local-kids', DateTime(2026, 1, 3)); + final plexHome = PlexHomeService( + connections: connections, + profileConnections: profileConnections, + storage: storage, + plexHomeUserFetcher: (_) async => const [], + ); + final activeProfile = ActiveProfileProvider( + registry: profiles, + plexHome: plexHome, + connections: connections, + profileConnections: profileConnections, + storage: storage, + ); + addTearDown(() async { + activeProfile.dispose(); + await plexHome.dispose(); + await db.close(); + }); + + await tester.pumpWidget( + TranslationProvider( + child: MultiProvider( + providers: [ + Provider.value(value: storage), + Provider.value(value: profiles), + Provider.value(value: profileConnections), + Provider.value(value: connections), + Provider.value(value: plexHome), + ChangeNotifierProvider.value(value: activeProfile), + ], + child: MaterialApp(theme: monoTheme(dark: true), home: const ProfileSwitchScreen()), + ), + ), + ); + + // Frame-by-frame, not pumpAndSettle: the regression was an intermediate + // order that only existed for one frame, which a settled assertion cannot + // see (#1792). + final paintedOrders = []; + for (var frame = 0; frame < 6; frame++) { + await tester.pump(const Duration(milliseconds: 16)); + final order = _visibleProfileNames(tester); + if (order.isNotEmpty) paintedOrders.add(order.join(',')); + } + + expect(paintedOrders, isNotEmpty, reason: 'the picker never painted a profile'); + expect(paintedOrders.toSet(), {'Kids,Owner'}, reason: 'the recency sort must not arrive a frame late'); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'ProfileTile:local-kids'); + + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pump(); + }); + + testWidgets('keeps the focused tile highlighted when the list re-sorts after first paint', (tester) async { + TvDetectionService.debugSetAppleTVOverride(true); + PlatformDetector.debugSetIsDesktopOSOverride(false); + addTearDown(() { + TvDetectionService.debugSetAppleTVOverride(null); + PlatformDetector.debugSetIsDesktopOSOverride(null); + }); + + final db = AppDatabase.forTesting(NativeDatabase.memory()); + final owner = Profile.local(id: 'local-owner', displayName: 'Owner', createdAt: DateTime(2026, 1, 1)); + final kids = Profile.local(id: 'local-kids', displayName: 'Kids', createdAt: DateTime(2026, 1, 2)); + final profiles = _MutableProfileRegistry(db, [owner, kids]); + final connections = _FakeConnectionRegistry(db); + final profileConnections = _FakeProfileConnectionRegistry(db); + final storage = await StorageService.getInstance(); + final plexHome = PlexHomeService( + connections: connections, + profileConnections: profileConnections, + storage: storage, + plexHomeUserFetcher: (_) async => const [], + ); + final activeProfile = ActiveProfileProvider( + registry: profiles, + plexHome: plexHome, + connections: connections, + profileConnections: profileConnections, + storage: storage, + ); + addTearDown(() async { + activeProfile.dispose(); + await plexHome.dispose(); + await profiles.close(); + await db.close(); + }); + + await tester.pumpWidget( + InputModeTracker( + child: TranslationProvider( + child: MultiProvider( + providers: [ + Provider.value(value: storage), + Provider.value(value: profiles), + Provider.value(value: profileConnections), + Provider.value(value: connections), + Provider.value(value: plexHome), + ChangeNotifierProvider.value(value: activeProfile), + ], + child: MaterialApp(theme: monoTheme(dark: true), home: const ProfileSwitchScreen()), + ), + ), + ), + ); + await tester.pumpAndSettle(); + + expect(_visibleProfileNames(tester), ['Owner', 'Kids']); + final focused = FocusManager.instance.primaryFocus; + expect(focused?.debugLabel, 'ProfileTile:local-owner'); + expect(_tileIsHighlighted(tester, 'Owner'), isTrue, reason: 'the focused tile starts highlighted'); + + // A refreshed profile source can re-sort the list after first paint. The + // tile must move with its focus node instead of the node being rebound to + // whatever profile now occupies its index. + profiles.emit([ + owner, + Profile.local( + id: 'local-kids', + displayName: 'Kids', + createdAt: DateTime(2026, 1, 2), + lastUsedAt: DateTime(2026, 1, 3), + ), + ]); + await tester.pumpAndSettle(); + + expect(_visibleProfileNames(tester), ['Kids', 'Owner'], reason: 'the emission should have re-sorted the list'); + expect(FocusManager.instance.primaryFocus, same(focused), reason: 'the reorder must not move focus off the tile'); + expect(_tileIsHighlighted(tester, 'Owner'), isTrue, reason: 'the focused tile must still draw focus chrome'); + + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pump(); + }); +} + +/// Tile labels in painted order. Only the two names the reorder tests seed are +/// considered, so surrounding chrome text cannot pollute the sequence. +List _visibleProfileNames(WidgetTester tester) { + return tester + .widgetList(find.byType(Text)) + .map((text) => text.data) + .where((label) => label == 'Owner' || label == 'Kids') + .cast() + .toList(); +} + +/// Whether the tile showing [name] draws its focus border. +/// +/// [FocusableWrapper] paints an opaque border only while it believes it holds +/// focus, so this catches the state where primary focus is correct but the +/// wrapper's chrome was reset by a rebuilt element. +bool _tileIsHighlighted(WidgetTester tester, String name) { + final wrapper = find.ancestor(of: find.text(name), matching: find.byType(FocusableWrapper)); + final containers = tester.widgetList( + find.descendant(of: wrapper, matching: find.byType(AnimatedContainer)), + ); + return containers.any((container) { + final border = (container.decoration as BoxDecoration?)?.border?.top; + return border != null && border.style != BorderStyle.none && border.color.a > 0; + }); } class _FakeProfileRegistry extends ProfileRegistry { @@ -230,6 +409,31 @@ class _FakeProfileRegistry extends ProfileRegistry { Future> list() async => _profiles; } +/// Profile registry whose stream keeps emitting, so a test can re-sort the +/// list after first paint the way a refreshed profile source does. +class _MutableProfileRegistry extends ProfileRegistry { + _MutableProfileRegistry(super.db, this._profiles); + + List _profiles; + final StreamController> _controller = StreamController>.broadcast(); + + @override + Stream> watchProfiles() async* { + yield _profiles; + yield* _controller.stream; + } + + @override + Future> list() async => _profiles; + + void emit(List profiles) { + _profiles = profiles; + _controller.add(profiles); + } + + Future close() => _controller.close(); +} + class _FakeConnectionRegistry extends ConnectionRegistry { final List _connections;