Add ASCII mode #60
Merged
Add ASCII mode #60
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
thomas-mauran
requested changes
Mar 24, 2024
There was a problem hiding this comment.
Nice code thanks for the contribution, while reading I noted a few ideas that might be cool to also implement in this pr:
- as mentioned in the code this PR could be a good start for having a more general theme selector in the game. The ascii theme could be different in style and color while adding as you did the ascii chess characters. Maybe the board could only use black and white and no background for the cell but only a border when the cell is white ? I don't know at all what could be done here but you can be creative any ideas are welcomed !
- Can you center vertically the letters so they are in the middle of the cell ?
- I was having trouble playing this the GNU Chess notation because the letter color changes depending of the cell color, with a more black and white theme with a black background and different types of cell borders this might be easier to play, like the screenshot mentioned in the issue Ascii mode #58 where the background stays black and the pieces are easier to differentiate due to their size
- run cargo clippy
Apart from those propositions thanks again for your contribution !
src/board.rs
Outdated
Comment on lines
790
to
811
| let paragraph = match self.display_mode { | ||
| DisplayMode::DEFAULT => { | ||
| let color_enum = color_to_ratatui_enum(piece_color); | ||
|
|
||
| let color_enum = color_to_ratatui_enum(piece_color); | ||
| let piece_enum = PieceType::piece_type_to_string_enum(piece_type); | ||
| // Place the pieces on the board | ||
| Paragraph::new(piece_enum).fg(color_enum) | ||
| } | ||
| DisplayMode::ASCII => { | ||
| // Determine piece letter case | ||
| let piece_enum_case = match piece_color { | ||
| // pieces belonging to the player on top will be lower case | ||
| Some(PieceColor::Black) => piece_enum.to_lowercase(), | ||
| // pieces belonging to the player on bottom will be upper case | ||
| Some(PieceColor::White) => piece_enum.to_uppercase(), | ||
| // Pass through original value | ||
| None => String::from(piece_enum), | ||
| }; | ||
|
|
||
| // Place the pieces on the board | ||
| Paragraph::new(piece_enum_case) | ||
| } | ||
| }; |
There was a problem hiding this comment.
this match could be in another function returning the Paragraph, this way if we need to add more "themes" it will be easier
There was a problem hiding this comment.
Where do you think is the best place to move this code to?
There was a problem hiding this comment.
Maybe we could put that in utils ? I don't know if you have a better idea
There was a problem hiding this comment.
Yeah, that makes sense. I moved it, let me know if this looks good.
|
Output of Checking chess-tui v1.2.0 (/home/snehanshu/projects/chess-tui)
warning: equality checks against false can be replaced by a negation
--> src/board.rs:318:28
|
318 | if self.is_promotion == false {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `!self.is_promotion`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
= note: `#[warn(clippy::bool_comparison)]` on by default
warning: this match could be written as a `let` statement
--> src/board.rs:366:17
|
366 | / match (
367 | | get_piece_type(self.board, [i, j]),
368 | | get_piece_color(self.board, [i, j]),
369 | | ) {
... |
397 | | }
398 | | }
| |_________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding
= note: `#[warn(clippy::match_single_binding)]` on by default
help: consider using a `let` statement
|
366 ~ let (piece_type, piece_color) = (
367 + get_piece_type(self.board, [i, j]),
368 + get_piece_color(self.board, [i, j]),
369 + );
370 + match PieceType::piece_to_fen_enum(piece_type, piece_color) {
371 + // Pattern match directly on the result of piece_to_fen_enum
372 + "" => {
373 + // Check if the string is not empty before using chars().last()
374 + if let Some(last_char) = result.chars().last() {
375 + if last_char.is_ascii_digit() {
376 + let incremented_char = char::from_digit(
377 + last_char.to_digit(10).unwrap_or(0) + 1,
378 + 10,
379 + )
380 + .unwrap_or_default();
381 + // Remove the old number and add the new incremented one
382 + result.pop();
383 + result.push_str(incremented_char.to_string().as_str());
384 + } else {
385 + result.push('1');
386 + }
387 + } else {
388 + result.push('1');
389 + }
390 + }
391 + letter => {
392 + // If the result is not an empty string, push '1'
393 + result.push_str(letter);
394 + }
395 + }
|
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:419:17
|
419 | result.push_str("q");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push('q')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
= note: `#[warn(clippy::single_char_add_str)]` on by default
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:441:21
|
441 | result.push_str(" ");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push(' ')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:449:9
|
449 | result.push_str(" ");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push(' ')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:452:9
|
452 | result.push_str(" ");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push(' ')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
warning: unneeded `return` statement
--> src/board.rs:470:17
|
470 | return false;
| ^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
= note: `#[warn(clippy::needless_return)]` on by default
help: remove `return`
|
470 - return false;
470 + false
|
warning: `if` chain can be rewritten with `match`
--> src/board.rs:552:13
|
552 | / if distance > 0 {
553 | | row_index_rook = 3;
554 | | if self.is_game_against_bot && self.player_turn == PieceColor::Black {
555 | | to_x = 0;
... |
561 | | }
562 | | }
| |_____________^
|
= help: consider rewriting the `if` chain to use `cmp` and `match`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
= note: `#[warn(clippy::comparison_chain)]` on by default
warning: `chess-tui` (lib) generated 8 warnings (run `cargo clippy --fix --lib -p chess-tui` to apply 6 suggestions)
Finished dev [unoptimized + debuginfo] target(s) in 0.69s
snehanshu@SM-PF3WY6M6:~/projects/chess-tui$ cargo clippy > clippy
warning: equality checks against false can be replaced by a negation
--> src/board.rs:318:28
|
318 | if self.is_promotion == false {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `!self.is_promotion`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
= note: `#[warn(clippy::bool_comparison)]` on by default
warning: this match could be written as a `let` statement
--> src/board.rs:366:17
|
366 | / match (
367 | | get_piece_type(self.board, [i, j]),
368 | | get_piece_color(self.board, [i, j]),
369 | | ) {
... |
397 | | }
398 | | }
| |_________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding
= note: `#[warn(clippy::match_single_binding)]` on by default
help: consider using a `let` statement
|
366 ~ let (piece_type, piece_color) = (
367 + get_piece_type(self.board, [i, j]),
368 + get_piece_color(self.board, [i, j]),
369 + );
370 + match PieceType::piece_to_fen_enum(piece_type, piece_color) {
371 + // Pattern match directly on the result of piece_to_fen_enum
372 + "" => {
373 + // Check if the string is not empty before using chars().last()
374 + if let Some(last_char) = result.chars().last() {
375 + if last_char.is_ascii_digit() {
376 + let incremented_char = char::from_digit(
377 + last_char.to_digit(10).unwrap_or(0) + 1,
378 + 10,
379 + )
380 + .unwrap_or_default();
381 + // Remove the old number and add the new incremented one
382 + result.pop();
383 + result.push_str(incremented_char.to_string().as_str());
384 + } else {
385 + result.push('1');
386 + }
387 + } else {
388 + result.push('1');
389 + }
390 + }
391 + letter => {
392 + // If the result is not an empty string, push '1'
393 + result.push_str(letter);
394 + }
395 + }
|
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:419:17
|
419 | result.push_str("q");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push('q')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
= note: `#[warn(clippy::single_char_add_str)]` on by default
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:441:21
|
441 | result.push_str(" ");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push(' ')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:449:9
|
449 | result.push_str(" ");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push(' ')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
warning: calling `push_str()` using a single-character string literal
--> src/board.rs:452:9
|
452 | result.push_str(" ");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `result.push(' ')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
warning: unneeded `return` statement
--> src/board.rs:470:17
|
470 | return false;
| ^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
= note: `#[warn(clippy::needless_return)]` on by default
help: remove `return`
|
470 - return false;
470 + false
|
warning: `if` chain can be rewritten with `match`
--> src/board.rs:552:13
|
552 | / if distance > 0 {
553 | | row_index_rook = 3;
554 | | if self.is_game_against_bot && self.player_turn == PieceColor::Black {
555 | | to_x = 0;
... |
561 | | }
562 | | }
| |_____________^
|
= help: consider rewriting the `if` chain to use `cmp` and `match`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
= note: `#[warn(clippy::comparison_chain)]` on by default
warning: `chess-tui` (lib) generated 8 warnings (run `cargo clippy --fix --lib -p chess-tui` to apply 6 suggestions)
Finished dev [unoptimized + debuginfo] target(s) in 0.02s |
|
How does this look?
|
to fix the lint with clippy I often use this command: |
@thomas-mauran sure, but those warnings aren't related to my changes, so I don't think that's relevant in this PR. |
ah yes my bad, I will fix them in another small lint pr when adding the lint test to the ci 👍 |
|
I underlined the current user's pieces. I didn't bold anything because I think it looks fine without it. |
looking great like that mate ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add ASCII mode
Description
Fixes #58
--asciiflag (suggested here). Please let me know if this flag will be desirable.How Has This Been Tested?
I have tested this manually, and all the existing test cases pass. However, I haven't written any tests for board rendering, since there doesn't seem to be any such existing tests.
Checklist: