Introduce preset messages into the app

[Happy0]

The new app is looking great :D. Well done :).

Note: I won't be offended if you don't want this. For me this was mostly a learning exercise in flutter and dart and I've learned a lot in the process and could probably do something more useful for the app :P.

Preset messages allow players (including anonymous non-logged in users) to send preset messages such as 'good luck' and 'have fun' on game start and 'gg' on game end.

This functionality mirrors that currently on the website.

Limitations:

• No internationalisation support (this is also the case on the website and would be a separate piece of work requiring updates to both the phone app and website to make the messages appear in both users' languages.)
• Whether a preset has already been set can't be synced up with the website / other devices (I don't think this is a big deal :P.)
• State forgotten when closing the app and returning to game

Message Bubble Bug Fix

Prior to this PR, the message bubble functionality didn't take into account that anonymous players can send these preset messages and the bubble 'else' case makes it look like it was the phone player who sent these even if it was the other player. I added a change to expose the "colour" field (c) in the websocket message.

State Persistence

The state is written to sqlite so it is retained when games are opened again after the app is closed (or the game is moved away from - I don't think this is possible at the moment but it perhaps could be in the future to navigate between multiple correspondence games if it isn't possible already.)

If you think this is heavy handed let me know and I could explore just retaining it for the lifetime of the game widget. Being a flutter noob, I'd have to work out how to pipe that state together as the chat widget state is built from scratch each time the chat button is clicked and creates a 'router change event'.

I thought putting it in SQLlite was probably fine because the chat widget does similar for new / read messages, etc.

Screenshots

Scenarios Tested

• Chat bubbles still work as expected after adding in the 'look at the anonymous player's colour if they're not logged in' change.
• Clicking the preset buttons makes them disappear.
• Pressing 2 buttons on either game start or end makes all the bubbles disappear.
• The chat buttons move from the 'start' buttons to 'end' buttons when the game has ended in less than 4 moves
• The 'start' chat buttons disappear after 4 moves have been played.
• The 'end' buttons appear if the user clicks that chat after the game ends
• The end buttons appear if the user is on the chat while the game ends
• The start buttons disappear if the user clicks the chat after the 4th move
• The start buttons disappear if the user is on the chat when the 4th move has been played

[veloce]

Thanks for the initiative and for this very detailed PR description. I'll review asap.

As for the SQL migration, I haven't checked the code but if it's a new table it should be pretty straightforward to test. You can build and use a version of the app prior to your PR and then build again with the version change of your code.

Now I'm not sure to understand why sql is needed for this feature? If I understand correctly, once you selected a preset, it automatically adds it for each game, that's right?
If you only need to remember what preset is actually selected, SQL is overkill indeed. SQL should be used only for large collections or when the data is critically important for the app logic. The rest should use shared preferences.

[Happy0]

@veloce

Apologies for the length of this message... There's no rush for you to respond 😅 :

As for the SQL migration, I haven't checked the code but if it's a new table it should be pretty straightforward to test. You can build and use a version of the app prior to your PR and then build again with the version change of your code.

Ah, thanks, that makes sense :)

Now I'm not sure to understand why sql is needed for this feature? If I understand correctly, once you selected a preset, it automatically adds it for each game, that's right?

it's not a configuration setting that automatically sends/adds preset messages per game. It's a bunch of buttons that appear in the chat at the start / end of games to automatically send 'good luck!' or 'good game', etc. The buttons change at the start / end of games.

Here's a gif of what it looks like at the moment. The app is closed in the middle and reopened to show that it remembers what button you already clicked for the game (due to the database entry.)

(The gif sits on my home screen for a while while the app is redeployed to the debugger)

(Note: annoyingly, there's a bug here where it doesn't display the end messages for some reason after resignation that I'd have to look into 😅 .)

This approach also has the side effect that when the chat widget is closed / reopened from the game screen it remembers what you'd clicked.

Without the database it forgets the state each time the 'chat' route is closed because the 'chat preset' provider is no longer being listened to by anything and is removed and forgets its state:

No state remembered)

Each time the chat route is opened the 'chatcontroller' and the 'chat presets controller' are started from scratch.

I do think the database is overkill just to remember those button states between app closes / game switches in correspondence when 99.999% of the time the user isn't going to return to chat when the game ends and the 'start' messages disappear after a few moves. There's a halfway solution where the state is remembered when you close the chat widget and open it again while still in the game - I'm just finding it challenging to work out the best way to achieve that with flutter / riverpod with the 'chat' route being independent from the 'game view' and its provider states being forgotten on close. I'm a bit of a UI noob in general...

