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

13505 - Prevent GameRefresher moving Decks with duplicate names #13507

Open
wants to merge 2 commits into
base: release-3.7
Choose a base branch
from
Open

13505 - Prevent GameRefresher moving Decks with duplicate names #13507

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b426c65
13505 - Prevent GameRefresher moving Decks with duplicate names
Jul 26, 2024
4b47553
Fix bad fix
Jul 27, 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
127 changes: 77 additions & 50 deletions vassal-app/src/main/java/VASSAL/build/module/GameRefresher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,59 +417,43 @@ public void execute(Set<String> options, Command command) throws IllegalBuildExc
for (final GamePiece pieceOrStack : map.getPieces()) {
if (pieceOrStack instanceof Deck) {
final Deck deck = (Deck) pieceOrStack;
// Match with a DrawPile if possible
boolean deckFound = false;
for (final DrawPile drawPile : drawPiles) {
final String deckName = deck.getDeckName();
if (deckName.equals(drawPile.getAttributeValueString(SetupStack.NAME))) {

// If drawPile is owned by a specific board, then we can only match it if that board is active in this game
if (drawPile.getOwningBoardName() != null) {
if (map.getBoardByName(drawPile.getOwningBoardName()) == null) {
continue;
}

// If the drawPile is on a map that doesn't have its current owning board active, then we
// cannot match that drawPile.
if (drawPile.getMap().getBoardByName(drawPile.getOwningBoardName()) == null) {
continue;
}
}

deckFound = true;
foundDrawPiles.add(drawPile);

final String drawPileName = drawPile.getAttributeValueString(SetupStack.NAME);

if (!options.contains(SUPPRESS_INFO_REPORTS))
log(Resources.getString("GameRefresher.refreshing_deck", deckName, drawPileName));

// This refreshes the existing deck with all the up-to-date drawPile fields from the module
deck.removeListeners();
deck.myRefreshType(drawPile.getDeckType());
deck.addListeners();

// Make sure the deck is in the right place
final Point pt = drawPile.getPosition();
final Map newMap = drawPile.getMap();
if (newMap != map) {
map.removePiece(deck);
newMap.addPiece(deck);
}
deck.setPosition(pt);
for (final GamePiece piece : deck.asList()) {
piece.setMap(newMap);
piece.setPosition(pt);
}

refreshable++;
break;
}
}
if (!deckFound) {
final String deckName = deck.getDeckName();

// Match with a DrawPile if possible. Take into account duplicate Deck names and moved
final DrawPile drawPile = findDrawPile(deck);
if (drawPile == null) {
deletable++;
decksToDelete.add(deck);
}
else {
foundDrawPiles.add(drawPile);

final String drawPileName = drawPile.getAttributeValueString(SetupStack.NAME);

if (!options.contains(SUPPRESS_INFO_REPORTS))
log(Resources.getString("GameRefresher.refreshing_deck", deckName, drawPileName));

// This refreshes the existing deck with all the up-to-date drawPile fields from the module
deck.removeListeners();
deck.myRefreshType(drawPile.getDeckType());
deck.addListeners();

// Make sure the deck is in the right place
final Point pt = drawPile.getPosition();
final Map newMap = drawPile.getMap();
if (newMap != map) {
map.removePiece(deck);
newMap.addPiece(deck);
}
deck.setPosition(pt);
for (final GamePiece piece : deck.asList()) {
piece.setMap(newMap);
piece.setPosition(pt);
}

refreshable++;
break;
}
}
}
}
Expand Down Expand Up @@ -611,6 +595,49 @@ else if (!decksToDelete.isEmpty()) {
}


/**
* Find the Vassal DrawPile that matches a given Deck.
* Deck Names may have been repeated (ill-advised). DrawPiles may have been added to or removed from the module
* or may exist on Boards that are not active in the current game
* If a DrawPile exists with a matching Deck Name on the same Map, return it.
* Otherwise, return the first DrawPile found with a matching name
*
* @param deck Deck to search for
* @return Matching DrawPile
*/
private DrawPile findDrawPile(Deck deck) {
DrawPile first = null;
DrawPile matching = null;
final String deckName = deck.getDeckName();
for (final DrawPile pile : getModuleDrawPiles()) {
if (deckName.equals(pile.getAttributeValueString(SetupStack.NAME))) {

// If drawPile is owned by a specific board, then we can only match it if that board is active in this game
final String pileBoardName = pile.getOwningBoardName();
if (pileBoardName != null) {
if (deck.getMap().getBoardByName(pileBoardName) == null) {
continue;
}

// If the drawPile is on a map that doesn't have its current owning board active, then we
// cannot match that drawPile.
if (pile.getMap().getBoardByName(pile.getOwningBoardName()) == null) {
continue;
}
}

if (first == null) {
first = pile;
if (deck.getMap().equals(pile.getMap())) {
Copy link

@jonathanrwatts jonathanrwatts Jul 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Am I misunderstanding something here, or should this if statement be outside of the (first == null) check, instead of within it? Is it not possible that you would find a match on the same map after finding a match on an earlier map? As written, it looks to me like it stops checking for a matching map once it's found any matching deck name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Jonathon, my cut and paste hatchet job got out of hand. That code makes no sense at all. I have committed a new version.

matching = pile;
break;
}
}
}
}
return matching != null ? matching : first;
}

/**
* Before refreshing, we need to go through every piece and commemorate all the Attachment trait relationships, since they contain direct references to other GamePieces, and all the references are
* about to be jumbled/invalidated when new updated versions of pieces are created.
Expand Down
Loading