Broadcast player results screen #1253

julien4215 · 2024-12-11T11:03:39Z

broadcast_results.webm

ijm8710 · 2024-12-11T17:00:20Z

Hey Julian, 2 requests.

mobile web green/reds wins and losses (the 0 and 1s) so that material results stand out. I like this and we should probably do same.
on mobile web, above the results list, they have the player bio (ranks across variants, player age, fide id that links to their fide player page,etc). I'd like to also have this too shown outright either above or below the results list you're adding.

This is awesome tho!

veloce · 2024-12-12T13:55:49Z

lib/src/view/broadcast/broadcast_boards_tab.dart

-              child: CircularProgressIndicator.adaptive(),
-            ),
-          ),
+        _ => const BroadcastPreview.loading()


I removed this on purpose. It was displaying a flash of shimmer boards in FIDE WC where only one board is displayed.

So it's better to not have it.

I think the board shimmer looks better than the circular indicator.

In the issue #1255 you can see that the flash is a black screen. This is because the circular indicator is not in the Scaffold widget but at the top.

Also I believe this loading is never reached because we are already listen higher in the tree to the round state (broadcast_round_screen).

If there is a way to know in advance the number of boards, then we can use the shimmer. Otherwise I think it is ugly to see a flash of 10 shimmer boards where in fact only one is show eventually.

Having only one board in a round is pretty rare. In the majority of cases I think the board shimmer looks nicer than the circular indicator.

Unfortunately the world championship is the most watch broadcast... even if it is not the majority it will be seen much more times than other tournaments with more boards.

I definitely agree with Julien here. The shimmer looks and feels much better. Also, the wch match is over, and the next one will take place in about two years.

It does unless there is only one or two board and you see a flash of shimmer if the request completes very quickly.

veloce · 2024-12-12T13:58:06Z

lib/src/view/broadcast/broadcast_game_screen.dart

@@ -79,11 +81,15 @@ class _BroadcastGameScreenState extends ConsumerState<BroadcastGameScreen>
  Widget build(BuildContext context) {
    final broadcastGameState = ref
        .watch(broadcastGameControllerProvider(widget.roundId, widget.gameId));
+    final broadcastRoundState =
+        ref.watch(broadcastRoundControllerProvider(widget.roundId));


Here we don't want to rebuild the whole screen if any of the other game of the round is updated, so you need to use .selectAsync.

But see this other comment, we should have all the state in a single controller.

I used select instead because selectAsync returns a Future which makes it unpractical to use inside a widget.

In order to use selectAsync you need to create a dedicated provider to retrieve the title. This is the way to do it with riverpod.

But what is the advantage of creating a new provider instead of using select like this ?

final title = ref.watch( broadcastRoundControllerProvider(widget.roundId) .select((round) => round.value?.round.name), );

It's mostly a problem of typing. Here you're mixing the async value which represent an asynchronous value which may be not here yet, with a nullable value that you derives from another asynchronous value.

By creating another provider that returns an AsyncValue for the title you're making it clear of what you expect. It is ok to declare a new provider for that. It is like a cached computed value, but async. Doing that is recommended in the riverpod doc (but I don't remember where).

Note also that using .value? directly you're not handling the error, it will be rethrown in the widget build method making it crash.

Ok thanks for the detailed explanation. I'll make a title provider.

I believe I have already done this in previous PR but I'll make sure to create a provider in the future

veloce · 2024-12-12T14:01:50Z

lib/src/view/broadcast/broadcast_player_results_screen.dart

