perf(detail): paint a show before its on-deck episode is looked up
Jellyfin has no equivalent of Plex's bundled `?includeOnDeck=1`, so a show detail open chained `/Shows/NextUp` behind the item fetch and the screen sat on a spinner for both round trips. The second one is not needed to paint: everything except the play button's episode label comes from the item. `fetchItemWithOnDeck` now takes an `onItemReady` callback and invokes it as soon as the item is known, when that is strictly before on-deck settles. Plex returns both together and never invokes it. Phone and desktop only. TV keeps its own reveal gate — `_isTvDetailReadyToReveal` holds the foreground at opacity 0 until extras, related hubs, seasons and the first episode page have all loaded, and those still run after the on-deck lookup settles, so TV sees no change. Both halves are pinned by tests. Measured on a remote Jellyfin server, 15 interleaved show-detail opens per version: time to content 1264ms -> 1042ms (-18%), with the rest of the load unchanged. Seasons and extras deliberately still start after the whole lookup settles. Starting them at the early paint measured worse (time to settled +21%) because they contend with the on-deck request instead of overlapping it — the same reason `/Shows/NextUp` is not fired in parallel with the item fetch. That trade-off is also why TV was left alone rather than being unblocked by moving those loads earlier. Two ordering hazards the early paint introduces, both covered by `media_detail_screen_test.dart`: - The early call must not write on-deck. `_loadFullMetadata` runs again after playback, and clearing there would blank the play button for the length of the round trip. `onDeckSettled` marks the authoritative write, so a reload that finds the series finished still clears it. - A settled empty on-deck must not drop the episode-derived fallback that `_ensureFallbackOnDeckEpisode` supplies. close #1784
This commit is contained in:
@@ -167,13 +167,24 @@ abstract class MediaServerClient {
|
||||
Future<MediaItem?> fetchItem(String id);
|
||||
|
||||
/// Fetch a single item *and* its on-deck episode (the next unwatched /
|
||||
/// in-progress episode) in one round-trip when the backend supports it.
|
||||
/// The item follows [fetchItem]'s error contract: an online HTTP 404 returns
|
||||
/// both nullable fields as `null`, while every other HTTP status throws.
|
||||
/// Plex bundles both via `/library/metadata/{id}?includeOnDeck=1`;
|
||||
/// Jellyfin has no equivalent endpoint and returns `onDeckEpisode: null`,
|
||||
/// leaving callers to fetch on-deck separately if they need it.
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id);
|
||||
/// in-progress episode). The item follows [fetchItem]'s error contract: an
|
||||
/// online HTTP 404 returns both nullable fields as `null`, while every other
|
||||
/// HTTP status throws.
|
||||
///
|
||||
/// Plex bundles both via `/library/metadata/{id}?includeOnDeck=1`. Jellyfin
|
||||
/// has no equivalent endpoint and needs a second request for on-deck, so it
|
||||
/// would otherwise hold the item behind a round trip the detail screen does
|
||||
/// not need in order to paint.
|
||||
///
|
||||
/// [onItemReady] exists for exactly that case: implementations invoke it as
|
||||
/// soon as the item is known, *if* that is strictly before the on-deck
|
||||
/// lookup finishes. Backends that return both together never invoke it, and
|
||||
/// neither does a null item. Callers must therefore treat it as an optional
|
||||
/// early paint and still handle the returned record.
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(
|
||||
String id, {
|
||||
void Function(MediaItem item)? onItemReady,
|
||||
});
|
||||
|
||||
/// Direct children of [parentId] — episodes of a season, seasons of a
|
||||
/// show, tracks of an album, items of a collection.
|
||||
|
||||
@@ -207,6 +207,12 @@ class _SeasonEpisodePager {
|
||||
}
|
||||
}
|
||||
|
||||
/// Identifies the TV reveal gate's opacity wrapper. The detail tree builds
|
||||
/// other [AnimatedOpacity] widgets (the scroll-linked app-bar scrim is 0 at
|
||||
/// rest), so tests must target this one specifically.
|
||||
@visibleForTesting
|
||||
const tvDetailRevealGateKey = ValueKey<String>('tvDetailRevealGate');
|
||||
|
||||
class MediaDetailScreen extends StatefulWidget {
|
||||
final MediaItem metadata;
|
||||
final bool isOffline;
|
||||
@@ -837,6 +843,9 @@ class _MediaDetailScreenState extends State<MediaDetailScreen>
|
||||
child: IgnorePointer(
|
||||
ignoring: !revealed,
|
||||
child: AnimatedOpacity(
|
||||
// Keyed so tests can assert this specific gate rather than
|
||||
// whichever AnimatedOpacity happens to be lowest on screen.
|
||||
key: tvDetailRevealGateKey,
|
||||
opacity: revealed ? 1 : 0,
|
||||
duration: const Duration(milliseconds: 160),
|
||||
curve: Curves.easeOutCubic,
|
||||
@@ -1280,16 +1289,20 @@ class _MediaDetailScreenState extends State<MediaDetailScreen>
|
||||
return;
|
||||
}
|
||||
|
||||
final result = await client.fetchItemWithOnDeck(_metadata.id);
|
||||
final metadata = result.item;
|
||||
final onDeckEpisode = result.onDeckEpisode;
|
||||
|
||||
if (!mounted) return;
|
||||
|
||||
// Preserve serverId from original metadata
|
||||
// Normalises a freshly fetched item against the row we navigated from
|
||||
// (which owns serverId/library) and paints it. Called once from
|
||||
// [onItemReady] on backends that learn the item before on-deck, and once
|
||||
// from the settled result.
|
||||
//
|
||||
// [onDeckSettled] separates "on-deck not looked up yet" from "on-deck
|
||||
// looked up and there is none". Only the settled call may write it, so
|
||||
// the early paint leaves whatever is on screen alone — this method runs
|
||||
// again after playback, and clearing there would blank the play button
|
||||
// for the length of the on-deck round trip — while a reload that finds
|
||||
// the series finished still clears it.
|
||||
MediaItem publish(MediaItem source, {MediaItem? onDeckEpisode, bool onDeckSettled = false}) {
|
||||
final serverId = _metadata.serverId;
|
||||
final serverName = _metadata.serverName;
|
||||
final source = metadata ?? _metadata;
|
||||
final base = _withFallbackLibrary(
|
||||
source.copyWith(serverId: serverId ?? source.serverId, serverName: serverName ?? source.serverName),
|
||||
_metadata,
|
||||
@@ -1306,9 +1319,29 @@ class _MediaDetailScreenState extends State<MediaDetailScreen>
|
||||
|
||||
setState(() {
|
||||
_fullMetadata = base;
|
||||
_onDeckEpisode = onDeckWithServerId;
|
||||
if (onDeckSettled) _onDeckEpisode = onDeckWithServerId;
|
||||
_isLoadingMetadata = false;
|
||||
});
|
||||
return base;
|
||||
}
|
||||
|
||||
// Jellyfin needs a second round trip for on-deck, which the screen does
|
||||
// not need in order to paint. Publishing the item as soon as it lands
|
||||
// takes that round trip off the critical path (#1784).
|
||||
//
|
||||
// Seasons/extras deliberately still start after the whole lookup
|
||||
// settles: starting them at the early paint measured *worse*, because
|
||||
// they contend with the on-deck request rather than overlapping it.
|
||||
final result = await client.fetchItemWithOnDeck(
|
||||
_metadata.id,
|
||||
onItemReady: (item) {
|
||||
if (mounted) publish(item);
|
||||
},
|
||||
);
|
||||
final metadata = result.item;
|
||||
|
||||
if (!mounted) return;
|
||||
final base = publish(metadata ?? _metadata, onDeckEpisode: result.onDeckEpisode, onDeckSettled: true);
|
||||
|
||||
if (base.isShow) {
|
||||
unawaited(_loadSeasons());
|
||||
|
||||
@@ -613,17 +613,22 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals {
|
||||
/// OnDeck semantics: returns the resume episode when one exists, or S1E1
|
||||
/// when the user hasn't started. Movies and other kinds short-circuit.
|
||||
///
|
||||
/// Deliberately still chained rather than fired in parallel off a caller
|
||||
/// kind hint: measured against a remote server that saved nothing, because
|
||||
/// the two requests contend rather than overlap (`/Shows/NextUp` went from
|
||||
/// 380ms alone to 1395ms beside the detail fetch), and it would cost a
|
||||
/// wasted request on every movie whose hint was absent or wrong.
|
||||
/// The chain stays sequential — firing both together measured no better,
|
||||
/// because the requests contend rather than overlap (`/Shows/NextUp` went
|
||||
/// from 380ms alone to 1395ms beside the detail fetch) and a movie would pay
|
||||
/// for a request it can never use. Instead the item is handed to
|
||||
/// [onItemReady] the moment it lands, so the caller can paint without
|
||||
/// waiting for the on-deck round trip (#1784).
|
||||
@override
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id) async {
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(
|
||||
String id, {
|
||||
void Function(MediaItem item)? onItemReady,
|
||||
}) async {
|
||||
final item = await fetchItem(id);
|
||||
if (item == null || item.kind != MediaKind.show) {
|
||||
return (item: item, onDeckEpisode: null);
|
||||
}
|
||||
onItemReady?.call(item);
|
||||
final nextUp = await _safeFetchItemsArray('/Shows/NextUp', {
|
||||
'seriesId': id,
|
||||
'userId': connection.userId,
|
||||
|
||||
@@ -3769,10 +3769,14 @@ class PlexClient
|
||||
}
|
||||
|
||||
/// Full item with on-deck episode from a single `/library/metadata/{id}`
|
||||
/// round-trip. Implements [MediaServerClient.fetchItemWithOnDeck];
|
||||
/// Jellyfin has no analogous endpoint and returns onDeck=null there.
|
||||
/// round-trip. Implements [MediaServerClient.fetchItemWithOnDeck]. Both
|
||||
/// halves arrive together, so there is no window in which the item is known
|
||||
/// and on-deck is not — `onItemReady` is intentionally never invoked.
|
||||
@override
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id) async {
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(
|
||||
String id, {
|
||||
void Function(MediaItem item)? onItemReady,
|
||||
}) async {
|
||||
try {
|
||||
final result = await getMetadataWithImagesAndOnDeck(id, shouldFallback: _shouldFallbackPlexItemLookup);
|
||||
final itemDto = result['metadata'] as PlexMetadataDto?;
|
||||
|
||||
@@ -315,9 +315,7 @@ void main() {
|
||||
},
|
||||
pendingPlayableDescendants: descendantsCompleter.future,
|
||||
);
|
||||
final manager = MultiServerManager()..debugRegisterClientForTesting(client);
|
||||
final provider = testMultiServerProvider(manager);
|
||||
addTearDown(provider.dispose);
|
||||
final provider = testMultiServer(clients: [client]).provider;
|
||||
|
||||
await tester.pumpWidget(
|
||||
TranslationProvider(
|
||||
@@ -342,6 +340,97 @@ void main() {
|
||||
expect(find.text('Specials'), findsNothing);
|
||||
expect(find.text('S1E1'), findsOneWidget);
|
||||
});
|
||||
testWidgets('TV detail reveal still waits for the supplemental sections', (tester) async {
|
||||
// Counterpart to the test above: the early paint does NOT move the TV
|
||||
// reveal. `_isTvDetailReadyToReveal` additionally requires extras, related
|
||||
// hubs, seasons and the first episode page, and those deliberately start
|
||||
// only once the on-deck lookup settles — starting them at the early paint
|
||||
// measured worse, because they contend with it rather than overlap.
|
||||
// Pinned so the phone/desktop win is never restated as an all-platform one.
|
||||
await SettingsService.getInstance();
|
||||
tester.view.physicalSize = const Size(1280, 720);
|
||||
tester.view.devicePixelRatio = 1;
|
||||
addTearDown(tester.view.resetPhysicalSize);
|
||||
addTearDown(tester.view.resetDevicePixelRatio);
|
||||
|
||||
final show = testMediaItem(
|
||||
id: 'show_1',
|
||||
backend: MediaBackend.jellyfin,
|
||||
kind: MediaKind.show,
|
||||
title: 'The Show',
|
||||
serverId: 'server_1',
|
||||
serverName: 'Server',
|
||||
);
|
||||
final season1 = testMediaItem(
|
||||
id: 'season_1',
|
||||
backend: MediaBackend.jellyfin,
|
||||
kind: MediaKind.season,
|
||||
title: 'Season 1',
|
||||
index: 1,
|
||||
parentId: show.id,
|
||||
serverId: show.serverId,
|
||||
serverName: show.serverName,
|
||||
);
|
||||
final episode1 = testMediaItem(
|
||||
id: 'episode_1',
|
||||
backend: MediaBackend.jellyfin,
|
||||
kind: MediaKind.episode,
|
||||
title: 'Episode 1',
|
||||
index: 1,
|
||||
parentIndex: season1.index,
|
||||
parentId: season1.id,
|
||||
grandparentId: show.id,
|
||||
serverId: show.serverId,
|
||||
serverName: show.serverName,
|
||||
);
|
||||
|
||||
final client = _FakeMediaServerClient(
|
||||
show: show,
|
||||
childrenByParent: {
|
||||
show.id: [season1],
|
||||
season1.id: [episode1],
|
||||
},
|
||||
)..onDeckGate = Completer<void>();
|
||||
final provider = testMultiServer(clients: [client]).provider;
|
||||
|
||||
await tester.pumpWidget(
|
||||
TranslationProvider(
|
||||
child: ChangeNotifierProvider<MultiServerProvider>.value(
|
||||
value: provider,
|
||||
child: MaterialApp(
|
||||
theme: monoTheme(dark: true),
|
||||
home: withProfileNavigationScope(
|
||||
child: SizedBox(width: 1280, height: 720, child: MediaDetailScreen(metadata: show)),
|
||||
),
|
||||
),
|
||||
),
|
||||
),
|
||||
);
|
||||
|
||||
for (var i = 0; i < 4; i++) {
|
||||
await tester.pump();
|
||||
}
|
||||
await tester.pump(const Duration(milliseconds: 200));
|
||||
|
||||
// Target the reveal gate specifically: the detail tree also builds a
|
||||
// scroll-linked app-bar scrim whose opacity is 0 at rest, so matching on
|
||||
// AnimatedOpacity by type would pass no matter what the gate does.
|
||||
double revealOpacity() => tester.widget<AnimatedOpacity>(find.byKey(tvDetailRevealGateKey)).opacity;
|
||||
|
||||
// The item was published early and the metadata phase is over...
|
||||
expect(client.earlyPaints, hasLength(1));
|
||||
expect(find.byType(CircularProgressIndicator), findsNothing);
|
||||
// ...yet TV shows nothing but the backdrop, because the reveal gate also
|
||||
// waits on extras, related hubs, seasons and the first episode page — none
|
||||
// of which have started, since they run after the on-deck lookup settles.
|
||||
expect(revealOpacity(), 0, reason: 'the early paint must not be claimed as a TV win');
|
||||
|
||||
// Let the held lookup finish so teardown is not left holding a suspended
|
||||
// future and a client mid-request.
|
||||
client.onDeckGate!.complete();
|
||||
await tester.pump();
|
||||
await tester.pump(const Duration(milliseconds: 100));
|
||||
});
|
||||
|
||||
testWidgets('TV detail summary uses light theme foreground color', (tester) async {
|
||||
await SettingsService.getInstance();
|
||||
@@ -441,9 +530,7 @@ void main() {
|
||||
season2.id: [episode2],
|
||||
},
|
||||
);
|
||||
final manager = MultiServerManager()..debugRegisterClientForTesting(client);
|
||||
final provider = testMultiServerProvider(manager);
|
||||
addTearDown(provider.dispose);
|
||||
final provider = testMultiServer(clients: [client]).provider;
|
||||
|
||||
await tester.pumpWidget(
|
||||
TranslationProvider(
|
||||
@@ -538,9 +625,7 @@ void main() {
|
||||
},
|
||||
childrenPageErrors: {season1.id: Exception('season cache failed')},
|
||||
);
|
||||
final manager = MultiServerManager()..debugRegisterClientForTesting(client);
|
||||
final provider = testMultiServerProvider(manager);
|
||||
addTearDown(provider.dispose);
|
||||
final provider = testMultiServer(clients: [client]).provider;
|
||||
|
||||
await tester.pumpWidget(
|
||||
TranslationProvider(
|
||||
@@ -629,9 +714,7 @@ void main() {
|
||||
},
|
||||
childrenPageFutures: {season2.id: season2Completer.future},
|
||||
);
|
||||
final manager = MultiServerManager()..debugRegisterClientForTesting(client);
|
||||
final provider = testMultiServerProvider(manager);
|
||||
addTearDown(provider.dispose);
|
||||
final provider = testMultiServer(clients: [client]).provider;
|
||||
|
||||
await tester.pumpWidget(
|
||||
TranslationProvider(
|
||||
@@ -839,15 +922,16 @@ void main() {
|
||||
final downloadProvider = DownloadProvider.forTesting(downloadManager: downloadManager, database: db);
|
||||
await downloadProvider.ensureInitialized();
|
||||
|
||||
final manager = MultiServerManager()..debugRegisterClientForTesting(client);
|
||||
final multiServerProvider = testMultiServerProvider(manager);
|
||||
// testMultiServer disposes the manager as well as its provider;
|
||||
// MultiServerProvider does not own the manager, and manager.dispose() is
|
||||
// what closes its status/progress controllers and the registered client.
|
||||
final multiServerProvider = testMultiServer(clients: [client]).provider;
|
||||
final watchStateOverlay = WatchStateStore();
|
||||
|
||||
addTearDown(() async {
|
||||
watchStateOverlay.dispose();
|
||||
downloadProvider.dispose();
|
||||
downloadManager.dispose();
|
||||
multiServerProvider.dispose();
|
||||
await db.close();
|
||||
});
|
||||
|
||||
@@ -912,6 +996,53 @@ void main() {
|
||||
);
|
||||
}
|
||||
|
||||
testWidgets('paints the item before the on-deck lookup settles', (tester) async {
|
||||
// Jellyfin needs a second round trip for on-deck; the phone/desktop
|
||||
// layout must not wait for it. Scoped to non-TV deliberately: on TV the
|
||||
// foreground stays at opacity 0 until `_isTvDetailReadyToReveal` is
|
||||
// satisfied, which this change does not move (see the TV counterpart).
|
||||
final show = buildShow();
|
||||
final season1 = buildSeason(show, 1);
|
||||
MediaItem episode(int number, {required int viewCount}) => testMediaItem(
|
||||
id: 'episode_$number',
|
||||
backend: MediaBackend.jellyfin,
|
||||
kind: MediaKind.episode,
|
||||
title: 'Episode $number',
|
||||
index: number,
|
||||
parentIndex: season1.index,
|
||||
parentId: season1.id,
|
||||
grandparentId: show.id,
|
||||
serverId: show.serverId,
|
||||
serverName: show.serverName,
|
||||
viewCount: viewCount,
|
||||
);
|
||||
|
||||
final client = _FakeMediaServerClient(
|
||||
show: show,
|
||||
childrenByParent: {
|
||||
show.id: [season1],
|
||||
season1.id: [episode(1, viewCount: 1), episode(2, viewCount: 0)],
|
||||
},
|
||||
)..onDeckGate = Completer<void>();
|
||||
|
||||
await pumpPhoneDetail(tester, client, show);
|
||||
|
||||
// On-deck is still in flight, but the item has landed.
|
||||
expect(client.earlyPaints, hasLength(1));
|
||||
expect(find.byType(CircularProgressIndicator), findsNothing, reason: 'painted without waiting for on-deck');
|
||||
|
||||
// Settling with no on-deck must not drop the episode-derived fallback
|
||||
// that `_ensureFallbackOnDeckEpisode` supplies.
|
||||
client.onDeckGate!.complete();
|
||||
for (var i = 0; i < 6; i++) {
|
||||
await tester.pump();
|
||||
}
|
||||
await tester.pump(const Duration(milliseconds: 300));
|
||||
|
||||
expect(find.text('S1E2'), findsOneWidget, reason: 'fallback survives a settled empty on-deck');
|
||||
expect(find.text('S1E1'), findsNothing);
|
||||
});
|
||||
|
||||
testWidgets('shows directors when they are the only additional info', (tester) async {
|
||||
final movie = testMediaItem(
|
||||
id: 'director_only',
|
||||
@@ -1212,6 +1343,17 @@ class _FakeMediaServerClient implements MediaServerClient {
|
||||
final childrenPageCalls = <({String parentId, int? start, int? size})>[];
|
||||
final thumbnailPaths = <String?>[];
|
||||
|
||||
/// On-deck episode returned by the next [fetchItemWithOnDeck]; mutate between
|
||||
/// loads to model the series being finished.
|
||||
MediaItem? onDeckEpisode;
|
||||
|
||||
/// Held open to keep the on-deck half of a load in flight while the item half
|
||||
/// has already been published.
|
||||
Completer<void>? onDeckGate;
|
||||
|
||||
/// Items handed to `onItemReady` — i.e. painted before on-deck settled.
|
||||
final earlyPaints = <MediaItem>[];
|
||||
|
||||
_FakeMediaServerClient({
|
||||
required this.show,
|
||||
required this.childrenByParent,
|
||||
@@ -1233,8 +1375,19 @@ class _FakeMediaServerClient implements MediaServerClient {
|
||||
ServerCapabilities get capabilities => ServerCapabilities.jellyfin;
|
||||
|
||||
@override
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id) async {
|
||||
return (item: show, onDeckEpisode: null);
|
||||
Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(
|
||||
String id, {
|
||||
void Function(MediaItem item)? onItemReady,
|
||||
}) async {
|
||||
// Mirrors the Jellyfin shape: the item is known first, on-deck needs a
|
||||
// second round trip.
|
||||
if (onItemReady != null) {
|
||||
earlyPaints.add(show);
|
||||
onItemReady(show);
|
||||
}
|
||||
final gate = onDeckGate;
|
||||
if (gate != null) await gate.future;
|
||||
return (item: show, onDeckEpisode: onDeckEpisode);
|
||||
}
|
||||
|
||||
@override
|
||||
|
||||
Reference in New Issue
Block a user