-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add ASCII mode #60
Add ASCII mode #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you think is the best place to move this code to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could put that in utils ? I don't know if you have a better idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 👍 |
Add ASCII mode
Description
Fixes #58
--ascii
flag (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: