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. |
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.