Skip to content
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 undo feature #58

Merged
merged 1 commit into from
Apr 28, 2024
Merged

Add undo feature #58

merged 1 commit into from
Apr 28, 2024

Conversation

bennyE31
Copy link
Contributor

@bennyE31 bennyE31 commented Apr 9, 2024

Add feature to track previous moves and undo upon user input with 'u'.

src/main.c Outdated
@@ -74,6 +75,8 @@ static int g_sudokuCount = 1; /* in case of -n we can the numbers of sudoku
static bool g_outIsPDF;
static DIFFICULTY g_level = D_EASY;
static WINDOW *grid, *infobox, *status;
static move_t g_undo_stack[UNDO_STACK_SIZE]; /* Stack of previous moves */
static int g_undo_stack_ptr = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I dont like the _ptr without it being a *. Maybe _index would be a better description? item?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, I like index as well. updated

src/main.c Outdated
@@ -337,12 +340,34 @@ static void init_windows(void)
wprintw(infobox, _(" r - Redraw\n"));
wprintw(infobox, _(" S - Solve puzzle\n"));
wprintw(infobox, _(" x - Delete number\n"));
wprintw(infobox, _(" u - Undo previous move\n"));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not undoing moving in the grid but undoing insertions/actions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking move in the game sense like a chess "move", not a traversing the board "move." I like action as it covers both insertion and deletion

Copy link
Owner

@jubalh jubalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your PR!
I left a couple of comments.

And one more thing:
I go to a field and insert 1 I go to another and insert 2. Now I go to the field where I insert 1 and press delete. When now pressing u it is not adding 1 back.
This might be confusing.

Signed-off-by: bennye31 <bennye3797@gmail.com>
@bennyE31
Copy link
Contributor Author

Thanks a lot for your PR! I left a couple of comments.

And one more thing: I go to a field and insert 1 I go to another and insert 2. Now I go to the field where I insert 1 and press delete. When now pressing u it is not adding 1 back. This might be confusing.

Yep I had just not thought of deletion as an undoable action, which it easily can and should be. I've added that in now and it should work as you expected

@bennyE31 bennyE31 requested a review from jubalh April 12, 2024 20:05
Copy link
Owner

@jubalh jubalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long waiting time! It looks good now! I don't see a reason to have move_t in the header though. Will merge anyways :)

@jubalh jubalh merged commit 08d2724 into jubalh:master Apr 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants