mobile icon indicating copy to clipboard operation
mobile copied to clipboard

feat: add tournament screen

Open tom-anders opened this issue 9 months ago • 3 comments

With this PR, we'd have very basic support for tournaments. However, the game screen is still missing some features, like berserking, displaying rank, etc. That will be a follow-up PR. Same goes for the "podium" screen for finished tournaments.

I did not add widget tests yet, wanted to get feedback on the layout/design first.

tournament_screen.webm

tom-anders avatar Mar 25 '25 18:03 tom-anders

The tournament screen is great! Love it. I think it would be useful to also have the players' recent results (at least the last one) to know if one needs to win one or two games to get the win-streak buff. In an end tournament rush, it can help to make strategic decisions.

teefoox avatar Mar 25 '25 22:03 teefoox

Nice work on the UI! Will review asap.

veloce avatar Mar 28 '25 08:03 veloce

Thanks for the review! I added two widgets tests now, let me know if you think there's anything else that should be tested

tom-anders avatar Mar 29 '25 23:03 tom-anders

Just need to change the mocking of data in tests and it's good!

Maybe I overlooked your comment about this, or you forgot to submit - What needs to be changed?

It would be nice to add some other tests to verify we cannot join the tournament if conditions (tournament conditions or not logged in) are not met.

Will do 👍

Once the logic is implemented, we'd want to test the redirect to a game. But that's for another PR.

The redirect is already implemented actually, but not via the socket message, as we could miss the message while we're still in the game. Instead, we're watching the gameId here: https://github.com/tom-anders/lichess-mobile/blob/tournamentScreen/lib/src/view/tournament/tournament_screen.dart#L47

So we could (and should) already test it I think

tom-anders avatar Apr 01 '25 10:04 tom-anders

Maybe I overlooked your comment about this, or you forgot to submit - What needs to be changed?

Oops. Looks like this comment got lost somehow... made another one: https://github.com/lichess-org/mobile/pull/1583#discussion_r2022706754

veloce avatar Apr 01 '25 11:04 veloce

Once the logic is implemented, we'd want to test the redirect to a game. But that's for another PR.

The redirect is already implemented actually, but not via the socket message, as we could miss the message while we're still in the game. Instead, we're watching the gameId here: https://github.com/tom-anders/lichess-mobile/blob/tournamentScreen/lib/src/view/tournament/tournament_screen.dart#L47

So we could (and should) already test it I think

Hmm, I tried, but I keep getting

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
A Timer is still pending even after the widget tree was disposed.
'package:flutter_test/src/binding.dart':
Failed assertion: line 1606 pos 12: '!timersPending'

in the test when the GameScreen is opened. The stack trace looks related to the websocket client, but I'm not familiar enough with the game screen to figure out why that happens

tom-anders avatar Apr 01 '25 17:04 tom-anders

Once the logic is implemented, we'd want to test the redirect to a game. But that's for another PR.

The redirect is already implemented actually, but not via the socket message, as we could miss the message while we're still in the game. Instead, we're watching the gameId here: https://github.com/tom-anders/lichess-mobile/blob/tournamentScreen/lib/src/view/tournament/tournament_screen.dart#L47 So we could (and should) already test it I think

Hmm, I tried, but I keep getting

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
A Timer is still pending even after the widget tree was disposed.
'package:flutter_test/src/binding.dart':
Failed assertion: line 1606 pos 12: '!timersPending'

in the test when the GameScreen is opened. The stack trace looks related to the websocket client, but I'm not familiar enough with the game screen to figure out why that happens

I pushed a commit with that test if you want to try to fix it. Otherwise, we can also skip that test for now until we figure out how to do it

tom-anders avatar Apr 01 '25 17:04 tom-anders

I pushed a commit with that test if you want to try to fix it. Otherwise, we can also skip that test for now until we figure out how to do it

The reason why timers were still pending is that the game controller onDispose callback was not called. I am not sure exactly what is happening on riverpod side, but that happens if the tests don't give the controller the time to properly load. Here this future was never resolved, so the test failed.

So we cannot just expect GameScreen, we need to load the game.

veloce avatar Apr 02 '25 09:04 veloce

https://github.com/user-attachments/assets/6a361f49-867a-4df3-8c7a-a1b988536d33

Problem with scrolling on iOS. This is due to the featured game clocks.

veloce avatar Apr 02 '25 10:04 veloce

Thanks, will take another look!

While I'm at it, I'll also refactor the testing code to be end to-end as discussed

tom-anders avatar Apr 02 '25 11:04 tom-anders

Couple of notes:

* the whole screen should probably be refreshable (pull-to-refresh)

The server already sends a reload event when something notable changes, so I don't think manual refreshing would have any benefit

* the featured game doesn't seem to move to the next one

Ah, it's because currently we only allow one socket at a time: https://github.com/lichess-org/mobile/blob/05d57ced0faf9bf09ac591b64ad4d1a68db89f9a/lib/src/network/socket.dart#L535

But here, we need the tournament socket to stay active. If I delete that linked code, moving to the next game works (but it's probably not the solution we want...?)

tom-anders avatar Apr 02 '25 17:04 tom-anders

The server already sends a reload event when something notable changes, so I don't think manual refreshing would have any benefit

Then it needs to register app lifecycle onPause/onResume callbacks to force a reload on resume

But here, we need the tournament socket to stay active. If I delete that linked code, moving to the next game works (but it's probably not the solution we want...?)

indeed it is by design we don't want more than one socket connection at a time; how does it work on the website? I don't think the tournament page opens 2 sockets. IIRC the old mobile app also do not open 2 sockets at the same time and still has the featured game.

veloce avatar Apr 02 '25 20:04 veloce

The server already sends a reload event when something notable changes, so I don't think manual refreshing would have any benefit

Then it needs to register app lifecycle onPause/onResume callbacks to force a reload on resume

Done :+1:

But here, we need the tournament socket to stay active. If I delete that linked code, moving to the next game works (but it's probably not the solution we want...?)

indeed it is by design we don't want more than one socket connection at a time; how does it work on the website? I don't think the tournament page opens 2 sockets. IIRC the old mobile app also do not open 2 sockets at the same time and still has the featured game.

Ah, I checked with Firefox devtools, the website sends a startWatching to the server, so we need to do the same. I guess that means we can't reuse the TvController anymore, but that's fine

tom-anders avatar Apr 02 '25 21:04 tom-anders

Alright, we're using the tournament websocket now for displaying the featured game, now everything works even when there's a new featured game.

I also made the board look more like in broadcasts:

Screenshot_1743714662

tom-anders avatar Apr 03 '25 21:04 tom-anders

Sorry it messed the PR commit history...

veloce avatar Apr 04 '25 14:04 veloce

Will merge soon I think, still some testing and a couple of UI changes.

veloce avatar Apr 04 '25 14:04 veloce