keymap fixes, takeback feature

JeromeSchmied commented 7 months ago

This creates an error: Pressing Esc return to Home when playing. I'll fix it soon(probably tomorrow).

JeromeSchmied commented 7 months ago

As far as I could understand the Pages enum, the Help variant was completely unnecessary and just made life harder. Therefore I removed it and utilized the app.show_help_popup. I also removed 'x' keybinding and replaced it's functionality with Esc. Let me know if there's anything that you think could further be changed.

JeromeSchmied commented 7 months ago

Sorry bro dunno how this works

JeromeSchmied commented 7 months ago

I haven't tested many scenarios, but one thing that still doesn't work is promotions. I'll try to implement them though.

thomas-mauran commented 7 months ago

I haven't tested many scenarios, but one thing that still doesn't work is promotions. I'll try to implement them though.

Nice job already hope you will be able to get over the promotion !

JeromeSchmied commented 7 months ago

Now that I know the code somewhat better, I got fed up with the way coordinates are stored, so this is what I came up with: instead of [i8; 2] or Vec<i8>, which is not that much to the point to tell the truth, I made something like this:

struct Coordinate {
    x: i8,
    y: i8,
}

I also started rewriting every occurence to comply with this new Coordinate design. It is quite a pain in the neck, but overall I'd say it's worth the time.

What do you think about it?

thomas-mauran commented 7 months ago

Now that I know the code somewhat better, I got fed up with the way coordinates are stored, so this is what I came up with: instead of [i8; 2] or Vec<i8>, which is not that much to the point to tell the truth, I made something like this:
struct Coordinate {
    x: i8,
    y: i8,
}
I also started rewriting every occurence to comply with this new Coordinate design. It is quite a pain in the neck, but overall I'd say it's worth the time.

What do you think about it?

might be a pain in the ass but can be worth it in the end so what does x and y represent x being the rows and y the lines of the board ?

JeromeSchmied commented 7 months ago

Now that I know the code somewhat better, I got fed up with the way coordinates are stored, so this is what I came up with: instead of [i8; 2] or Vec<i8>, which is not that much to the point to tell the truth, I made something like this:
struct Coordinate {
    x: i8,
    y: i8,
}
I also started rewriting every occurence to comply with this new Coordinate design. It is quite a pain in the neck, but overall I'd say it's worth the time. What do you think about it?
might be a pain in the ass but can be worth it in the end so what does x and y represent x being the rows and y the lines of the board ?

just the other way round: y: rows, x: lines

thomas-mauran commented 7 months ago

Now that I know the code somewhat better, I got fed up with the way coordinates are stored, so this is what I came up with: instead of [i8; 2] or Vec<i8>, which is not that much to the point to tell the truth, I made something like this:
struct Coordinate {
    x: i8,
    y: i8,
}
I also started rewriting every occurence to comply with this new Coordinate design. It is quite a pain in the neck, but overall I'd say it's worth the time. What do you think about it?
might be a pain in the ass but can be worth it in the end so what does x and y represent x being the rows and y the lines of the board ?
just the other way round: y: rows, x: lines

ok i see maybe we could directly put

struct Coordinate { line: i8, row: i8, }

for it to be very clear ?

JeromeSchmied commented 7 months ago

Now that I know the code somewhat better, I got fed up with the way coordinates are stored, so this is what I came up with: instead of [i8; 2] or Vec<i8>, which is not that much to the point to tell the truth, I made something like this:
struct Coordinate {
    x: i8,
    y: i8,
}
I also started rewriting every occurence to comply with this new Coordinate design. It is quite a pain in the neck, but overall I'd say it's worth the time. What do you think about it?
might be a pain in the ass but can be worth it in the end so what does x and y represent x being the rows and y the lines of the board ?
just the other way round: y: rows, x: lines
ok i see maybe we could directly put

struct Coordinate { line: i8, row: i8, }

for it to be very clear ?

only: col (column) instead of line, is that good?

thomas-mauran commented 7 months ago

Now that I know the code somewhat better, I got fed up with the way coordinates are stored, so this is what I came up with: instead of [i8; 2] or Vec<i8>, which is not that much to the point to tell the truth, I made something like this:
struct Coordinate {
    x: i8,
    y: i8,
}
I also started rewriting every occurence to comply with this new Coordinate design. It is quite a pain in the neck, but overall I'd say it's worth the time. What do you think about it?
might be a pain in the ass but can be worth it in the end so what does x and y represent x being the rows and y the lines of the board ?
just the other way round: y: rows, x: lines
ok i see maybe we could directly put struct Coordinate { line: i8, row: i8, } for it to be very clear ?
only: col (column) instead of line, is that good?

sounds good to me !

JeromeSchmied commented 7 months ago

Fixes #55, #56, hopefully will once fix #52.

JeromeSchmied commented 7 months ago

As I'm working on dealing with edge cases for takeback:

castle
en-passant
promotion

I explored some weird things: I wrote some tests, made Coords check(panic) if a coordinate is not between 0 and 7. Well that happens quite often, not only when UNDEFINED_POSITION is used. Why is that? I can't work it out really, if I'm right, only between (0;0) and (7;7) coordinates are correct to index board which I made a type alias now(also Piece).

JeromeSchmied commented 7 months ago

One more thing: board::tests::takeback_kick is a test I wrote to check whether takeback works in case of a piece has been kicked, needs to be placed back on the board. Well as far as you can tell (from actually playing with the game) it works. But the test fails for some reason I can't find. Could you check please? it'd be a huge help.

thomas-mauran commented 6 months ago

One more thing: board::tests::takeback_kick is a test I wrote to check whether takeback works in case of a piece has been kicked, needs to be placed back on the board. Well as far as you can tell (from actually playing with the game) it works. But the test fails for some reason I can't find. Could you check please? it'd be a huge help.

