feat: add board editor #865

tom-anders · 2024-07-14T14:38:22Z

Closes #458

Opening an early draft PR to let people know I'm working on this.

Functionality that should definitely be part of this PR IMO:

Move around pieces freely on the board.
Add a bottom bar to add pieces similar to the old app and lichess.org, selecting a piece in this bar and then tapping a square will add a piece to that square, selecting the bin will remove tapped pieces from the board.
Button to flip the board
Buttons for castling rights and for side to play
Buttons to open position in analysis screen

Functionality that we could add later:

Drag-and-drop pieces from the bottom bar (see above) onto the board - I'm not sure if this needs adjustments in flutter-chessground as well...? Edit: yes, most definitely
Discard pieces by dragging them off-screen
Load a specific opening as starting position
Add button to challenge from built position
Export position as FEN

@veloce Feedback would be welcome, if you find the time :)

tom-anders · 2024-07-14T16:50:15Z

Okay, so after playing around a bit already, it looks to me like most of the board editor logic needs to be added directly to flutter-chessground. Not sure about the "piece-menu" for adding pieces (see above). But if we want to support adding pieces by dragging them from the menu to the board, we probably have to implement this in chessground as well...?

veloce · 2024-07-15T09:28:14Z

Quick feedback: if we have to modify chessground to support this, the changes should be the simplest and the lightest possible.

Like we should only add "generic" (optional) handlers. Something like:

class BoardEditorOptions {
  /// Enable board editor mode
  final bool enable;

  /// Callback to add or remove a piece on this square id.
  final void Function(SquareId squareId)? onTogglePiece;

  final void Function(SquareId squareId)? onRemovePiece;
  ...
}

This is already a significant change because we have to implement the "board editor" logic in the pointer event listener handlers.

Don't know if we can handle drag to board feature like that, but I'd say it is not mandatory to support this so we can figure it out later.

Anyway, changes in chessground shouldn't be more than that.

tom-anders · 2024-07-15T09:43:32Z

Quick feedback: if we have to modify chessground to support this, the changes should be the simplest and the lightest possible.

Like we should only add "generic" (optional) handlers. Something like:
class BoardEditorOptions {
  /// Enable board editor mode
  final bool enable;

  /// Callback to add or remove a piece on this square id.
  final void Function(SquareId squareId)? onTogglePiece;

  final void Function(SquareId squareId)? onRemovePiece;
  ...
}
This is already a significant change because we have to implement the "board editor" logic in the pointer event listener handlers.

Don't know if we can handle drag to board feature like that, but I'd say it is not mandatory to support this so we can figure it out later.

Anyway, changes in chessground shouldn't be more than that.

Thanks for the quick reply! However, after playing around with the code a bit yesterday, I'm not a 100% sure if it's really the best idea to modify the Board class for this:

The Board has a lot of parameters and logic related to them which are not relevant in board editor mode, such as drawing shapes, premoves, highlighting squares, the sideToMove variable, ... I tried to add a editorMode flag (similar to the BoardEditorSettings you described above), and it quickly got messy, with a lof of if (!editorMode) at various places.
Even if we get this to work, we still have to take care of disabling some rules, like castling. Currently, the Board does not modify itself, but rather calls the onMove callback, which then will call something like position.playUnchecked() - There's currently no way to disable castling for this method, so we'd need to modify dartchess as well probably. If we had a dedicated BoardEditor widget that could simply modify the position by itself when a move is played, we wouldn't have these problems.

So maybe adding a separate BoardEditor widget makes sense? Or do you think we'd have too much code duplication in this case?

tom-anders · 2024-07-15T15:49:46Z

@veloce I added a quick proof of concept implementing a basic board editor (minus adding new pieces) by adding a new BoardEditor widget: lichess-org/flutter-chessground#40

veloce · 2024-07-15T22:26:11Z

Yeah totally, a separate BoardEditor is the way to go. Thanks for tackling this!

Now there is still the question of the chessground API. I haven't had much time to think about it, but I still think the chessground package should only expose a lightweight API that would allows package users to build a full-fledged board editor.
Meaning we shouldn't build the full-fledged board editor inside chessground, but rather provide ways to do it through the BoardEditor widget and its callbacks.

The reason behind this is that we don't want to maintain a full-fledged editor in an external package. Because there is no limit in terms of what a board editor can be in terms of feature, the majority of the UI should be implemented in the app and chessground should just take care of what is going on inside the board.

tom-anders · 2024-07-16T05:51:44Z

Thanks for taking a look, I really appreciate the quick feedback!

Yeah I agree with that, the BoardEditor widget in the chessground PR really should be minimal, with the rest (e.g. the piece menu) being implemented by library consumers. I think the draft in the PR is a good start, the only things it's missing are (off the top of my head) addPiece() and removePiece() methods, and also callbacks like clickedSquare and draggedOntoSquare. Probably also a getter that calculates the current FEN.

I'd suggest that I add the missing API described above and clean up and document the code a bit more (maybe reduce some more duplication). Then when I mark it as "Ready To Review", you can take a closer look and we can continue the discussion over at flutter-chessground.

veloce · 2024-07-24T06:51:07Z

@tom-anders I think you can start working on this using a local chessground plugin based on current main, if you don't mind?

I'll publish the 4.0.0 version later when I had the chance to test this.

tom-anders · 2024-07-24T10:57:23Z

@tom-anders I think you can start working on this using a local chessground plugin based on current main, if you don't mind?

I'll publish the 4.0.0 version later when I had the chance to test this.

Sure!

tom-anders · 2024-07-24T15:11:26Z

@veloce There are a lot of API breaks to be fixed when updating to the main branch, so I'll think for now I'll stick with a commit before those breaks. Let me know when the API breaks are fixed in the mobile repo (or even if you just have a branch ready that I can rebase onto).

tom-anders · 2024-07-24T21:17:48Z

Okay, so here's what I got so far. The code is not cleaned up yet, but I think it has all the basic feature we need. Right now, I'm mainly looking for design feedback:

Menu/Button structure
Which colors do we want to use for the piece menu? Same as lichess.org/editor? Or stick with material color palette?
I think we want to at least have the same icons as lichess.org/editor, namely the pointer and the trash icon. I found them here: https://github.com/lichess-org/lila/tree/8dec1e4c8223483b29e95638ceed56365dbd4a66/public/images/icons What's the process for adding new lichess icons to the app?

board_editor.webm

veloce · 2024-07-25T10:17:15Z

@veloce There are a lot of API breaks to be fixed when updating to the main branch, so I'll think for now I'll stick with a commit before those breaks. Let me know when the API breaks are fixed in the mobile repo (or even if you just have a branch ready that I can rebase onto).

I'm ok if you fix the API breaks in this PR. These are only name changes so it should be straightforward to fix.

veloce · 2024-07-25T18:24:43Z

Thanks for your work @tom-anders , it looks great already!

Here's my feedback:

I think we should use the material color palette for the piece menu, trying to find a contrasting color that looks good
I don't understand the bottom left button; is it to reverse the board? then we should use the existing button from the TV screen
I don't think we need the same trash and pointer icons as lichess website.
- I agree they don't look that good right now but we can improve that I think:
- You can try the Cupertino hand icon: https://api.flutter.dev/flutter/cupertino/CupertinoIcons/hand_draw-constant.html
- and delete: https://api.flutter.dev/flutter/cupertino/CupertinoIcons/delete-constant.html
- which kind of look better
Not sure the settings button is the best placed here at the top right:
- it's not a real settings button, it is more the settings of the position; a real settings button would be for settings controlling the editor itself
- I'd try to find another icon more appropriate
- and put it in the bottom bar instead

tom-anders · 2024-07-28T21:57:22Z

Thanks for the feedback, ready for code review now!

* I think we should use the material color palette for the piece menu, trying to find a contrasting color that looks good

Yeah I'm using plain Color.grey, Color.green, Color.blue and Color.red for now. I saw on Discord that someone wanted to help with the app's design, so maybe they can take a look at what colors we should use?

* I don't understand the bottom left button; is it to reverse the board? then we should use the existing button from the TV screen

Yep, wrong icon, fixed now

* I don't think we need the same trash and pointer icons as lichess website.
  
  * I agree they don't look that good right now but we can improve that I think:
  * You can try the Cupertino hand icon: https://api.flutter.dev/flutter/cupertino/CupertinoIcons/hand_draw-constant.html
  * and delete: https://api.flutter.dev/flutter/cupertino/CupertinoIcons/delete-constant.html
  * which kind of look better

Done, I scaled down the icons a bit so that they match the piece size.

* Not sure the settings button is the best placed here at the top right:
  
  * it's not a real settings button, it is more the settings of the position; a real settings button would be for settings controlling the editor itself
  * I'd try to find another icon more appropriate
  * and put it in the bottom bar instead

It uses the "menu" icon now and has been moved to the bottom bar.

veloce · 2024-07-29T10:09:10Z

Yeah I'm using plain Color.grey, Color.green, Color.blue and Color.red for now.

That is not the material color palette I meant, sorry for the confusion. What you want to use is the ColorScheme.

veloce · 2024-07-29T13:24:53Z

lib/src/model/analysis/analysis_controller.dart

I don't understand all these changes in analysis_controller. The board editor isn't supposed to make a difference in analysis screen.

Huh, not sure what went wrong here. There are some changes expected due to the chessground API breaks, but maybe I messed up during the rebase. I'll take a look

Definitely some kind of merge mistake, the changes are only part of the first commit that fixes the chessground API breaks. Sorry about that! Will fix later this evening.

In the meantime, the second commit (feat: add board editor) seems fine, so I think you can already review that one

Fixed, sorry again for the confusion!

veloce · 2024-08-02T15:21:29Z

I just published chessground 4.0.0 to pub.dev @tom-anders .

I think you want to update your PR with that version. There are other breaking changes, sorry for that, but it should not be too hard to fix them.
Here's a summary:

dartchess Square is now an extension type so we gain convenient methods at no cost
chessground now uses all the dartchess types, including Square. There is no more need for chessground_compat.dart
importing dartchess and chessground with prefixes is no more useful and should be avoided.

tom-anders · 2024-08-03T15:19:58Z

Nice work!

Uses chessground 4.0.0 now, fixing all API breaks. Also rebased against main

veloce

Outstanding work, thank you very much.

veloce · 2024-08-05T08:09:18Z

lib/src/view/board_editor/board_editor_screen.dart

