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

12542 - Mitigate race condition when selecting player sides #12667

Merged
merged 78 commits into from
Sep 20, 2023
Merged

12542 - Mitigate race condition when selecting player sides #12667

merged 78 commits into from
Sep 20, 2023

Conversation

riverwanderer
Copy link
Collaborator

No description provided.

@riverwanderer
Provides facility to set the hot-seat default side via a module Globa…
91ef123 
…l Property VassalNextSide. The GP may be defined as a Preferences string property (on a blank tab to keep it hidden), in which case the VassalNextSide will take effect a per-player level.
@riverwanderer
Provides facility to set the hot-seat default side via a module Globa…
fc94dfb 
…l Property VassalNextSide. The GP may be defined as a Preferences string property (on a blank tab to keep it hidden), in which case the VassalNextSide will take effect a per-player level.
@riverwanderer
Provides facility to set the hot-seat default side via a module Globa…
026178c 
…l Property VassalNextSide. The GP may be defined as a Preferences string property (on a blank tab to keep it hidden), in which case the VassalNextSide will take effect a per-player level.
@riverwanderer
Provides facility to set the hot-seat default side via a module Globa…
b03eef8 
…l Property VassalNextSide. The GP may be defined as a Preferences string property (on a blank tab to keep it hidden), in which case the VassalNextSide will take effect a per-player level.
@riverwanderer
Validate VassalNextSide - check that it is an available side, rather …
07e3128 
…than just not unavailable.
@riverwanderer
Validate VassalNextSide - check that it is an available side, rather …
007630c 
…than just not unavailable.
@riverwanderer
Validate VassalNextSide - check that it is an available side, rather …
c5dcadb 
…than just not unavailable.
@riverwanderer
Use null-safe test on VassalNextSide.
59ea275
@riverwanderer
Ref manual updated. Supercedes equivalent update in PR vassalengine#1…
d153db4 
…2636.
@riverwanderer
Attempt race condition check for non-Wizard switch side.
afc7319
@riverwanderer
Attempt race condition check for non-Wizard switch side.
9662e1d
@riverwanderer
Attempt race condition check for non-Wizard switch side.
cdb5d2c
@riverwanderer
Attempt race condition check for non-Wizard switch side.
77a3b0e
@riverwanderer riverwanderer added bug Something isn't working Work In Progress labels Sep 7, 2023
@riverwanderer
Copy link
Collaborator Author

Latest build achieves objective as a demo for the Retire/Switch Side button. includes translations for all but Polish (unresolved issue with Commit), Japanese and Chinese.

Further work needed to

  1. Validate text for side choice unavailable (too long?).
  2. Test translations
  3. Migrate this enhancement to wizard / initial connect side choices.
  4. Investigate possible issue where an available side doesn't show after trapping a side-switch by another player (the side dropped by the other player?).

@BrentEaston BrentEaston added this to the 3.8.0 milestone Sep 8, 2023
@BrentEaston BrentEaston added the Review-Changes Requested Reviewer has request changes label Sep 8, 2023
@riverwanderer
Merge branch 'master' of https://github.com/vassalengine/vassal into …
1986378 
…12542-side-switch-race-mitigation

# Conflicts:
#	vassal-app/src/main/java/VASSAL/build/module/PlayerRoster.java
@riverwanderer
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
c075ac6 
…n' into 12542-side-switch-race-mitigation
@riverwanderer
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
7b1840b 
…n' into 12542-side-switch-race-mitigation
@riverwanderer
Update documentation to remove Solitaire changes which may layer on t…
079d27b 
…his PR.
@riverwanderer
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
5a5e01d 
…n' into 12542-side-switch-race-mitigation
@riverwanderer
Copy link
Collaborator Author

Retested OK after resolving conflicts (tests in English and French).

@riverwanderer riverwanderer added Ready to Merge YeeeeeeeeeeeHAW!!! and removed Conflicts Has Conflicts with master that need to be resolved labels Sep 14, 2023
@riverwanderer riverwanderer removed the Ready to Merge YeeeeeeeeeeeHAW!!! label Sep 19, 2023
@riverwanderer
Merge branch 'master' of https://github.com/vassalengine/vassal into …
0cbee71 
…12542-side-switch-race-mitigation

# Conflicts:
#	vassal-app/src/main/java/VASSAL/build/module/PlayerRoster.java
@riverwanderer
Re-aligned to master.
a561316
@riverwanderer
Re-aligned to master.
9604c83
@riverwanderer
Copy link
Collaborator Author

Basic tests repeated ok. Further tasks required:

  1. Re-test mixed language operations.
  2. Fix Cancel of race mitigation prompt for side (following a first time connect) - currently cancel of the "side taken" prompt will sync to the room. Note that cancel of the main prompt leaves playerside null but does not sync to the room.
  3. Retest (2) but for a log file situation.
  4. Auto-exclusion of a zero sides button does not work. setVisibility was re-added into finish() - I don't think this helps but rather button launch is where the check / use of SetVisibility should be.

@riverwanderer
Copy link
Collaborator Author

Tests out ok.

Additional fixes:

  1. Resolved an issue where connection to <observer> was initially rejects (side taken), arising from using the common function PromptForSide() to validate initial connection.
  2. Rather than allow a connection into a game as a null side in the finish() routine, the side is changed to <observer>. Possibly this should instead just not connect to the game, as it results from the user cancelling the input panel.

I have done nothing to prevent the Retire button displaying if no sides are defined. This can be achieved by not populating the side button text/image.

@riverwanderer riverwanderer added Ready for Review Ready to be reviewed for Merging and removed Work In Progress labels Sep 19, 2023
@uckelman uckelman changed the title 12542 side switch race mitigation 12542 - Mitigate race condition when selecting player sides Sep 20, 2023
@uckelman uckelman merged commit 02ee482 into vassalengine:master Sep 20, 2023
1 check passed
@riverwanderer riverwanderer mentioned this pull request Oct 20, 2023
Merged
@riverwanderer riverwanderer deleted the 12542-side-switch-race-mitigation branch November 25, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uckelman uckelman uckelman left review comments

@BrentEaston BrentEaston BrentEaston approved these changes

Assignees
No one assigned
Labels
bug Something isn't working Ready for Review Ready to be reviewed for Merging Testing Complete Independent testing completed successfully
Projects
None yet
Milestone
3.7.1
Development

Successfully merging this pull request may close these issues.

Race condition in side switcher
3 participants
@riverwanderer @BrentEaston @uckelman