Fix retro evaluation and simplify retro interface#2248
Conversation
| .read(engineEvaluationPreferencesProvider) | ||
| .copyWith(engineSearchTime: kMaxEngineSearchTime) | ||
| : ref.read(engineEvaluationPreferencesProvider); | ||
| ref.read(engineEvaluationPreferencesProvider).copyWith(isEnabled: true, numEvalLines: 1); |
There was a problem hiding this comment.
These settings should not change based on some logic, as this will effectively kill and restart the engine.
There was a problem hiding this comment.
Makes sense, maybe we can add a warning comment about this somewhere in the code?
| feedback: RetroFeedback.findMove, | ||
| mainlinePath: _root.mainlinePath, | ||
| pov: options.initialSide, | ||
| currentNode: RetroCurrentNode.fromNode(_root.view), |
There was a problem hiding this comment.
Avoid using root.view when possible.
There was a problem hiding this comment.
Makes sense, I think the missing conversion from view back to mutable node was the reason why I used views here
| state.copyWith(feedback: RetroFeedback.evalMove, evalRequestedAt: DateTime.now()), | ||
| ); | ||
| requestEval(); | ||
| // Be sure to get enough depth to evaluate the move properly |
There was a problem hiding this comment.
Ahh that’s a much better solution of course, nice!
|
|
||
| @override | ||
| bool get hideBestMoveArrow => analysisState.isSolving; | ||
| bool get hideBestMoveArrow => true; |
There was a problem hiding this comment.
Hmm, don't we want to show the arrows after the solution has been found, in case the user wants to analyse further?
There was a problem hiding this comment.
I think it is better to keep it separate from analysis, and then we don't want to use analysis settings. So I found it simpler to remove the lines and arrows for now.
| await tester.pumpWidget(await makeTestApp(tester, moves: 'e4 e5')); | ||
|
|
||
| // Wait for retro to load | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
Should always use pump instead of pumpOfSettle. The latter is normally only needed when you need to wait for an animation to finish.
| expect(find.text('Try another move for white'), findsOneWidget); | ||
|
|
||
| await playMove(tester, 'd2', 'd4'); | ||
| await tester.pumpAndSettle(const Duration(milliseconds: 500)); |
There was a problem hiding this comment.
This was causing the test to fail.
2 participants
No description provided.