Skip to content

Commit

Permalink
Fix issue 12826 (UnitSeparator#categorize:213 - java.util.ConcurrentM…
Browse files Browse the repository at this point in the history 
…odificationException) (#12828)

* Fix issue 12826 (UnitSeparator#categorize:213 - java.util.ConcurrentModificationException)

PlacePanel.java
- new method updateUnitsInUnitsToPlacePanel that ensures copying the unit collection (existed already in method updateStep, but not in gameDataChanged or updateUnits)
- Cleanup: extract new methods from declaration of variable placeMapSelectionListener which are getUnitsToPlace, getScrollPaneFromChooser, getPreferredHeight and getPreferredWidth

* PlayerUnitsPanel ToDo done via redraw

Replace
invalidate(); validate(); revalidate();getParent().invalidate();
with SwingComponents.redraw(this);
  • Loading branch information
@frigoref
frigoref authored Aug 9, 2024
1 parent 8b4bbda commit 5e511b4
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 98 deletions.
7 changes: 2 additions & 5 deletions ...-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/PlayerUnitsPanel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import lombok.Getter;
import org.triplea.java.collections.CollectionUtils;
import org.triplea.java.collections.IntegerMap;
import org.triplea.swing.SwingComponents;

/** Panel showing full list of units for a player in a given battle simulation. */
public class PlayerUnitsPanel extends JPanel {
Expand Down Expand Up @@ -102,11 +103,7 @@ public void init(
}
}

// TODO: probably do not need to do this much revalidation.
invalidate();
validate();
revalidate();
getParent().invalidate();
SwingComponents.redraw(this);
}

/**
Expand Down
200 changes: 107 additions & 93 deletions game-app/game-headed/src/main/java/games/strategy/triplea/ui/PlacePanel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.function.Predicate;
Expand All @@ -34,21 +35,71 @@
import javax.swing.JPanel;
import javax.swing.JScrollPane;
import javax.swing.SwingUtilities;
import org.jetbrains.annotations.NotNull;
import org.triplea.java.collections.CollectionUtils;
import org.triplea.swing.SwingComponents;

class PlacePanel extends AbstractMovePanel implements GameDataChangeListener {
private static final long serialVersionUID = -4411301492537704785L;
private final JLabel leftToPlaceLabel = createIndentedLabel();
private PlaceData placeData;
private transient PlaceData placeData;

private final SimpleUnitPanel unitsToPlacePanel;

private GamePlayer lastPlayer;
private boolean postProductionStep;

private final MapSelectionListener placeMapSelectionListener =
private final transient MapSelectionListener placeMapSelectionListener =
new DefaultMapSelectionListener() {

private PlaceableUnits getUnitsToPlace(final Territory territory) {
try (GameData.Unlocker ignored = getData().acquireReadLock()) {
// not our territory
if (!territory.isWater() && !territory.isOwnedBy(getCurrentPlayer())) {
if (GameStepPropertiesHelper.isBid(getData())) {
final PlayerAttachment pa = PlayerAttachment.get(territory.getOwner());
if ((pa == null || !pa.getGiveUnitControl().contains(getCurrentPlayer()))
&& !territory.anyUnitsMatch(Matches.unitIsOwnedBy(getCurrentPlayer()))) {
return new PlaceableUnits();
}
} else {
return new PlaceableUnits();
}
}
// get the units that can be placed on this territory.
Collection<Unit> units = getCurrentPlayer().getUnits();
if (territory.isWater()) {
GameProperties properties = getData().getProperties();
if (!(Properties.getProduceFightersOnCarriers(properties)
|| Properties.getProduceNewFightersOnOldCarriers(properties)
|| Properties.getLhtrCarrierProductionRules(properties)
|| GameStepPropertiesHelper.isBid(getData()))) {
units = CollectionUtils.getMatches(units, Matches.unitIsSea());
} else {
final Predicate<Unit> unitIsSeaOrCanLandOnCarrier =
Matches.unitIsSea().or(Matches.unitCanLandOnCarrier());
units = CollectionUtils.getMatches(units, unitIsSeaOrCanLandOnCarrier);
}
} else {
units = CollectionUtils.getMatches(units, Matches.unitIsNotSea());
}
if (units.isEmpty()) {
return new PlaceableUnits();
}
final IAbstractPlaceDelegate placeDel =
(IAbstractPlaceDelegate) getPlayerBridge().getRemoteDelegate();
final PlaceableUnits production = placeDel.getPlaceableUnits(units, territory);
if (production.isError()) {
JOptionPane.showMessageDialog(
getTopLevelAncestor(),
production.getErrorMessage() + "\n\n",
"Cannot produce units",
JOptionPane.INFORMATION_MESSAGE);
}
return production;
}
}

@Override
public void territorySelected(final Territory territory, final MouseDetails e) {
if (!isActive() || (e.getButton() != MouseEvent.BUTTON1)) {
Expand All @@ -66,21 +117,7 @@ public void territorySelected(final Territory territory, final MouseDetails e) {
if (maxUnits >= 0) {
chooser.setMaxAndShowMaxButton(maxUnits);
}
final Dimension screenResolution = Toolkit.getDefaultToolkit().getScreenSize();
final int availHeight = screenResolution.height - 120;
final int availWidth = screenResolution.width - 40;
final JScrollPane scroll = new JScrollPane(chooser);
scroll.setBorder(BorderFactory.createEmptyBorder());
scroll.setPreferredSize(
new Dimension(
(scroll.getPreferredSize().width > availWidth
? availWidth
: (scroll.getPreferredSize().width
+ (scroll.getPreferredSize().height > availHeight ? 20 : 0))),
(scroll.getPreferredSize().height > availHeight
? availHeight
: (scroll.getPreferredSize().height
+ (scroll.getPreferredSize().width > availWidth ? 26 : 0)))));
final JScrollPane scroll = getScrollPaneFromChooser(chooser);
final int option =
JOptionPane.showOptionDialog(
getTopLevelAncestor(),
Expand All @@ -92,15 +129,43 @@ public void territorySelected(final Territory territory, final MouseDetails e) {
null,
null);
if (option == JOptionPane.OK_OPTION) {
final Collection<Unit> choosen = chooser.getSelected();
placeData = new PlaceData(choosen, territory);
final Collection<Unit> selectedUnits = chooser.getSelected();
placeData = new PlaceData(selectedUnits, territory);
updateUnits();
if (choosen.containsAll(units)) {
if (selectedUnits.containsAll(units)) {
leftToPlaceLabel.setText("");
}
release();
}
}

@NotNull
private JScrollPane getScrollPaneFromChooser(final UnitChooser chooser) {
final Dimension screenResolution = Toolkit.getDefaultToolkit().getScreenSize();
final int availHeight = screenResolution.height - 120;
final int availWidth = screenResolution.width - 40;
final JScrollPane scroll = new JScrollPane(chooser);
scroll.setBorder(BorderFactory.createEmptyBorder());
scroll.setPreferredSize(
new Dimension(
(scroll.getPreferredSize().width > availWidth
? availWidth
: getPreferredWith(scroll, availHeight)),
(scroll.getPreferredSize().height > availHeight
? availHeight
: getPreferredHeight(scroll, availWidth))));
return scroll;
}

private int getPreferredHeight(final JScrollPane scroll, final int availWidth) {
return scroll.getPreferredSize().height
+ (scroll.getPreferredSize().width > availWidth ? 26 : 0);
}

private int getPreferredWith(final JScrollPane scroll, final int availHeight) {
return scroll.getPreferredSize().width
+ (scroll.getPreferredSize().height > availHeight ? 20 : 0);
}
};

PlacePanel(final GameData data, final MapPanel map, final TripleAFrame frame) {
Expand Down Expand Up @@ -147,42 +212,39 @@ private void updateStep() {
}

if (showUnitsToPlace) {
// Small hack: copy the unit list before passing it to a new thread.
// This is to prevent ConcurrentModification. If the 'unitsToPlace' list is modified
// later in this thread, before "SwingUtilities.invokeLater" can execute and complete,
// then we will get a ConcurrentModification exception.
// Ideally we would not modify the 'unitsToPlace' collection again except when
// the swing thread signals that the user has taken action.. Short of that, we create a copy
// here.
final Collection<Unit> unitsToPlaceCopy = new ArrayList<>(unitsToPlace);
SwingUtilities.invokeLater(
() -> {
unitsToPlacePanel.setUnits(unitsToPlaceCopy);
SwingComponents.redraw(unitsToPlacePanel);
});
updateUnitsInUnitsToPlacePanel(unitsToPlace);
} else {
SwingUtilities.invokeLater(unitsToPlacePanel::removeAll);
}
}

private void updateUnitsInUnitsToPlacePanel(final Collection<Unit> newUnitsToPlace) {
// Small hack: copy the unit list before passing it to a new thread.
// This is to prevent ConcurrentModification. If the 'unitsToPlace' list is modified
// later in this thread, before "SwingUtilities.invokeLater" can execute and complete,
// then we will get a ConcurrentModification exception.
// Ideally we would not modify the 'unitsToPlace' collection again except when
// the swing thread signals that the user has taken action.
// Short of that, we create a copy here.
final Collection<Unit> unitsToPlaceCopy =
Collections.unmodifiableCollection(new ArrayList<>(newUnitsToPlace));
SwingUtilities.invokeLater(
() -> {
unitsToPlacePanel.setUnits(unitsToPlaceCopy);
SwingComponents.redraw(unitsToPlacePanel);
});
}

@Override
public void gameDataChanged(final Change change) {
final Collection<Unit> unitsToPlace;
final GameData data = getData();
try (GameData.Unlocker ignored = data.acquireReadLock()) {
final GamePlayer player = data.getSequence().getStep().getPlayerId();
if (player == null) {
return;
if (player != null) {
final Collection<Unit> unitsToPlace = player.getUnits();
updateUnitsInUnitsToPlacePanel(unitsToPlace);
}
unitsToPlace = player.getUnits();
}

SwingUtilities.invokeLater(
() -> {
unitsToPlacePanel.setUnits(unitsToPlace);
unitsToPlacePanel.revalidate();
unitsToPlacePanel.repaint();
});
}

@Override
Expand Down Expand Up @@ -210,54 +272,6 @@ PlaceData waitForPlace(final boolean bid, final PlayerBridge playerBridge) {
return placeData;
}

private PlaceableUnits getUnitsToPlace(final Territory territory) {
try (GameData.Unlocker ignored = getData().acquireReadLock()) {
// not our territory
if (!territory.isWater() && !territory.isOwnedBy(getCurrentPlayer())) {
if (GameStepPropertiesHelper.isBid(getData())) {
final PlayerAttachment pa = PlayerAttachment.get(territory.getOwner());
if ((pa == null || !pa.getGiveUnitControl().contains(getCurrentPlayer()))
&& !territory.anyUnitsMatch(Matches.unitIsOwnedBy(getCurrentPlayer()))) {
return new PlaceableUnits();
}
} else {
return new PlaceableUnits();
}
}
// get the units that can be placed on this territory.
Collection<Unit> units = getCurrentPlayer().getUnits();
if (territory.isWater()) {
GameProperties properties = getData().getProperties();
if (!(Properties.getProduceFightersOnCarriers(properties)
|| Properties.getProduceNewFightersOnOldCarriers(properties)
|| Properties.getLhtrCarrierProductionRules(properties)
|| GameStepPropertiesHelper.isBid(getData()))) {
units = CollectionUtils.getMatches(units, Matches.unitIsSea());
} else {
final Predicate<Unit> unitIsSeaOrCanLandOnCarrier =
Matches.unitIsSea().or(Matches.unitCanLandOnCarrier());
units = CollectionUtils.getMatches(units, unitIsSeaOrCanLandOnCarrier);
}
} else {
units = CollectionUtils.getMatches(units, Matches.unitIsNotSea());
}
if (units.isEmpty()) {
return new PlaceableUnits();
}
final IAbstractPlaceDelegate placeDel =
(IAbstractPlaceDelegate) getPlayerBridge().getRemoteDelegate();
final PlaceableUnits production = placeDel.getPlaceableUnits(units, territory);
if (production.isError()) {
JOptionPane.showMessageDialog(
getTopLevelAncestor(),
production.getErrorMessage() + "\n\n",
"Cannot produce units",
JOptionPane.INFORMATION_MESSAGE);
}
return production;
}
}

@Override
public String toString() {
return "PlacePanel";
Expand Down Expand Up @@ -315,6 +329,6 @@ protected final List<Component> getAdditionalButtons() {
}

private void updateUnits() {
unitsToPlacePanel.setUnits(getCurrentPlayer().getUnits());
updateUnitsInUnitsToPlacePanel(getCurrentPlayer().getUnits());
}
}

0 comments on commit 5e511b4

Please sign in to comment.