Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1043 add castling method option v2 #1347

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions lib/src/model/settings/board_preferences.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import 'package:lichess_mobile/l10n/l10n.dart';
import 'package:lichess_mobile/src/model/settings/preferences_storage.dart';
import 'package:lichess_mobile/src/styles/styles.dart';
import 'package:lichess_mobile/src/utils/color_palette.dart';
import 'package:lichess_mobile/src/utils/l10n_context.dart';
import 'package:riverpod_annotation/riverpod_annotation.dart';

part 'board_preferences.freezed.dart';
Expand Down Expand Up @@ -43,6 +44,10 @@ class BoardPreferences extends _$BoardPreferences with PreferencesStorage<BoardP
await save(state.copyWith(pieceShiftMethod: pieceShiftMethod));
}

Future<void> setCastlingMethod(CastlingMethod castlingMethod) {
return save(state.copyWith(castlingMethod: castlingMethod));
}

Future<void> toggleHapticFeedback() {
return save(state.copyWith(hapticFeedback: !state.hapticFeedback));
}
Expand Down Expand Up @@ -125,6 +130,7 @@ class BoardPrefs with _$BoardPrefs implements Serializable {
required ClockPosition clockPosition,
@JsonKey(defaultValue: PieceShiftMethod.either, unknownEnumValue: PieceShiftMethod.either)
required PieceShiftMethod pieceShiftMethod,
required CastlingMethod castlingMethod,

/// Whether to enable shape drawings on the board for games and puzzles.
@JsonKey(defaultValue: true) required bool enableShapeDrawings,
Expand All @@ -150,6 +156,7 @@ class BoardPrefs with _$BoardPrefs implements Serializable {
materialDifferenceFormat: MaterialDifferenceFormat.materialDifference,
clockPosition: ClockPosition.right,
pieceShiftMethod: PieceShiftMethod.either,
castlingMethod: CastlingMethod.either,
enableShapeDrawings: true,
magnifyDraggedPiece: true,
dragTargetKind: DragTargetKind.circle,
Expand Down Expand Up @@ -347,6 +354,19 @@ enum ClockPosition {
};
}

enum CastlingMethod {
kingOverRook,
kingTwoSquares,
either;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not support either because it is not supported on website.

The default is kingOverRook

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default is now either since we'll support all three castling methods which is correctly set on line 159.


String castlingMethodl10n(BuildContext context, CastlingMethod castlingMethod) =>
switch (castlingMethod) {
CastlingMethod.kingOverRook => context.l10n.preferencesCastleByMovingOntoTheRook,
CastlingMethod.kingTwoSquares => context.l10n.preferencesCastleByMovingTwoSquares,
CastlingMethod.either => context.l10n.preferencesBothClicksAndDrag,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using preferencesBothClicksAndDrag here? If the string gets changed in the future from "Either" to "Either click or drag", then we wouldn't want the castling method string to change to that as well. I'm unsure when new strings should be created or if maybe we should rename preferencesBothClicksAndDrag to something like preferencesEither

Copy link
Contributor Author

@Jimima Jimima Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point, not sure how to correctly handle this as there is no string currently for this. Maybe I just hard code an english string and add a comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just hardcode it for now and leave a // TODO translate

};
}

String dragTargetKindLabel(DragTargetKind kind) => switch (kind) {
DragTargetKind.circle => 'Circle',
DragTargetKind.square => 'Square',
Expand Down
6 changes: 4 additions & 2 deletions lib/src/view/analysis/analysis_board.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:lichess_mobile/src/model/common/chess.dart';
import 'package:lichess_mobile/src/model/common/eval.dart';
import 'package:lichess_mobile/src/model/engine/evaluation_service.dart';
import 'package:lichess_mobile/src/model/settings/board_preferences.dart';
import 'package:lichess_mobile/src/widgets/board.dart';
import 'package:lichess_mobile/src/widgets/pgn.dart';

class AnalysisBoard extends ConsumerStatefulWidget {
Expand Down Expand Up @@ -66,12 +67,13 @@ class AnalysisBoardState extends ConsumerState<AnalysisBoard> {
)
: ISet();

return Chessboard(
return BoardWidget(
size: widget.boardSize,
boardPrefs: boardPrefs,
fen: analysisState.position.fen,
lastMove: analysisState.lastMove as NormalMove?,
orientation: analysisState.pov,
game: GameData(
gameData: GameData(
playerSide:
analysisState.position.isGameOver
? PlayerSide.none
Expand Down
6 changes: 4 additions & 2 deletions lib/src/view/broadcast/broadcast_game_screen.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import 'package:lichess_mobile/src/view/broadcast/broadcast_player_widget.dart';
import 'package:lichess_mobile/src/view/engine/engine_gauge.dart';
import 'package:lichess_mobile/src/view/engine/engine_lines.dart';
import 'package:lichess_mobile/src/view/opening_explorer/opening_explorer_view.dart';
import 'package:lichess_mobile/src/widgets/board.dart';
import 'package:lichess_mobile/src/widgets/buttons.dart';
import 'package:lichess_mobile/src/widgets/clock.dart';
import 'package:lichess_mobile/src/widgets/pgn.dart';
Expand Down Expand Up @@ -318,12 +319,13 @@ class _BroadcastBoardState extends ConsumerState<_BroadcastBoard> {
)
: ISet();

return Chessboard(
return BoardWidget(
size: widget.boardSize,
boardPrefs: boardPrefs,
fen: broadcastAnalysisState.position.fen,
lastMove: broadcastAnalysisState.lastMove as NormalMove?,
orientation: broadcastAnalysisState.pov,
game: GameData(
gameData: GameData(
playerSide:
broadcastAnalysisState.position.isGameOver
? PlayerSide.none
Expand Down
64 changes: 64 additions & 0 deletions lib/src/view/settings/board_settings_screen.dart
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,30 @@ class _Body extends ConsumerWidget {
}
},
),
SettingsListTile(
settingsLabel: Text(
context.l10n.preferencesCastleByMovingTheKingTwoSquaresOrOntoTheRook,
),
settingsValue: boardPrefs.castlingMethod.name,
showCupertinoTrailingValue: false,
onTap: () {
if (Theme.of(context).platform == TargetPlatform.android) {
showChoicePicker(
context,
choices: CastlingMethod.values,
selectedItem: boardPrefs.castlingMethod,
labelBuilder: (t) => Text(t.castlingMethodl10n(context, t)),
onSelectedItemChanged: (CastlingMethod? value) {
ref
.read(boardPreferencesProvider.notifier)
.setCastlingMethod(value ?? CastlingMethod.either);
},
);
} else {
Navigator.of(context).push(CastlingMethodSettingsScreen.buildRoute(context));
}
},
),
SwitchSettingTile(
title: Text(context.l10n.mobilePrefMagnifyDraggedPiece),
value: boardPrefs.magnifyDraggedPiece,
Expand Down Expand Up @@ -263,6 +287,46 @@ class PieceShiftMethodSettingsScreen extends ConsumerWidget {
}
}

class CastlingMethodSettingsScreen extends ConsumerWidget {
const CastlingMethodSettingsScreen({super.key});

static Route<dynamic> buildRoute(BuildContext context) {
return buildScreenRoute(
context,
screen: const BoardClockPositionScreen(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong screen here.

title: 'Clock position',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong title.

);
}

@override
Widget build(BuildContext context, WidgetRef ref) {
final castlingMethod = ref.watch(
boardPreferencesProvider.select((state) => state.castlingMethod),
);

void onChanged(CastlingMethod? value) {
ref.read(boardPreferencesProvider.notifier).setCastlingMethod(value ?? CastlingMethod.either);
}

return CupertinoPageScaffold(
navigationBar: const CupertinoNavigationBar(),
child: SafeArea(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to have a SafeArea around ListView.

child: ListView(
children: [
ChoicePicker(
notchedTile: true,
choices: CastlingMethod.values,
selectedItem: castlingMethod,
titleBuilder: (t) => Text(t.castlingMethodl10n(context, t)),
onSelectedItemChanged: onChanged,
),
],
),
),
);
}
}

class BoardClockPositionScreen extends ConsumerWidget {
const BoardClockPositionScreen({super.key});

Expand Down
6 changes: 4 additions & 2 deletions lib/src/view/study/study_screen.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import 'package:lichess_mobile/src/view/study/study_gamebook.dart';
import 'package:lichess_mobile/src/view/study/study_settings.dart';
import 'package:lichess_mobile/src/view/study/study_tree_view.dart';
import 'package:lichess_mobile/src/widgets/adaptive_action_sheet.dart';
import 'package:lichess_mobile/src/widgets/board.dart';
import 'package:lichess_mobile/src/widgets/buttons.dart';
import 'package:lichess_mobile/src/widgets/feedback.dart';
import 'package:lichess_mobile/src/widgets/pgn.dart';
Expand Down Expand Up @@ -530,8 +531,9 @@ class _StudyBoardState extends ConsumerState<_StudyBoard> {
final sanMove = currentNode.sanMove;
final annotation = makeAnnotation(studyState.currentNode.nags);

return Chessboard(
return BoardWidget(
size: widget.boardSize,
boardPrefs: boardPrefs,
settings: boardPrefs.toBoardSettings().copyWith(
borderRadius: widget.borderRadius,
boxShadow: widget.borderRadius != null ? boardShadows : const <BoxShadow>[],
Expand All @@ -552,7 +554,7 @@ class _StudyBoardState extends ConsumerState<_StudyBoard> {
? IMap({Move.parse(altCastles[sanMove.move.uci]!)!.to: annotation})
: IMap({sanMove.move.to: annotation})
: null,
game:
gameData:
position != null
? GameData(
playerSide: studyState.playerSide,
Expand Down
146 changes: 146 additions & 0 deletions lib/src/widgets/board.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
import 'package:chessground/chessground.dart';
import 'package:dartchess/dartchess.dart';
import 'package:fast_immutable_collections/fast_immutable_collections.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';
import 'package:lichess_mobile/src/model/settings/board_preferences.dart';

class BoardWidget extends StatelessWidget {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A doc comment indicating the purpose of this widget would be welcome.

BoardWidget({
required this.size,
required this.boardPrefs,
required this.fen,
required this.orientation,
required this.gameData,
this.lastMove,
this.shapes,
required this.settings,
this.boardOverlay,
this.error,
this.annotations,
this.boardKey,
}) : setup = Setup.parseFen(fen);

final double size;
final BoardPrefs boardPrefs;
final String fen;
final Side orientation;
final GameData? gameData;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I don't think GameData should be nullable. This board widget purpose is to wrap interactive boards only.

Perhaps renaming it to InteractiveBoardWidget would make it even clearer.

final Move? lastMove;
final ISet<Shape>? shapes;
final ChessboardSettings settings;
final String? error;
final Widget? boardOverlay;
final IMap<Square, Annotation>? annotations;
final GlobalKey? boardKey;
final Setup setup;

@override
Widget build(BuildContext context) {
final Map<Square, Square> castlingMap = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a static const.

Square.a1: Square.c1,
Square.a8: Square.c8,
Square.h1: Square.g1,
Square.h8: Square.g8,
};

GameData? modifiedGameData;

MapEntry<Square, ISet<Square>> mapper(Square sq, ISet<Square> moves) {
return MapEntry(
sq,
setup.board.kings.squares.contains(sq) && //king move
setup.castlingRights.squares.intersectsWith(
moves, //king can castle
)
? (switch (boardPrefs.castlingMethod) {
CastlingMethod.kingOverRook => moves.removeAll(castlingMap.values),
CastlingMethod.kingTwoSquares => moves.removeAll(castlingMap.keys),
_ => moves,
})
: moves,
);
}

if (gameData != null) {
modifiedGameData = GameData(
playerSide: gameData!.playerSide,
sideToMove: gameData!.sideToMove,
validMoves: gameData!.validMoves.map(mapper),
promotionMove: gameData!.promotionMove,
onMove: gameData!.onMove,
onPromotionSelection: gameData!.onPromotionSelection,
premovable: gameData?.premovable,
isCheck: gameData?.isCheck,
);
}

final board = Chessboard(
key: boardKey,
size: size,
fen: fen,
orientation: orientation,
game: modifiedGameData ?? gameData,
lastMove: lastMove,
shapes: shapes,
settings: settings,
annotations: annotations,
);

if (boardOverlay != null) {
return SizedBox.square(
dimension: size,
child: Stack(
children: [
board,
SizedBox.square(
dimension: size,
child: Center(
child: SizedBox(
width: (size / 8) * 6.6,
height: (size / 8) * 4.6,
child: boardOverlay,
),
),
),
],
),
);
} else if (error != null) {
return SizedBox.square(
dimension: size,
child: Stack(children: [board, _ErrorWidget(errorMessage: error!, boardSize: size)]),
);
}

return board;
}
}

class _ErrorWidget extends StatelessWidget {
const _ErrorWidget({required this.errorMessage, required this.boardSize});
final double boardSize;
final String errorMessage;

@override
Widget build(BuildContext context) {
return SizedBox.square(
dimension: boardSize,
child: Center(
child: Padding(
padding: const EdgeInsets.all(16.0),
child: Container(
decoration: BoxDecoration(
color:
Theme.of(context).platform == TargetPlatform.iOS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the new theme we can remove these distinctions between iOS and android. So use colorScheme.surface on both platforms.

I should have changed it in the new theme PR, but let's do it here.

? CupertinoColors.secondarySystemBackground.resolveFrom(context)
: Theme.of(context).colorScheme.surface,
borderRadius: const BorderRadius.all(Radius.circular(10.0)),
),
child: Padding(padding: const EdgeInsets.all(10.0), child: Text(errorMessage)),
),
),
),
);
}
}
Loading