Hi, thanks for the feedback I will check both your test and the panick you mentioned above in the week (lot of work in parallel :disappointed: I don't have a lot of time) !

JeromeSchmied commented 6 months ago

One more thing: board::tests::takeback_kick is a test I wrote to check whether takeback works in case of a piece has been kicked, needs to be placed back on the board. Well as far as you can tell (from actually playing with the game) it works. But the test fails for some reason I can't find. Could you check please? it'd be a huge help.

Hi, thanks for the feedback I will check both your test and the panick you mentioned above in the week (lot of work in parallel 😞 I don't have a lot of time) !

I'm sorry to hear. It can wait anyway.

thomas-mauran commented 6 months ago

How is it going @JeromeSchmied I see that you made a lot of commits which is great ! feel free to put the pr in a ready tstae when you need be to review it

joshka commented 6 months ago

Hey @JeromeSchmied,

I was looking at the source and was going to submit some ideas I had to improve this, but this PR seems to be a large rewrite of a significant portion of the app. It has many different changes rolled up into a single PR. Monolithic changes like this are problematic as it makes it difficult to work in any areas of the app without conflict, and they're more difficult to review as the time investment to do so scales exponentially (or at least more than linearly) with the size of the changes.

@thomas-mauran / @JeromeSchmied what's your preferred way forward here? I see a couple of options:

Get this to a state where it can be reviewed and merge it soon?
Split this up into smaller mergable chunks. I think there's some really good ideas in here that could be easily broken up into smaller PRs if you wanted to.
Submit PRs that ignore what's going on with this PR and expect this PR to merge those changes instead.

Regarding coordinates: currently these are Row/Col but the chess terms are Rank/File, why not make these explicit enums and implement ops::Index against the board https://doc.rust-lang.org/std/ops/trait.Index.html

JeromeSchmied commented 6 months ago

@joshka thanks for this comment, I couldn't agree with you more. The only reason for putting this huge amount of commits in one PR is because I have no idea how to create more than one PRs at once, but I spotted out some crucial issues in the code while working on the takeback.

issues? I fixed:

Using Coords has a huge impact on development speed I think.
Handling key release events just like you did it in #59 which is in fact prettier code than the one I wrote.
Usage of methods instead of functions taking potential self as argument.
Popup window handling.
fen import
movehistory now doesn't hold an Option<PieceType>, but a PieceType as it can't anyway, right?

issues I've been working on, but are not yet done:

takeback
pgn import

To resolve the conflict, in my opinion it'd be best if I reverted my commits about

takeback(which can be written a lot more easily once pgn import is done I guess)
pgn import
and also KeyRelease fix and then make PRs for each remaining, also implement std::ops::Index for Board.

I'll see what I can do.

JeromeSchmied commented 6 months ago

I'm sorry, it looks like I can't actually revert any commits other than KeyRelease fix from the issues I just mentioned, as my git history is such a huge mess.

To fix this, may we merge this PR as is, if I just comment out code using functions I wrote for pgn import (takeback is already commented out), so that the extra code I wrote will remain there, and soon I'll try to implement then one by one.

Is this ok?

Sorry for any inconveniences I made.

joshka commented 6 months ago

Hey, no problem on the inconvenience. If @thomas-mauran is ok with your direction, then that's his call as he code owner, not mine. I have a preference for small orthogonal chunks of work over large ones, but not everyone shares that approach.

It sounds like you're using git as a single branch in your local repository, and so you're thinking about the history is one dimensional. What I'd generally try to do in this sort of thing is a branch for each feature, trying to make each branch have little overlaps in the code that they're touching, and having a clear goal / finished state. Thing two dimensional in terms of non-overlapping areas.

At a high level splitting up the PR looks something like:

You're currently using a local branch named main, this can make things a little confusing as your main and the main here are two different things. Run git branch all-the-things to create a new branch at the current point in your local to come back to if you need to, then run git reset --hard upstream/main to reset your main branch to be the same as the repo main. (your remote name might be different than upstream, run git remote -v to check).
Create and switch to a new branch for the first feature you want to merge (e.g. git switch -c coords)
Cherry pick commits that have the code that you want to be part of that branch (e.g. git cherry-pick fc42fc5) to bring in fc42fc53fadf1fd5e00ffc08dc6f839036f70ca0 and then pick up more commits to make up the PR
Resolve any conflicts (there will probably be some places where you update code in unrelated parts to use the new code)
Make sure tests etc. compile
Create a PR from that new branch - hopefully being smaller will make it easier to review and merge quickly
switch to the all-the-things branch
rebase the all-the-things branch onto the coords branch (you can also create a new branch to do this on if you want the placeholder branch in place to come back to at some later date to compare). You'll notice that any of the cherry picked commits will be removed as they have already been applied to the source, though you might have to resolve some conflicts.

Repeat for each small chunk of work.

Another approach is to just throwaway your changes (but use them as a reference point for doing them again right upfront). I do this fairly often myself (implement something incrementally and then redo it correctly).

The last approach is just to get this to a point where it's bug free and mergable as-is. I think this might be difficult.

thomas-mauran commented 6 months ago

Yep, as @joshka said the best thing would be to split your pr in multiple ones, thanks a lot @joshka for providing details on how to do that easily for @JeromeSchmied I think this will be very hepful. @JeromeSchmied tell me if you are having any issue splitting the pr

JeromeSchmied commented 6 months ago

I started extracting my commits to different PR-s. I think I shall close this one, as I'll make new ones anyway, right?

joshka commented 6 months ago

I started extracting my commits to different PR-s. I think I shall close this one, as I'll make new ones anyway, right?

Sounds good!

thomas-mauran / chess-tui

keymap fixes, takeback feature #54