468 show captured pieces instead of material imbalance #1142
Conversation
|
UI looks fine! |
|
I added a secondary option to the settings for this. I wonder if this is the best approach, it was simple to add it this way but maybe a single choice with 3 options would be better 
Was also a bit unclear on why the theme preference implementation did something custom for the choice picker on ios, this seems to work but can take the other approach if there is a reason to do so |
|
I think it makes more sense to have one setting with 3 options. Rather than 2 separate toggle due to the fact that the settings screen keeps getting more options and to limit it best we can. Obv Veloce/toms opinion will carry more weight but I think it's best to keep that contained best they can |
|
Alternate option 
|
|
In-game option Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-14.at.08.55.42.mp4 |
|
Would appreciate a review @veloce @tom-anders |
|
It is better with a single setting yes. Thanks! |
| Board.standard.materialCount(Side.black); | ||
| final IMap<Role, int> whiteStartingCount = | ||
| Board.standard.materialCount(Side.white); | ||
| // TODO: parameterise starting position maybe so it can be passed in |
There was a problem hiding this comment.
Yes I think this is needed before merging - It's already possible to view (but not play) variants that have a different starting position (e.h. Horde). With this PR, capturedPieces will display incorrect values currently.
However, I noticed that the web UI does not display material difference at all for Horde, maybe we should do the same? @veloce wdyt?
| @@ -110,7 +119,9 @@ class BoardPrefs with _$BoardPrefs implements Serializable { | |||
| required bool boardHighlights, | |||
| required bool coordinates, | |||
| required bool pieceAnimation, | |||
| required bool showMaterialDifference, | |||
| // required bool showMaterialDifference, | |||
| required MaterialDifference materialDifference, | |||
There was a problem hiding this comment.
Calling this MaterialDifferenceFormat (like in the commented line below) would be more readable I think. Currently, there's both MaterialDiff and MaterialDifference which is confusing
|
@tom-anders I pushed up a bunch of changes, would appreciate you taking another look when you have the time 🙂 |
lib/src/view/game/game_player.dart
Outdated
| : '', | ||
| ), | ||
| ], | ||
| else if (materialDiff != null && |
There was a problem hiding this comment.
I'm wondering if both these conditions should be moved into the new MaterialDifferenceDisplay's build() method as well (returning SizedBox.shrink() if either of them is false) . This way we'd have all the logic in one place. (it would also make the ternary in lib/src/view/game/game_body.dart l. 131 redundant)
|
very nice, no more comments from me :) |
|
Oh should have said, I still don't really know the best label to use for the setting. I just changed it to simply "Material" just now but I've changed it a few times and still can't decide what is best. Happy to be told here! |
Thanks, will take a look at the l10n stuff tomorrow |
Jimima
force-pushed
the
468-show-captured-pieces-instead-of-material-imbalance
branch
from
edd44f6
to
a4716f7
Compare
… of github.com:Jimima/lichess-mobile into 468-show-captured-pieces-instead-of-material-imbalance
|
@veloce @tom-anders I think I've gone as far as I can with this. Would appreciate any further feedback you have for me 🙂 |
Had a go at this, draft at this stage as there is a bit to do but wanted feedback on the overall idea, if possible
Relates #468