feat: add study list screen

[tom-anders]

@veloce Since studies are such a huge feature, I'd suggest we split it up and start with this PR that only will add the study list screen. This also makes it easier for others to take over development from me if necessary. After review, we can hide the button until the study feature is fully implemented.

[ijm8710]

I like the big icons they show on web.
Makes it easier to quickly differentiate between studies

[tom-anders]

study_list.webm

[veloce]

@veloce Since studies are such a huge feature, I'd suggest we split it up and start with this PR that only will add the study list screen. This also makes it easier for others to take over development from me if necessary. After review, we can hide the button until the study feature is fully implemented.

@tom-anders yeah great initiative! we should split this one as much as possible; I prefer to review small PRs :)

[tom-anders]

@veloce Since studies are such a huge feature, I'd suggest we split it up and start with this PR that only will add the study list screen. This also makes it easier for others to take over development from me if necessary. After review, we can hide the button until the study feature is fully implemented.

@tom-anders yeah great initiative! we should split this one as much as possible; I prefer to review small PRs :)

Perfect 👍 I'll further split up this PR as well.

[ijm8710]

Thanks for adding the icons :)

2 more

A few times in video the names become ... prob because too long. But on mobile web I don't see it. So I'm assuming (because names are prob capped anyway) should be able to fit them always without truncation

I do slightly prefer how mobile web the chosen filter sort is actively shown (ie "hot" studies) rather than having to open the filter to see the specified choice

[tom-anders]

Thanks for adding the icons :)

2 more

A few times in video the names become ... prob because too long. But on mobile web I don't see it. So I'm assuming (because names are prob capped anyway) should be able to fit them always without truncation

I guess that depends on the on the screen size, you probably always have cases where names are to long. But we can tweak the layout and font size too make it a bit less likely.

I do slightly prefer how mobile web the chosen filter sort is actively shown (ie "hot" studies) rather than having to open the filter to see the specified choice

Good point - I don't want the buttons to be visible always, but adjusting the title of the top bar would probably be the best solution. Something like "Study - All - Popular"

[tom-anders]

(converted back to a draft, as I'll split some code into smaller independent PRs

[tom-anders]

@veloce I'm currently thinking about how we should implement the analysis board for studies. We probably want to reuse as much of the existing AnalysisController, but it currently expects a PGN string being passed in its build method. I guess we can either...

1. Convert the study analysis object returned by the API into a PGN and then use that to call analysisControllerProvider(pgn, options), but it feels weird, because the controller will then parse the PGN again and turn it into its own Root object representation
2. Refactor analysisControllerProvider, so that you can either pass a PGN, or an existing Root object (that we convert the study json into). I'm not really sure what's the most idiomatic way to do this though, do we simply use two nullable parameters and require that one of them has to be non-null?

Or maybe there's another approach I'm missing? Let me know what you think when you find the time :)

[veloce]

I think the StudyController should be completely separate from the analysis controller and should have its own logic.

Over reusing a controller is not a good idea. I plan to write a dedicated controller for the opening explorer for instance, because almost all the logic of the analysis controller is not needed in the explorer.

Once we have several controller that share some logic, we can think about refactoring, using mixins probably. This kind of refactoring is not that easy to do, so we'd better way for things to stabilise.

[veloce]

That being said, if you fancy that kind of refactoring and want to create a Notififier mixin to handle basic operations like navigating in the tree, fell free to try. But I'm not sure it is easy, because each screen has different needs.

Refactor analysisControllerProvider, so that you can either pass a PGN, or an existing Root object (that we convert the study json into). I'm not really sure what's the most idiomatic way to do this though, do we simply use two nullable parameters and require that one of them has to be non-null?

Is there a direct way to convert the study JSON to a Node? We need a tree structure in study for sure, but study doesn't have the same need as analysis controller. A couple of notes:

• analysis controller takes a PGN because it aims to support lichess analysis and standalone analysis (PGN input)
• performance wise, you don't want to supply large objects to a riverpod Notifier family instance (such a an IList, or a large object), because provider will compute the hash each time the widget is built, and the hash of a large object is expensive to compute. Riverpod's doc warns agains this also (should only provide primitives to a provider family)

For this reason, the study notifier should take only the study ID as parameter, and be asynchronous.

[tom-anders]

Thanks for the quick and helpful reply!

I think the StudyController should be completely separate from the analysis controller and should have its own logic.

Over reusing a controller is not a good idea. I plan to write a dedicated controller for the opening explorer for instance, because almost all the logic of the analysis controller is not needed in the explorer.

Once we have several controller that share some logic, we can think about refactoring, using mixins probably. This kind of refactoring is not that easy to do, so we'd better way for things to stabilise.

Ah, the opening explorer was actually the code that gave me this idea of reusing the controller in the first place. Ok, then I agree that we should write a separate study controller, and (maybe) do the refactoring in the future.

Is there a direct way to convert the study JSON to a Node?

Haven't tried yet, but the layout looked similar enough. But maybe it's better to use a dedicated StudyNode class instead, we'll see.

• performance wise, you don't want to supply large objects to a riverpod Notifier family instance (such a an IList, or a large object), because provider will compute the hash each time the widget is built, and the hash of a large object is expensive to compute. Riverpod's doc warns agains this also (should only provide primitives to a provider family)

For this reason, the study notifier should take only the study ID as parameter, and be asynchronous.

Good hint, will keep this in mind 👍

[julien4215]

For the broadcast feature, I made a different screen for the analysis board but I reused the analysis controller because I thought the logic was almost the same. I also went for this because the api returns a pgn https://lichess.org/api#tag/Studies/operation/studyChapterPgn so I don't understand why you said tom-anders that you need to pass a Root object to the analysis controller. By the way I think the study and broadcast feature might have some code in common because the broadcast is implemented as a study on the backend side.

[tom-anders]

For the broadcast feature, I made a different screen for the analysis board but I reused the analysis controller because I thought the logic was almost the same. I also went for this because the api returns a pgn https://lichess.org/api#tag/Studies/operation/studyChapterPgn so I don't understand why you said tom-anders that you need to pass a Root object to the analysis controller.

Oh wow, I must have completely missed that API endpoint when looking through the docs. Currently I'm using lichess.org/study/<id>/<chapter> (run curl -X GET 'https://lichess.org/study/U3guoz56/byuyr1dH' -H "Accept: application/json" | jq --color-output | less for an example), which returns a tree-like structure instead of a PGN. That's why I was talking about nodes etc.

[tom-anders]

Marking this as "Ready to Review" again, as I don't think I can split this up further

[tom-anders]

I rebased against main to fix conflicts with the build_runner PR, also adjusted the formatting of study chapters/members a bit, to avoid the names getting cut off.

Here's an updated video:

study_list.webm

[ijm8710]

Excited for this Tom. The one thing I find that's goofy is the open circle bullets for each chapter. I know that's how it is on mobile web but as Veloce says they don't always have to mirror.

The one thing I'd ask is do they serve any point. If those circles get filled in or checked off when a study is completed then they do make some sense. If they don't currently, is that even possible to do even if web doesn't since it adds decent utility? If not, do you think there's a better formatting marker to use instead?

I know that's two decently long paragraphs for such a minor formatting marker and no clue why they look so goofy to me lol

[ijm8710]

Also, you mentioned splitting this pr and only starting with study list screen itself. I assume the favorite studies and other options of the like are part of a later phase, is that correct? Totally cool and more than happy to have a slightly limited alpha version at first, just curious if that's part of what you were implying earlier?

[veloce]

Oh, that's not actually a freeze, sorry for not mentioning that, it's just a dummy study screen I added here: https://github.com/lichess-org/mobile/pull/990/files#diff-d6c82c9cf2716bd768ab5fc1abfc30ce85567b52d36230ebdf68f2c616388d56

The intention was to make #1111 (which adds the "real" study screen) independent from the study list screen PR.

I know it's an empty study screen. What I meant by freeze is that the animation starts then stops abruptly at about 25% after it started, it stays blocked during a short time then resume. That's not normal, it should be smooth and continuous all the time.

[tom-anders]

Oh, that's not actually a freeze, sorry for not mentioning that, it's just a dummy study screen I added here: https://github.com/lichess-org/mobile/pull/990/files#diff-d6c82c9cf2716bd768ab5fc1abfc30ce85567b52d36230ebdf68f2c616388d56
The intention was to make #1111 (which adds the "real" study screen) independent from the study list screen PR.

I know it's an empty study screen. What I meant by freeze is that the animation starts then stops abruptly at about 25% after it started, it stays blocked during a short time then resume. That's not normal, it should be smooth and continuous all the time.

Ah, got it, I can reproduce as well. But I still think it's related to the empty screen - If I take #1111 and rebase it onto this branch (Pass -Xtheirs to git rebase), then the freeze is gone

[veloce]

The search bar is sticky so it should be part of the page header, it will look much better. On Android it is very easy, on iOS we have to workaround a little. You can see how it's done in the opening explorer screen.

[veloce]

Why is pumpAndSettle needed here? it is usually needed when animations need to finish, but I prefer put a comment each time we use pump() or pumpOrSettle to explain what are we waiting for, to help future readers.

[tom-anders]

ah this one is needed, I added a comment

[tom-anders]

(although pump seems to also be sufficient)

[veloce]

Also explain why there is a pumpAndSettle at the end.

Would be good to track the requests made and add a check to confirm that the requests match the expected.

[tom-anders]

Ah, this pumpAndSettle() was needed because

WidgetsBinding.instance.addPostFrameCallback((_) {
   setState(() {
      requestedNextPage = false;
   });
});

causes a crash otherwise. But, thanks to the helpful flutter error message, we can add an if (mounted) check to fix this instead.

[tom-anders]

Would be good to track the requests made and add a check to confirm that the requests match the expected.

Added helper variables to track the requests, not sure if this is the most elegant solution though

[veloce]

The search bar is sticky so it should be part of the page header, it will look much better. On Android it is very easy, on iOS we have to workaround a little. You can see how it's done in the opening explorer screen.

I must add that another simpler solution is to make the search bar non sticky. This is perhaps even better.

[tom-anders]

The search bar is sticky so it should be part of the page header, it will look much better. On Android it is very easy, on iOS we have to workaround a little. You can see how it's done in the opening explorer screen.

I must add that another simpler solution is to make the search bar non sticky. This is perhaps even better.

Done, I wrapped the column with a SingleChildScrollView such that the search bar is scrolled as well. This broke the ScrollListener in the ListView though, so I had to move it up to _Body

[veloce]

I tested it on a low end device to check if embedding a ListView inside a SingleChildScrollView would not cause performance issues.

Turn out that the answer is yes... I suppose the ListView has to be the primary scrollable to be efficient.

A simple way to deal with it I think:

itemBuilder: (context, index) {
  if (index == 0) {
    return SearchBar();
  } else {
      final study = studies.studies[index - 1];
      ....
  }

[tom-anders]

I tested it on a low end device to check if embedding a ListView inside a SingleChildScrollView would not cause performance issues.

Turn out that the answer is yes... I suppose the ListView has to be the primary scrollable to be efficient.

A simple way to deal with it I think:

itemBuilder: (context, index) {
  if (index == 0) {
    return SearchBar();
  } else {
      final study = studies.studies[index - 1];
      ....
  }

Ah yes, that seems more elegant anyway. Done.

[tom-anders]

This one (and also "Sort by") do not exist in the Web UI and need will mobile-specific translations at some point in the future