One approach I tried was using cacheFor (https://riverpod.dev/docs/essentials/auto_dispose#example-keeping-state-alive-for-a-specific-amount-of-time) to cache the 'chat presets controller' state around for X minutes after its closed. I don't like this because it probably has a reasonably high memory footprint because the 'game controller provider' will end up in the provider tree too (to get the game state from to work out the preset group) so the more games you open the worse the memory footprint gets and it just seems hacky in general.

You have a lot more flutter/riverpod experience than me - have you ran into a scenario where a route is 'downstream' of another route and you want that route to remember some state each time its opened as long as the 'parent' is still open further up the stack? (I'm not sure if that makes any sense at all 😅 .)

But maybe I'm overthinking this and that 2nd gif is actually good enough? The website currently forgets the state when you refresh the page afterall 😅 .

[veloce]

So the SQL is definitely overkill yes. I'd rather have the buttons to always be displayed, no matter you clicked them already or not.
I think having the state tied to a game is fine for a first implementation: as long as the game is loaded you know which button was clicked and if you change game you'd lose the state.

If you want to remember the state across games I'd use a dedicated memory store that'd be reset when the app is restarted. But I don't think such feature is necessary, let's keep things simple for now.

[Happy0]

@veloce Makes sense to me :). I've made those changes and pushed so the PR is ready for review now whenever you get the chance.

This was my approach for keeping the state tied to a game as the chat is opened or closed: https://github.com/lichess-org/mobile/pull/825/files#diff-4921abb96f5f54e0628bbb3f807f679ebe816c62f7e06499fadbed2bbe9b6de6R428 (the provider won't be removed by flutter when the chat is closed because there's still a listener.) Let me know if you have any better ideas around that or want me to rethink it.

[veloce]

This was my approach for keeping the state tied to a game as the chat is opened or closed: https://github.com/lichess-org/mobile/pull/825/files#diff-4921abb96f5f54e0628bbb3f807f679ebe816c62f7e06499fadbed2bbe9b6de6R428 (the provider won't be removed by flutter when the chat is closed because there's still a listener.) Let me know if you have any better ideas around that or want me to rethink it.

I'm surprised that you have to do that to keep the state. Looks like a workaround, and in theory we should not need to do this (or it indicates that it can be done differently).
The chat state is listened to here:

mobile/lib/src/view/game/game_body.dart

Lines 430 to 432 in c8219b5

	final chatStateAsync = gamePrefs.enableChat == true
	? ref.watch(chatControllerProvider(id))
	: null;

so we can display the unread count chip in the bottom bar. So as I understand it the state is watched all the time the game is shown, which means it should stay available. But maybe I'm missing something?

[veloce]

Ok now I understand why the state is invalidated.

I think the easiest way to fix this is to remove the chatPresetController and put its logic in the chatController. This will simplify things hopefully as this logic belongs to the chat controller, and you won't have to worry about state invalidation anymore.

[veloce]

Not sure to understand why you need a private local var here.

[veloce]

Here I don't think ref.listen is the best way to handle this.

Since we want to act upon the game state, ref.watch(gameControllerProvider(id)) feels more like what we want.
We don't want to rebuild the chat state each time the game state changes, so you can also use selectAsync to filter rebuild by returning only PresetMessageGroup when they change.

[Happy0]

Hey @veloce . Yeah, the same thought actually occurred to me (moving the functionality to 'chat controller' which retains its state in order to display the 'count chip.') I've went ahead and moved it, got rid of the 'chat presets controller' and pushed :)

I used selectAsync like you suggested and what I've pushed works as far as I can tell (tried with opening and closing the chat, sending chat messages both when it's closed / opened and with the presets test scenarios in the PR description...) However, I wanted to flag that I seen some very weird behaviour with selectAsync that I don't really understand and it worries me a bit that I'm missing some undefined behaviour / race condition (or that there might even be a flutter bug.) If you move final presetMessageGroup = ... above _getMessages() when the chat is opened, the chat spinner spins infinitely and the getMessages() future never completes - I added some print statements and it seems to enter _socketClient.stream.firstWhere but it never receives full from the server (It seems it receive crowd topic message then nothing.) I don't understand why the order would matter here but the behaviour is consistent when moving the order. It doesn't seem to be some subtlety around rebuilding because this happens the first time it's built.

I also wanted to note that I added the alreadySynced flag because without it when the provider build is reran when the game status changes in a way that changes the preset buttons (satisfying the selectAsync), you would get an infinite spinner because _socketClient.stream.firstWhere happens on top of an already opened socket (cached by _socketClient = ref.read(socketPoolProvider).open(GameController.gameSocketUri(id)); so it never receives the 'full' message which has already been received on the socket in the past. I think this behaviour hadn't been seen prior because the chat controller provider never had to be rebuilt. The alternative to this would probably be to close the underlying socket by adding _socketClient.close() at the top of build and reopen it but I think it's more desirable to keep the socket open and use the 'sync' flag (haven't tried this.) Let me know if you have any thoughts...

[veloce]

@Happy0 I think you've hit a tricky case but you couldn't know it before so it's not your fault ;)

There is indeed an issue with the initialisation of the providers and the fact that the socket API is designed as such the gameFull message is sent only once upon connection.

The original mistake was made by me actually. Because of that websocket API, the chatController should not connect to the game websocket at all. Because if we do that, we'll always have to worry about the order of connections and the fact one consumer may need this gameFull message.

To fix this situation I think the chatController should only listen to the global WS event stream:

mobile/lib/src/model/common/socket.dart

Line 52 in 2f1f9c8

final socketGlobalStream = _globalStreamController.stream;

Also the SocketPool should has a send method to send messages to whatever connected WS. This send method should be restricted to some type of WS messages only (like send in chat).

This is more or less how it's done on the website actually.

So the chat controller must be refactored. It should have a new method to set all messages at once (that the game controller will call when receiving the gameFull event).

The socket pool must be updated too, I'll take care of this and ping you when it's ready.

[Happy0]

Thanks a lot @veloce :). I'll make those changes soon

[veloce]

@Happy0 actually you can already use send a message to whatever socket client is connected:

ref.read(socketPoolProvider).currentClient.send('talk', 'hello');

No need to modify SocketPool contrary to what I said above.

[Happy0]

Hullo @veloce_ :D. Sorry about the delay - I got a bit ill / busy for a bit.

I'm currently trying to implement your suggested changes here.

The bit I'm having difficult with is giving the ChatController a method that accepts the initialMessages (received on the 'gameFull' event.) I defined a new method on ChatController called setInitialMessages which should update the state with the initial messages and call it from GameController when the full message arrives. However, when this method is reached the state object doesn't appear to be initialised. It looks like the call to setInitialMessages is interleaved with the ChatController build method and completes before build completes (so the ChatController's state is never initialised, so whenData doesn't have any state to update inside the setInitialMessages method.)

This isn't super surprising as both gameController and chatController have a lot of async operations going on so they're being interleaved with each other. I'm just not sure what to do about it and whether I'm missing some flutter / riverpod knowledge that might lead me in the right direction 😅 .

I made this temporary branch here to show you the changes I've made: Happy0/lichess-mobile@preset_messages_v2...Happy0:lichess-mobile:preset_messages_v5

I was wondering if you had any advice?

• I know I could probably use ref.watch (with select to limit the number of times it fires) in the GameController build method to wait for the chat controller becoming initialised fully but that seems hacky and also I don't think GameController be happy with being rebuilt (for the same reasons we started making these changes to ChatController - it would freeze while waiting for a full message again.)

• I could perhaps just add the chat messages to the state of GameController's GameState and allow ChatController to subscribe to the updates on it and update its own state. GameController would listen for new messages on the socket and update the chat state. The game view should use select with watch to avoid rebuilding the widget on chat message updates. ChatController would remain and would deal with tracking unread messages, the chat preset states and how to send a chat message on the websocket. It would also take a reference to the chat message array in its own state.

I think this second option maybe makes sense to me... It also gets rid of the circular dependency between ChatController and GameController where GameController would be calling setInitialMessages on ChatController but ChatController is also subscribing to GameController to look for the game status changing. What do you reckon? 😅

[veloce]

You're right, it's not a good idea to set the messages in the chat controller like I suggested. Because it violates a riverpod rule: https://riverpod.dev/docs/essentials/do_dont#dont-perform-side-effects-during-the-initialization-of-a-provider

I have to think about it. Letting the game controller handle the chat messages would work of course, but I'd rather avoid it if possible. I wish the socket api was designed such as not having this gameFull; perhaps it is not a big deal after all, and we can let the app make 2 force connection requests?