+                    onTap: () {
+                      pushPlatformRoute(
+                        context,
+                        builder: (context) => BroadcastGameScreen(


Since we can now open a screen from another context, this should be refactored:

mobile/lib/src/view/broadcast/broadcast_game_screen.dart

Lines 372 to 378 in 9a6a400

// TODO

// we'll probably want to remove this and get the game state from a single controller

// this won't work with deep links for instance

final game = ref.watch(

broadcastRoundControllerProvider(roundId)

.select((round) => round.requireValue.games[gameId]!),

);

I don't understand why this would not work with deep link. The two controllers are independent, they just listen to the same route because there is only one for broadcast.

If you can load a game by itself you don't want to load the whole round, right? In a tournament where you have 100 board you'd load 99 boards (and listen to their updates) for nothing.

I agree with that but I have done it this way because in the API there are no endpoints to get the player information for a specific board.

Maybe this refactoring can be done in an other branch once there is an API endpoint for that ?

Yes let's do it in another PR.

This code is also problematic because you assume you assume the round controller is loaded: select((round) => round.requireValue.games[gameId]!)

I'm not sure it is a good practice to mix select and requireValue in riverpod. So far we know it is loaded because the only way to access the game screen was through the round screen.

Now that you can access the game screen from the player result screen directly, can you be sure the round screen will be loaded? If you can access a game from another round it won't be the case. This is the main reason why it must be refactored.

I removed the requireValue. Do you still want to do the refactoring in another branch ?

After talking with Thibault, it's decided that it is ok to load the full round to display a single game. So no refactoring after all.

But we need to ensure the round controller is loaded before the game controller in the game screen. (So we can do a requireValue)

veloce · 2024-12-12T14:03:34Z

lib/src/view/broadcast/broadcast_players_tab.dart

@@ -118,7 +121,7 @@ class _PlayersListState extends ConsumerState<PlayersList> {
  @override
  Widget build(BuildContext context) {
    final double eloWidth = max(MediaQuery.sizeOf(context).width * 0.2, 100);
-    final double scoreWidth = max(MediaQuery.sizeOf(context).width * 0.15, 70);
+    final double scoreWidth = max(MediaQuery.sizeOf(context).width * 0.15, 90);


Why this change? Have you noticed a case where it was too small?

This is because I changed the score from 'x/y' to 'x / y'.

veloce · 2024-12-12T14:04:57Z

lib/src/view/broadcast/broadcast_players_tab.dart

              color: Theme.of(context).platform == TargetPlatform.iOS
-                  ? index.isEven
+                  ? (index - 1).isEven


I think it was fine like that, you should leave it as is. I want the color appear in this order on iOS.

This was to have the same color order as in the opening explorer

controller

julien4215 · 2024-12-13T17:08:40Z

I think I have addressed all of your comments

veloce · 2024-12-15T11:37:37Z

lib/src/view/broadcast/broadcast_game_screen.dart

+    final title = widget.title ??
+        (switch (ref.watch(broadcastGameScreenTitleProvider(widget.roundId))) {
+          AsyncData(value: final title) => title,
+          _ => 'Broadcast Game',


I'd rather have a SizedBox.shrink() here than a default title.

julien4215 force-pushed the broadcast-player-screen branch 4 times, most recently from 03d785c to e948d04 Compare December 11, 2024 15:02

julien4215 force-pushed the broadcast-player-screen branch from e948d04 to de1b7ba Compare December 11, 2024 23:40

Add player results screen

1e8253b

julien4215 force-pushed the broadcast-player-screen branch from de1b7ba to 1e8253b Compare December 12, 2024 00:31

julien4215 marked this pull request as ready for review December 12, 2024 11:09

julien4215 added 3 commits December 12, 2024 13:02

Use slugs instead of round url to share game pgn

0af48bd

Rename federation ID to names variable

56075dc

Fix conflicts with upstream

0b99e6b

veloce reviewed Dec 12, 2024

View reviewed changes

julien4215 added 9 commits December 12, 2024 15:26

Restore list color order for broadcast players

cf3a3de

Use circular indicator instead of board shimmer

1b2ed99

Listen only to the name of the round

28a5e39

Fix color of BOT title

e845066

Make players results accessible from broadcast game screen

c2f59b6

Fix null value error on broadcast game screen

387c057

Check if player information is loaded

1cb097b

Fix conflicts with upstream

4052bb5

Use providers with selectAsync to get values from broadcast round

4e9ad45

controller

Improve score sorting on broadcast tab players

7c4098c

veloce reviewed Dec 15, 2024

View reviewed changes

julien4215 added 4 commits December 15, 2024 15:43

Remove useless Column widget

b623b3f

Don't show a default title if title is not yet loaded

10cb336

Rename some broadcast variables

b410cbe

Fix conflicts with upstream

c5d8fdc

julien4215 force-pushed the broadcast-player-screen branch from 3edcebe to c5d8fdc Compare December 16, 2024 09:40

julien4215 added 3 commits December 16, 2024 15:42

Merge upstream to use the new Dart formatter

795391e

Merge main branch from upstream

c264d98

Use the new flag asset

5ad9246

veloce merged commit 5ad9246 into lichess-org:main Dec 18, 2024
1 check passed

julien4215 deleted the broadcast-player-screen branch December 19, 2024 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broadcast player results screen #1253

Broadcast player results screen #1253

julien4215 commented Dec 11, 2024 •

edited

Loading

ijm8710 commented Dec 11, 2024 •

edited

Loading

veloce Dec 12, 2024

julien4215 Dec 12, 2024 •

edited

Loading

veloce Dec 12, 2024

julien4215 Dec 12, 2024

veloce Dec 12, 2024

HaonRekcef Dec 12, 2024

veloce Dec 13, 2024

veloce Dec 12, 2024

veloce Dec 12, 2024

julien4215 Dec 12, 2024 •

edited

Loading

veloce Dec 13, 2024

julien4215 Dec 13, 2024 •

edited

Loading

veloce Dec 13, 2024

veloce Dec 13, 2024

julien4215 Dec 13, 2024

veloce Dec 12, 2024

julien4215 Dec 12, 2024

veloce Dec 12, 2024

julien4215 Dec 12, 2024 •

edited

Loading

veloce Dec 13, 2024

veloce Dec 13, 2024

julien4215 Dec 13, 2024 •

edited

Loading

veloce Dec 13, 2024

veloce Dec 12, 2024

julien4215 Dec 12, 2024

veloce Dec 12, 2024

julien4215 Dec 12, 2024

julien4215 commented Dec 13, 2024

veloce Dec 15, 2024

julien4215 Dec 15, 2024

	// TODO
	// we'll probably want to remove this and get the game state from a single controller
	// this won't work with deep links for instance
	final game = ref.watch(
	broadcastRoundControllerProvider(roundId)
	.select((round) => round.requireValue.games[gameId]!),
	);

Broadcast player results screen #1253

Broadcast player results screen #1253

Conversation

julien4215 commented Dec 11, 2024 • edited Loading

ijm8710 commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

julien4215 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien4215 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien4215 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien4215 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien4215 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien4215 commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien4215 commented Dec 11, 2024 •

edited

Loading

ijm8710 commented Dec 11, 2024 •

edited

Loading

julien4215 Dec 12, 2024 •

edited

Loading

julien4215 Dec 12, 2024 •

edited

Loading

julien4215 Dec 13, 2024 •

edited

Loading

julien4215 Dec 12, 2024 •

edited

Loading

julien4215 Dec 13, 2024 •

edited

Loading