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

12632 - Broaden search for "solo" PlayerSide. #12636

Closed
wants to merge 11 commits into from
Closed

12632 - Broaden search for "solo" PlayerSide. #12636

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fba93fc
Broaden search for "solo" PlayerSide.
riverwanderer Sep 2, 2023
583e6eb
Broaden search for "solo" PlayerSide. Try with space rather than dot …
riverwanderer Sep 2, 2023
903d382
Merge remote-tracking branch 'origin/12632_moreSoloSides' into 12632_…
riverwanderer Sep 2, 2023
b9be8a5
Broaden search for "solo" PlayerSide, including a side in comma-separ…
riverwanderer Sep 3, 2023
85ca949
Broaden search for "solo" PlayerSide, including a side in comma-separ…
riverwanderer Sep 3, 2023
f5bc754
Broaden search for "solo" PlayerSide, including a side in comma-separ…
riverwanderer Sep 3, 2023
1a9056b
Back out exclude list property, use side name a special character pre…
riverwanderer Sep 11, 2023
843c2e8
Back out exclude list property, use side name a special character pre…
riverwanderer Sep 11, 2023
6b4a330
Merge remote-tracking branch 'origin/12632_moreSoloSides' into 12632_…
riverwanderer Sep 11, 2023
b9005f7
Merge branch 'master' into 12632_moreSoloSides
riverwanderer Jan 18, 2024
65da192
Remove duplicated, prior version of isSoloSide()
riverwanderer Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions vassal-app/src/main/java/VASSAL/build/module/PlayerRoster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,13 +622,36 @@ protected boolean allSidesAllocated() {

/**
* @param side Name of a side to see if it's a "solo side"
* @return True if the side is "Solitaire", "Solo", "Moderator", or "Referee"
* @return True if the side is "Solo" or begins "Solitaire", "Solo:", "Moderator", "Referee" or a Side
* found in the Global Translatable Message Property: VassalSkipSideList (a comma-separated list).
* To test specifically for Referee/Moderator, use isRefereeSide()
*/
public static boolean isSoloSide(String side) {
return Resources.getString("PlayerRoster.solitaire").equals(side) ||
Resources.getString("PlayerRoster.solo").equals(side) ||
Resources.getString("PlayerRoster.moderator").equals(side) ||
Resources.getString("PlayerRoster.referee").equals(side);
// FIXME: A future version of Vassal may provide a Side Type attribute in Definition of Sides that would allow this function to be rationalised.
// See also isRefereeSide()
return side.startsWith(Resources.getString("PlayerRoster.solitaire")) ||
side.equals(Resources.getString("PlayerRoster.solo")) ||
side.startsWith(Resources.getString("PlayerRoster.solo") + ":") ||
side.startsWith(Resources.getString("PlayerRoster.moderator")) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why "startsWith" here where it used to only check equals? Are we expecting "Moderator Left" and "Moderator Right"?

If so, why are we requiring a different format here from Solo (where it requires the colon e.g. "Solo:")? And for that matter why do we only eg allow "Solo:" and not "Solitaire:"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My thinking on prefixes (startsWith) is to allow more options for filtering out Solitaire or Referee sides. I think multiple moderator sides is a possibility but mainly it didn't seem harmful to allow this.

I introduced "Solo:" specifically because "Solo" prefix has the remote possibility of conflicting with an actual side name. I could use this to enable choosing one of 3 solo modes directly from the side selection phase.

side.startsWith(Resources.getString("PlayerRoster.referee")) ||
side.startsWith(Resources.getString("PlayerRoster.auxiliarySidePrefix")) || // auxiliary side prefix
side.startsWith(SOLITAIRE) || // these checks needed in case a translated module has not localised these side names
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these really needed? If a translated module hasn't localized the side names don't they come through in English anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the actual problem is in the ChessClock which calling isSoloSide with the translated Sides, it should be checking untranslated sides, but displaying translated sides,

The concern is that ChessClocks now have translated sides recorded in them?, they should be untranslated sides.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@BrentEaston So you think this usage is okay? Also are you okay with the various changes to "startsWith"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@BrentEaston And we're comparing both Resources.getString("Some String") and the literal "Some String" in here. Surely only one is right? Or is one way being left in because ChessClock is broken? (are other things broken too?)

side.startsWith(SOLO) ||
side.startsWith(SOLO + ":") ||
side.startsWith(MODERATOR) ||
side.startsWith(REFEREE);
}

/**
* @param side Name of a side to see if it's a "referee side"
* @return True if the side is begins "Moderator" or "Referee"
* To test for non-player sides in general, use isSoloSide()
*/
public static boolean isRefereeSide(String side) {
return side.startsWith(Resources.getString("PlayerRoster.moderator")) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again I'm fuzzy on the change to startsWith and also the need to compare to the raw strings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The raw strings comparison arises from a concern that I have about this along the following lines...

  1. Say a module has partial translation but not in the Definition of Sides component. This is not so outlandish, as translated PlayerSides can open up other complications if the module depends on PlayerSide, so I've found.
  2. The translation resources for solitaire etc exist regardless.... therefore when Vassal isSoloSide() scans the sides array it will attempt to find the translated side name. This will fail if the solo side names don't match in translation, but the raw string will then catch it.

Have I got that right?

side.startsWith(Resources.getString("PlayerRoster.referee")) ||
side.startsWith(MODERATOR) || // these checks needed for a translated module has not localised these side names
side.startsWith(REFEREE);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion vassal-app/src/main/java/VASSAL/build/module/chessclockcontrol/ChessClock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public long getVerified() {
* @return True if passed side is a referee
*/
protected static boolean isReferee(String name) {
return PlayerRoster.isSoloSide(name);
return PlayerRoster.isRefereeSide(name);
}

@Override
Expand Down
1 change: 1 addition & 0 deletions vassal-app/src/main/resources/VASSAL/i18n/VASSAL.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,7 @@ PlayerRoster.solitaire=Solitaire
PlayerRoster.solo=Solo
PlayerRoster.moderator=Moderator
PlayerRoster.referee=Referee
PlayerRoster.auxiliarySidePrefix=~
Cattlesquat marked this conversation as resolved.
Show resolved Hide resolved
PlayerRoster.changed_sides=* ~%1$s switched sides from %2$s to %3$s.
PlayerRoster.changed_sides_2=* ~<b>%1$s switched sides from %2$s to %3$s.</b>
PlayerRoster.joined_side=* ~%1$s joined the game as %2$s.
Expand Down
3 changes: 3 additions & 0 deletions vassal-doc/src/main/readme-referencemanual/ReferenceManual/GameModule.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ Simply type a name for each side and refer to that name in the restricted compon
Only one player may be assigned to a side.
When joining a game, players will be prompted to take one of the remaining available sides.
Any number of observers (players who belong to no side) are allowed.

When prompting for a side, a next default entry is offered. Certain names are skipped in this process; these are "Solo" and names beginning "Solitaire", "Solo:", "Moderator", "Referee" or name found in the <<GlobalTranslatableMessages.adoc#top, Global Translatable Messages>> property _VassalSkipSideList_. If used, the later should be defined as a comma-separated list of Sides.

riverwanderer marked this conversation as resolved.
Show resolved Hide resolved
The <<Toolbar.adoc#Retire,Retire>> or <<Toolbar.adoc#SwitchSides,Switch Sides>> button, in the main controls toolbar, allows a player to relinquish their side (making it available to the next player joining the game). You can specify the text, icon, and mouse-over tooltip for the toolbar button.
|===

Expand Down