lichess-org
1043 add castling method option v2
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.
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 https://github.com/lichess-org/dartchess/pull/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
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.
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 Used translation strings for labels etc.
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 🙂
Will review asap.
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.
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
- Move king two squares
- Either (move king two squares or move king onto rook)
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.
https://github.com/user-attachments/assets/0dbda333-c97d-4349-b4af-fb226e3d5f7d
Sorry @Jimima for the delay I was focused on fixing the new theme. Could you fix the conflicts and I look at this asap. Thanks!
Oh, by the way if we have to support the "either" as the website I'd rather have an "either" choice in the settings as you did initially and asked you to remove... sorry about that.
@veloce thanks, and no problem at all. I am still not sure of the correct approach here, as I said, happy to take a steer from you if you think there is more to do with refactoring or whatever. I might need some guidance, however :-)
@veloce I had a go at the tests. I can check that clicking on the king highlights the relevant squares as valid moves. I wanted to also test that you could/could not perform a castling move in line with the selected preference. I tried (for way too long) to find a way to make this work but couldn't really figure it out. I think I need to learn a bit more about how everything fits together regarding riverpod, testing, mocking, overrides etc. If you have any guidance here I would appreciate it.
In the process I wondered it in any case the widget should be using the shared state directly rather than being passed in. I think this would simplify mocking the state in the tests but I'm not really sure.
Would really appreciate any guidance you can give me on how to progress this. I also appreciate you are likely very busy so don't feel you need to prioritise responding to me!
@veloce I made the requested changes and started to implement the tests on the Analysis screen. I couldn't figure out how to override the preferences, however. I am really not very familiar with this side of things (testing/provider/mocks etc.) so if you can point me in the right direction I would appreciate it. I was trying various things and not really getting very far so thought I would ask to save my sanity, hope you don't mind!
@Jimima to override the preferences you have the defaultPreferences parameter of the helper function makeTestProviderScopeApp
Here is an example of its usage for the engine test:
https://github.com/lichess-org/mobile/blob/d285854e238b6d1dfd12caa3eb4f0bc9400b32f2/test/view/engine/test_engine_app.dart#L56
@veloce added tests on the Analysis screen, please feel free to review when able.
Sorry @Jimima I had to refactor, as I didn't want to parse the fen a second time for nothing in InteractiveBoardWidget. And I removed the either option to do the same as website, where kingOverRook acts as either. I also added tests for game screen.
I realise this PR was much harder to do than it was supposed to, because the castling option was not planned from the beginning.
That's cool, I was struggling to find the time to work on it in any case! I'll review what you did just for my learning, thanks