Opening explorer #836
Opening explorer #836
Conversation
|
Hi Mauritz opening-explorer.mp4Note that the code and the design are not polished. If I remember correctly, the communication with the server is not done well, and there are some other things not implemented. Some feedback and points to discuss:
|
|
Hi! Thanks for taking the time to share your thoughts and give feedback, I appreciate it more than you know. To address your points:
On a side note, I tried to get each color block in the percentage graph to have widths according to their percentage, but I had a lot of problems with the table changing size depending on the data and sometimes going of screen, and I couldn't really figure out why. Do you remember if you had any similar problems? From what I can see in your video, the opening explorer is only available as a standalone tool, separate from the analysis board. I think that it's a must that it's a part of the analysis board, so you can use it when analyzing games. I think designing the ui is the difficult part. The table itself takes up a lot of space, and you also need to be able to switch between the masters, lichess, and player db, and the different settings for each of them. All while still being available from within the analysis board. One option that comes to mind is to be able to swipe between the move list and the opening explorer. It has the benefit of always being able to see the board, which I think would be a big plus, but it leaves less space for the explorer compared to having a whole screen dedicated to it. Any and all ideas are appreciated, since I feel like it can be quite tricky to get it right. |
|
This one will wait in the queue for the review, until I've dealt with older PRs. Just a couple of quick notes from what I've read here:
|
It causes weird display issues, most likely because it's in a table
checks that the screen loads with data, for all database types
Mauritz8
force-pushed
the
opening-explorer
branch
from
28690da
to
86c294d
Compare
|
I've refactored the cache implementation, it's still exposing a stream though (see my previous comment). I also added a move number cap to try and fetch the wikibooks url, and now it selects the current user as default for the player db. I added a few widget tests as well, that check that the data loads for each db type. I'm not sure what the preferred way to mock a provider is, when it needs different state between test cases. I did a bit of a hack to change the database type between tests, but there's probably a better way. |
| ); | ||
|
|
||
| openingExplorerStreamFuture.then( | ||
| (openingExplorerStream) => openingExplorerStream.listen( |
There was a problem hiding this comment.
Here you want to save the StreamSubscription and cancel it on dispose. It is perhaps easier to do with an async/await.
| import '../../test_app.dart'; | ||
| import '../../test_utils.dart'; | ||
|
|
||
| MockClient client(OpeningDatabase db) => MockClient((request) { |
There was a problem hiding this comment.
Instead of mocking a client per database, you can mock a client that handles every possible urls (masters, lichess, etc..) and override default value in shared preferences to set the default database selected.
This way we are really testing the logic of sending the right request according to settings.
To override preferences you can do sth like:
SharedPreferences.setMockInitialValues({});
final sharedPreferences = await SharedPreferences.getInstance();
...
overrides: [
sharedPreferencesProvider.overrideWithValue(sharedPreferences),
],
...To simplify things I suppose another optional parameter Map<String, Object> defaultPreferences can be added to the buildTestApp function.
There was a problem hiding this comment.
Once this is setup it will be easier to add more tests later, where we can test change of settings through the UI.
lib/src/model/common/http.dart
Outdated
| return response.stream | ||
| .map(utf8.decode) | ||
| .where((e) => e.isNotEmpty && e != '\n') | ||
| .map((e) => jsonDecode(e) as Map<String, dynamic>) |
There was a problem hiding this comment.
Instead of having 3 map I think we can combine them for more efficiency:
return response.stream
.where((e) => e.isNotEmpty && e != '\n')
.map((e) {
final json = jsonUtf8Decoder.convert(e);
return mapper(json);
});
``There was a problem hiding this comment.
I think we have to convert the bytes to a string first in order to do the where, but the other two can be combined:
return response.stream
.map(utf8.decode)
.where((e) => e.isNotEmpty && e != '\n')
.map((e) {
final json = jsonDecode(e) as Map<String, dynamic>;
return mapper(json);
});There was a problem hiding this comment.
Yeah probably. I wonder if the e != '\n' is really necessary btw. I don't remember why I put that in the first place.
There was a problem hiding this comment.
Yes it's needed, as empty lines may be sent to avoid timeouts.
| final request = Request('GET', url); | ||
| if (headers != null) request.headers.addAll(headers); | ||
| final response = await send(request); | ||
| if (response.statusCode > 400) { |
There was a problem hiding this comment.
There is already a _checkResponseSuccess helper that I think we can reuse here.
There was a problem hiding this comment.
That function needs a Response but we have a StreamedResponse, which is not a subtype. It checks the body of the response, which is not available in a StreamedResponse.
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final ctrlProvider = |
There was a problem hiding this comment.
Since this is assigned with the return of ref.watch the name is not good here. What we have is the state of analysisController. So analysisState would be the proper name.
Mauritz8
force-pushed
the
opening-explorer
branch
from
1222001
to
92e1f5d
Compare
|
I've updated everything now. |
|
I must thank you again for your work @Mauritz8 . It is really awesome to see the explorer in the app finally 🎉 I've added only cosmetic changes to your PR, and improved the loading effect (it doesn't blink anymore). |
|
Hey @Mauritz8 had two questions?
|
|
Sorry not necessarily more native. But what I mean is the page that opens first in mobile web is not the wikibooks page. There's a more dedicated page that opens first that has a link to wikibooks page. Basically wondering if that's the page we should first have access to. If you try on mobile web perhaps you'll see what page I mean . It's just a different link I was curious about.
I think the bug is if you go far enough I , like 8 moves sometimes, clicking the opening doesn't do anything in app. But putting the exact same opening sequence in mobile web, the link is clickable. If you need a recording lmk |
|
@ijm8710 I think this will eventually be its own tool, instead of only linking to the opening explorer. Currently in Beta, it's Lichess's way of offering their own crowdsourced explanations to openings and their variations without relying on the (in my opinion) often vague wiki books explanations. |
|
Right now I believe it uses wikibooks as a baseline and are slowly filling it in with original content |
|
I'm not asking for it to be more native. I'm saying currently it's just a web redirect but the page provided is different from mobile web. Shouldn't we use the same one? |
|
I see, it should just link to the lichess.org page instead. I like that, and it seems like an easy thing to change. A recording of the bug would be appreciated. |
I see, sorry for the misunderstanding |
Awesome glad you agree and here's video of other. When there's a pause it doesn't open link trim.FA61F938-A557-4850-865F-55F87986D0D9.MOV |
|
This recording doesn't show what you described with opening master games. Do you have an example where it works for a certain position one time, but not after opening the explorer from a master game? If not, it seems like it's an issue with certain openings at all times, such as the moscow variation you showed. |
|
Perhaps that's a better way of describing the issue. I probably interpreted it wrong. Either way Moscow does work in mobile and has a wiki book so would assume it should work. |
|
Actually yes I have an example. If you go to the 8th master game from top (the ones that show up without making a first move) and from analysis you jump right to c5?! It will not work. But if you move one move back it will work. But both have the exact same name I believe |
|
Wikibooks has a different page for each sequence of moves, so even if two positions have the same opening name in lichess they will have different wikibook pages. In this case, after c5 there is no wikibook page. |
|
Ah okay that makes sense! Apologies for the long road to show you that. On mobile web, since it opens that OTHER beta page I referenced rather than wikibooks directly, it opens up "Indian Defence with e6, Nf3" If you were to use this OTHER beta page, would it be able to do it even in the app too off of c5?! As well or is there some extra magic they do to basically go off move name rather than the entire sequence if there's an innacurzcy to the sequence line opening? |
|
Linking to the lichess.org page, like the website does, should be quite easy. Then there will be a link even after the move c5. |
|
I created a pr now #989 |
Only supports the master database for now. Thought it'd be a good idea to create a pr before I go any further, in order to get your thoughts.
demo:
Screencast.from.2024-07-06.22.35.13.webm
close #456
close #872