+  @override
+  Widget build(BuildContext context, WidgetRef ref) {
+    final pgn = ref.watch(boardEditorControllerProvider).pgn;
+    final orientation = ref.read(boardEditorControllerProvider).orientation;


Should be ref.watch

veloce · 2024-08-05T08:18:25Z

lib/src/model/board_editor/board_editor_controller.dart

+
+  String get fen => _setup.fen;
+
+  String? get pgn {


It should be made clear in a doc comment that only valid position PGN is returned, and it returns null if not valid.

veloce · 2024-08-05T08:19:28Z

lib/src/model/common/uci.dart

@@ -37,7 +37,7 @@ class UciCharPair with _$UciCharPair {
            String.fromCharCode(35 + f),
            String.fromCharCode(
              p != null
-                  ? 35 + 64 + 8 * _promotionRoles.indexOf(p) + squareFile(t)
+                  ? 35 + 64 + 8 * _promotionRoles.indexOf(p) + t.file.value


Since the File extension type implements int, unwrapping .value is not necessary.

veloce · 2024-08-05T08:40:09Z

lib/src/view/board_editor/board_editor_screen.dart

+      ),
+      pointerMode: ref.watch(boardEditorControllerProvider).editorPointerMode,
+      onDiscardedPiece:
+          ref.read(boardEditorControllerProvider.notifier).discardPiece,


One should not use ref.read in build() methods. Also it is better to put it at the beginning of build() so that we know the provider dependencies of a widget.

veloce · 2024-08-05T12:11:38Z

test/view/board_editor/board_editor_test.dart

+}
+
+Future<BoardEditorController> setupBoardEditor(WidgetTester tester) async {
+  final boardEditorController = BoardEditorController();


It is not ideal to construct directly a notifier instance. Testing providers should be done according to Riverpod's doc:
https://riverpod.dev/docs/essentials/testing

Creating/reading providers should always go through a Container.

Done here: 86dd3fb

Thanks, will keep that in mind for the next time

tom-anders · 2024-08-05T12:30:31Z

Thanks again for the quick and valuable feedback! Implementing this was really fun

ijm8710 · 2024-09-06T15:12:54Z

Hey @tom-anders great work on this again.

Would it make sense to have a "clear board" option similar to how web has one? If there was a menu that would be easy but if you are trying to avoid a menu, perhaps holding down the trash-can?
Out of curiosity, how seamless easy would it be to have an option if you're in the analysis board tool to have it become editable? On web, that option exists. Technically you can just grab the fen/pgn and then load position and choose board editor option but just curious if you could simply but easily enter board editor mode directly from analysis board without much configuration

tom-anders · 2024-09-07T07:54:41Z

1. Would it make sense to have a "clear board" option similar to how web has one? If there was a menu that would be easy but if you are trying to avoid a menu, perhaps holding down the trash-can?

Yeah, what's still missing is a "Load Position" menu, where you can set the board to a specific opening or endgame position. We could also have a "Clear Board" entry in that list (I think the old app does as well).

2. Out of curiosity, how seamless easy would it be to have an option if you're in the analysis board tool to have it become editable? On web, that option exists. Technically you can just grab the fen/pgn and then load position and choose board editor option but just curious if you could simply but easily enter board editor mode directly from analysis board without much configuration

Yeah sounds useful, shouldn't be too hard to implement.

I'll open Github issues for both of these features, I think these are "Good first issue" candidates

tom-anders mentioned this pull request Jul 15, 2024

feat: add a separate widget for BoardEditor lichess-org/flutter-chessground#40

Closed

tom-anders force-pushed the board_editor branch from ef52aa5 to 7d41cf1 Compare July 28, 2024 21:52

tom-anders marked this pull request as ready for review July 28, 2024 21:52

tom-anders force-pushed the board_editor branch from 7d41cf1 to 14859d6 Compare July 28, 2024 21:59

veloce reviewed Jul 29, 2024

View reviewed changes

tom-anders force-pushed the board_editor branch from 14859d6 to 11a1a4f Compare July 29, 2024 16:53

tom-anders force-pushed the board_editor branch from 39b5e74 to 9a4670d Compare August 3, 2024 15:10

tom-anders added 2 commits August 3, 2024 17:19

feat: upgrade to chessground 4.0.0

fb5a442

feat: add board editor

d300dee

tom-anders force-pushed the board_editor branch from 9a4670d to d300dee Compare August 3, 2024 15:19

veloce reviewed Aug 5, 2024

View reviewed changes

veloce merged commit 4246037 into lichess-org:main Aug 5, 2024
1 check passed

tom-anders mentioned this pull request Aug 10, 2024

[Feature] Add board editor in Lichess mobile v2 #911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add board editor #865

feat: add board editor #865

tom-anders commented Jul 14, 2024 •

edited

Loading

tom-anders commented Jul 14, 2024 •

edited

Loading

veloce commented Jul 15, 2024

tom-anders commented Jul 15, 2024 •

edited

Loading

tom-anders commented Jul 15, 2024

veloce commented Jul 15, 2024

tom-anders commented Jul 16, 2024

veloce commented Jul 24, 2024

tom-anders commented Jul 24, 2024

tom-anders commented Jul 24, 2024

tom-anders commented Jul 24, 2024 •

edited

Loading

veloce commented Jul 25, 2024 •

edited

Loading

veloce commented Jul 25, 2024

tom-anders commented Jul 28, 2024

veloce commented Jul 29, 2024 •

edited

Loading

veloce Jul 29, 2024

tom-anders Jul 29, 2024 •

edited

Loading

tom-anders Jul 29, 2024

tom-anders Jul 29, 2024

veloce commented Aug 2, 2024

tom-anders commented Aug 3, 2024

veloce left a comment

veloce Aug 5, 2024

veloce Aug 5, 2024

veloce Aug 5, 2024

veloce Aug 5, 2024

veloce Aug 5, 2024

veloce Aug 5, 2024 •

edited

Loading

tom-anders Aug 5, 2024

tom-anders commented Aug 5, 2024

ijm8710 commented Sep 6, 2024

tom-anders commented Sep 7, 2024

feat: add board editor #865

feat: add board editor #865

Conversation

tom-anders commented Jul 14, 2024 • edited Loading

tom-anders commented Jul 14, 2024 • edited Loading

veloce commented Jul 15, 2024

tom-anders commented Jul 15, 2024 • edited Loading

tom-anders commented Jul 15, 2024

veloce commented Jul 15, 2024

tom-anders commented Jul 16, 2024

veloce commented Jul 24, 2024

tom-anders commented Jul 24, 2024

tom-anders commented Jul 24, 2024

tom-anders commented Jul 24, 2024 • edited Loading

veloce commented Jul 25, 2024 • edited Loading

veloce commented Jul 25, 2024

tom-anders commented Jul 28, 2024

veloce commented Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

tom-anders Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloce commented Aug 2, 2024

tom-anders commented Aug 3, 2024

veloce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloce Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-anders commented Aug 5, 2024

ijm8710 commented Sep 6, 2024

tom-anders commented Sep 7, 2024

tom-anders commented Jul 14, 2024 •

edited

Loading

tom-anders commented Jul 14, 2024 •

edited

Loading

tom-anders commented Jul 15, 2024 •

edited

Loading

tom-anders commented Jul 24, 2024 •

edited

Loading

veloce commented Jul 25, 2024 •

edited

Loading

veloce commented Jul 29, 2024 •

edited

Loading

tom-anders Jul 29, 2024 •

edited

Loading

veloce Aug 5, 2024 •

edited

Loading