1043 add castling method option v2 #1347
Conversation
|
Just for my understanding, why the v1 was closed? thanks :) |
|
Sure, I had attempted an implementation that modified dartchess and chessground which maybe was not the correct approach. I didn't have a great deal of time to look at it at that point so I just closed the PRs for hygeine. See here for reference lichess-org/dartchess#44 This change is limited to only the mobile app with no changes elsewhere. Not sure if this is the right approach still, happy to be told if you think there is an alternative way to come at it |
Jimima
force-pushed
the
1043-add-castling-method-option-v2
branch
from
c97a375 to
b2a2c28
Compare
Jimima
force-pushed
the
1043-add-castling-method-option-v2
branch
from
b2a2c28 to
c8348c0
Compare
|
I think this is ready to be looked at @veloce. Not sure what's going on with the tests but maybe an issue unrelated to this PR? |
|
A fix was pushed on the main branch. The tests need to be rerun manually. |
|
@Jimima can you rebase your branch on top of main so the tests pass? thank. |
|
@veloce done! |
There was a problem hiding this comment.
Thanks for this. This is looking good but there are a couple of issues to address on top of the comments I've made:
- for now this settings applies only to online games, but it should also apply to puzzles, analysis, study, etc.
- the logic you added to game_controller.dart must be refactored so it can work everywhere.
- this is the time for a refactoring: we need a global
boardwrapper so we can apply settings to the whole app - a good starting point for this refactoring imo, is the private _BoardWidget which should be extracted to a public widget in another file. It should be modified to take only
BoardPrefsandBoardSettingsOverride(and not directly theChessboardSettingsfrom chessground which should be constructed inside the widget from the prefs and the optional override). - the new castling logic should be tested
|
|
||
| // TODO: l10n | ||
| String get label => switch (this) { | ||
| CastlingMethod.kingOverRook => 'Move king over rook', |
There was a problem hiding this comment.
translation keys already exist: look at app_en.arb: preferencesCastleByMovingOntoTheRook.
| enum CastlingMethod { | ||
| kingOverRook, | ||
| kingTwoSquares, | ||
| either; |
There was a problem hiding this comment.
We should not support either because it is not supported on website.
The default is kingOverRook
| @@ -369,3 +429,11 @@ String pieceShiftMethodl10n(BuildContext context, PieceShiftMethod pieceShiftMet | |||
| PieceShiftMethod.drag => context.l10n.preferencesDragPiece, | |||
| PieceShiftMethod.tapTwoSquares => 'Tap two squares', | |||
| }; | |||
|
|
|||
| String castlingMethodl10n(BuildContext context, CastlingMethod castlingMethod) => | |||
There was a problem hiding this comment.
Duplicated l10n logic; must be defined directly in the enum to be consistent with rest of code.
|
Thanks for the feedback, that all makes sense. I also wonder if we are worried about premoves respecting the preference as right now they do not. I think the main site does not either but I would need to check this. |
|
@veloce I would appreciate another review if possible, I have made a few changes: Created a new BoardWidget component and use this in place of Chessboard where possible Some questions: I wonder if we can support either castling method in the app as it seems like an improvement over the web site. I have left it in for now but can easily remove it if you prefer. Right now I kept the refactoring light and boardPrefs is passed in as a parameter. Should the board widget be fetching the boardPrefs directly from the shared state rather than taking it as a parameter? Is there further refactoring to be done? You mentioned taking only BoardPrefs and BoardSettingsOverride but I was a bit unsure about how to make this work. Any further guidance would be gratefully received 🙂 |
|
Oh yes, also I will add some tests in the meantime |
…ethod-option-v2' into 1043-add-castling-method-option-v2
|
Will review asap.
No here I think we should stick to what the website does. It would be less error prone and easier to maintain. |
|
@veloce The website only gives two castling options, but the second castling option (move king onto rook) still allows you to move the king two spaces to castle. So in reality the two options on the website are
If we're going to stick to what the website does, we should make sure to do it this way or include all three options since the website options aren't explicit enough. Lichess.Website.-.Move.king.onto.rook.mov |
Adds an option to change castling method. Still needs some work to tidy up but the approach is implemented. Note that it only really works for Standard variants as chess960 always uses king over rook. I think this is okay but maybe the UI could make this clearer.