Bookmark games

[julien4215]

Close #594
Continuation of #868

• Context menu
• Game history
• Archived game screen
• Game screen

bookmark.webm

[veloce]

Thanks for taking this over.

In order to land this feature we also need the filter to display bookmarked games in the game list.
Endpoint: https://lichess.org/api#tag/Games/operation/apiGamesUser

This filter is not yet available server side, so you should create an issue on Lila about it.

[julien4215]

Currently there are two ways to bookmark a game from game history screen. Do we want to keep both ?

[veloce]

Currently there are two ways to bookmark a game from game history screen. Do we want to keep both ?

yes

the background of the slidable needs to be dark on dark theme

[julien4215]

I went with creating a new provider that store the changes made by the user. I am not really happy with this solution because this provider needs the keepalive flag even if should actually be disposed when the game isn't stored in any other provider.

[veloce]

This provider looks dubious, I think it is not necessary and this state can be store elsewhere (probably a transient widget state is enough).

[veloce]

For this, prefer a StatefulWidget. This state should stay local.

[veloce]

Again: make it a StatefulWidget and don't use a provider for this.

[veloce]

StatefulWidget, not provider

[veloce]

I went with creating a new provider that store the changes made by the user. I am not really happy with this solution because this provider needs the keepalive flag even if should actually be disposed when the game isn't stored in any other provider.

Indeed this provider needs to be removed as I said in the comments.

[julien4215]

Well there is a reason why I created this provider. If you use only StatefulWidget how are you going to sync the bookmark value on different screen ?

All the game providers are cached so we really need to store the state locally as if no new requests are made, the game providers cannot know about a new bookmark value.

[veloce]

Well there is a reason why I created this provider. If you use only StatefulWidget how are you going to sync the bookmark value on different screen ?

All the game providers are cached so we really to store the state locally as if no new requests, the game providers cannot know about a new bookmark value.

We don't want to store the state of whether a game is bookmarked globally in the app. The source of truth here is the server, not the app, so each time we want to know if a game is bookmarked, the server must provide this information.

If you want to store temporarily a "bookmarked" state while waiting for a server response, then a local state in a StatefulWidget is the way to go.

[julien4215]

The games HTTP requests are cached and so is the bookmark value. If we don't want to store the state of whether a game is bookmarked then we would have to remove the HTTP requests cache for games.

[julien4215]

As I guess we don't want to remove the game cache, the only other option I see is too add a new endpoint just for bookmarks, this way we don't have to reask the server all game information with it.

[veloce]

The games HTTP requests are cached and so is the bookmark value. If we don't want to store the state of whether a game is bookmarked then we would have to remove the HTTP requests cache for games.

You mean cached by the provider right? Then you must invalidate the provider to refetch the data when changing bookmark status.

[julien4215]

So on each screen where you can bookmark a game I have to invalidate something like 4 or 5 providers that store games with the bookmark field ?

[veloce]

Not sure to understand the problem. Can you please list here these providers that store games?

[julien4215]

There are currently only the first two providers that are needed because I didn't add the bookmark option on the home tab recent games list, I did it only for the user game history and game/archived screen but there will probably be even more screens where we will need the bookmark option (analysis screen, study screen, broadcast screen ...).

• archiveGameProvider
• userGameHistoryProvider
• myRecentGamesProvider
• userRecentGamesProvider

So when a game is bookmarked I will need to call ref.invalidate on each of these providers. Maybe by adding some logic, I can avoid invalidating some providers. I thought that was a better option to keep in a provider the bookmarked changes made by the user but I can go with invalidating providers if you think there is no problem.

[veloce]

I see. So the various caches I added were not a problem so far. But now they are since we're modifying the bookmark state from the app.

Now you need to rebase to the last main commit, where I removed the caches. So now all the listed providers above won't cause a problem anymore.

Only in the UserGameHistory provider you need to update the state to reflect the new bookmark state. Since that one is a Notifier, it is easy to add a new bookmark method to it to update the corresponding game.

[julien4215]

I will probably still need to invalidate some providers as the providers on top of the route stack will not get their providers disposed

[veloce]

I will probably still need to invalidate some providers as the providers on top of the route stack will not get their providers disposed

I suppose you meant below in the stack? Like if you bookmark a game in the game screen and then pop the screen to go back to the game list screen? In that case indeed the game won't be bookmarked in that list.

We probably need a centralised service to bookmark games. This service would be aware of all the notifiers that implement the bookmarkGame method, and would be the single entry point to bookmark a game. We don't want to have to "remember" to invalidate providers, so we need a single entry point that update all needed things for us.

[julien4215]

I suppose you meant below in the stack? Like if you bookmark a game in the game screen and then pop the screen to go back to the game list screen? In that case indeed the game won't be bookmarked in that list.

Yes sorry this is what I meant.

The bookmark provider that I had created was an attempt to centralize bookmark values but I agree it had some flaws. If we decide to create a centralized service for bookmarks I think the removal of cache for games provider is not useful anymore so maybe that commit can be reverted ?

[ijm8710]

I suppose you meant below in the stack? Like if you bookmark a game in the game screen and then pop the screen to go back to the game list screen? In that case indeed the game won't be bookmarked in that list.

@veloce hopefully it's okay to briefly hijack. Would this be similar to if you run analysis and then go back to game screen and it doesn't reflect it in game list that this is a game that has had analysis run until you pull down to refresh. And if so is it worth filing a new ticket to improve that (if it's even possible) in the same way?

[veloce]

I suppose you meant below in the stack? Like if you bookmark a game in the game screen and then pop the screen to go back to the game list screen? In that case indeed the game won't be bookmarked in that list.

@veloce hopefully it's okay to briefly hijack. Would this be similar to if you run analysis and then go back to game screen and it doesn't reflect it in game list that this is a game that has had analysis run until you pull down to refresh. And if so is it worth filing a new ticket to improve that (if it's even possible) in the same way?

Yes it's ok to create an issue for that.

[veloce]

The bookmark provider that I had created was an attempt to centralize bookmark values but I agree it had some flaws. If we decide to create a centralized service for bookmarks I think the removal of cache for games provider is not useful anymore so maybe that commit can be reverted ?

No we should still avoid caching here, that will simplify things.

[julien4215]

We probably need a centralised service to bookmark games. This service would be aware of all the notifiers that implement the bookmarkGame method, and would be the single entry point to bookmark a game. We don't want to have to "remember" to invalidate providers, so we need a single entry point that update all needed things for us.

Do you have an idea on how this should be implemented ?