Fix/Opening Explorer not recognize right move book off FEN#3084
Conversation
… doesnt match 100
veloce
left a comment
veloce
left a comment
There was a problem hiding this comment.
Thanks for tackling this.
I need tests before accepting the PR (Widget tests). That cover the bug we want to fix, but also the other cases (updating opening, etc).
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
We already have some code that is supposed to deal with that:
mobile/lib/src/model/analysis/analysis_controller.dart
Lines 273 to 299 in c7b3d26
Probably this code is not working completely, since we have an issue mentioned in this PR.
But instead of adding a new method that loops over the nodes again, I'd rather fix the existing code.
There was a problem hiding this comment.
Root cause
In build(), the onVisitNode callback accumulated the path variable only when options.initialMoveCursor != null. In the common case where no cursor is set, path stayed UciPath.empty throughout the loop. As a result, _fetchOpening(branch.position.fen, path) was called with an empty path for every node, storing all openings at the root instead of at the correct node path. Additionally, the opening == null guard meant that for archived games which have a game-level opening set from server metadata per-node opening fetches were skipped entirely. Both issues caused _nodeOpeningAt to find nothing when navigating, leaving currentBranchOpening null.
What was changed
lib/src/model/analysis/analysis_controller.dart:
Added a separate UciPath mainlinePath = UciPath.empty variable that accumulates independently for every mainline node visited, regardless of initialMoveCursor. Replaced the broken if (isMainline && opening == null && branch.position.ply <= 10) block with if (isMainline), which always accumulates mainlinePath and fetches the opening using mainlinePath (the correct path) for positions up to ply 30.
The ply limit is aligned from <= 10 to <= 30, consistent with the existing check already present in _setPath.
Added an else branch in _updateOpening so that when an async opening fetch completes for a node that is an ancestor of the current position, currentBranchOpening is updated in state immediately.
Tests
test/model/analysis/fake_opening_service.dart:
Extended FakeOpeningService to accept a configurable openings map (keyed on EPD the first four fields of a FEN) so tests can return specific openings for specific positions.
test/view/analysis/analysis_opening_test.dart (new file, 3 tests):
- Opening is set when navigating to a mainline position: verifies that after loading a PGN analysis, navigating to a mainline position correctly sets currentBranchOpening.
- Ancestor opening is used when current node has no direct opening: verifies the core bug fix: when the current position has no opening, _nodeOpeningAt walks up the path and returns the nearest ancestor's opening, which is only possible if openings were stored at the correct node paths during build().
- Opening updates correctly when navigating forward and backward: verifies that currentBranchOpening reflects the right opening at each position as the user navigates through the move tree.
https://github.com/user-attachments/assets/7bf27393-f6d3-483d-85e9-83f572caa0a5
The bug from production.
https://github.com/user-attachments/assets/1c9b3f23-ef58-45e1-ab42-75f53348d234
The fix from local development environment.
…not-recognize-right-move-book-off-FEN-when-order-doesnt-match-100
2 participants
Issue
Previously, when opening a completed game and jumping directly to a move that is past the known opening theory the opening name would not be displayed in the Opening Explorer. The name only appeared correctly when navigating move by move from the start.
Root cause
The opening name for each position is fetched and stored on the corresponding node in the game tree. When navigating step by step, each intermediate node gets its opening fetched along the way. When jumping directly to a later move, the intermediate nodes have no opening data yet, so _nodeOpeningAt, which walks the path to find the last known opening, returns null.
Fix (analysis_controller.dart)
Two changes were made:
Improvement: local development support (opening_explorer_repository.dart)
Additional change
Added a _explorerUri helper that uses Uri.http instead of Uri.https when kLichessOpeningExplorerHost points to a local address (localhost, 10.x, 192.168.x). This allows developers to run the Opening Explorer locally without SSL errors, matching the existing behavior of lichessUri used for the main lichess host. This was one of my struggles to set-up a good development environment.
Fixes #1480