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.

Sign up for GitHub

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

Jump to bottom

refactor undo handling with of synced actions. #2979

Closed
gfgtdf opened this issue Apr 28, 2018 · 5 comments
Closed

refactor undo handling with of synced actions. #2979

gfgtdf opened this issue Apr 28, 2018 · 5 comments
Labels
Engine General game engine issues that do not fit in any other category. Enhancement Issues that are requests for new features or changes to existing ones. MP Issues with multiplayer support or bundled multiplayer content. Replays Issues with game replays support. WML Issues involving the WML engine or WML APIs.

Comments

@gfgtdf
Copy link
Contributor

gfgtdf commented Apr 28, 2018

i didn't test this but from looking at the source it very much looks like this won't work as SYNCED_COMMAND_HANDLER_FUNCTION(custom_command, child, /*use_undo*/, /*show*/, /*error_handler*/) contains no code than deals with the undo stack.
@gfgtdf gfgtdf added Bug Issues involving unexpected behavior. MP Issues with multiplayer support or bundled multiplayer content. Replays Issues with game replays support. WML Issues involving the WML engine or WML APIs. Engine General game engine issues that do not fit in any other category. labels Apr 28, 2018
@gfgtdf gfgtdf self-assigned this Apr 28, 2018
@CelticMinstrel
Copy link
Member

I was thinking about that when I looked at it before, but since I didn't really know how the undo stack works and didn't feel it was high priority, I elected to let it go. I also kinda wonder if we should be using show and error_handler for something in custom_command.

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Apr 28, 2018

Iirc show basically forward things like 'skip_ai_turn', 'skip_replay' etc. Maybe we refactor it out in favour of a function like play_comtroller->is_skipping() , error_handler is mostly used to indicate invalid content inthe'child' parameter, not sure how to implement the since Lua dies not have something like typed exceptions

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Apr 28, 2018

I am also considering dropping the use_undo parameter, afaik the onyl reason it exists is an optimisation in the ai code not to bother with undo stacks. But that is probably negltable while on the other hand it surely does make that code a bit more complicated.

gfgtdf added a commit that referenced this issue Apr 29, 2018
@gfgtdf
fix [on_undo] in menu items and custom_command
7751b79 
also fixes menu items beeing marked as undoabel even if they used the
synced rng.

#2979
@gfgtdf
Copy link
Contributor Author

gfgtdf commented Apr 29, 2018

the main issue is fixed, i still think it'd be nice to refactor it soemhow so that this type of error is no longer possible.

@gfgtdf gfgtdf added Enhancement Issues that are requests for new features or changes to existing ones. and removed Bug Issues involving unexpected behavior. labels Apr 29, 2018
@gfgtdf gfgtdf changed the title [on_undo] does not work during custom_command refactor undo handling with of synced actions. Aug 11, 2018
jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 6, 2018
@gfgtdf @jostephd
fix [on_undo] in menu items and custom_command
cbec317 
also fixes menu items beeing marked as undoabel even if they used the
synced rng.

wesnoth#2979
jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018
@gfgtdf
fix [on_undo] in menu items and custom_command
9ac25a8 
also fixes menu items beeing marked as undoabel even if they used the
synced rng.

wesnoth#2979

(cherry-picked from commit 7751b79)
@Pentarctagon
Copy link
Member

Closing since the issue reportedly was fixed, and IMO "refactor it somehow" isn't a valid issue topic (especially since gfgtdf is MIA).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Engine General game engine issues that do not fit in any other category. Enhancement Issues that are requests for new features or changes to existing ones. MP Issues with multiplayer support or bundled multiplayer content. Replays Issues with game replays support. WML Issues involving the WML engine or WML APIs.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@CelticMinstrel @Pentarctagon @gfgtdf