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

Add separate case for adding just territory name if null attachment #3171

Merged
merged 1 commit into from
Feb 27, 2018
Merged

Add separate case for adding just territory name if null attachment #3171

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
110 changes: 58 additions & 52 deletions game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,59 +602,65 @@ public void mouseEntered(final Territory territory) {
void refresh() {
territoryInfo.removeAll();

final StringBuilder buf = new StringBuilder();
buf.append(territoryLastEntered == null ? "none" : territoryLastEntered.getName());
if (territoryLastEntered != null) {
final TerritoryAttachment ta = TerritoryAttachment.get(territoryLastEntered);
if (ta != null) {
final List<TerritoryEffect> territoryEffects = ta.getTerritoryEffect();
int count = 0;
final StringBuilder territoryEffectText = new StringBuilder();
for (final TerritoryEffect territoryEffect : territoryEffects) {
try {
final JLabel territoryEffectLabel = new JLabel();
territoryEffectLabel.setIcon(uiContext.getTerritoryEffectImageFactory().getIcon(territoryEffect, false));
territoryEffectLabel.setBorder(BorderFactory.createEmptyBorder(0, 0, 0, 10));
territoryInfo.add(territoryEffectLabel,
new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST, GridBagConstraints.NONE,
new Insets(0, 0, 0, 0), 0, 0));
} catch (final IllegalStateException e) {
territoryEffectText.append(territoryEffect.getName() + ", ");
}
}
message.setText((territoryLastEntered == null) ? "none" : territoryLastEntered.getName());

// If territory is null or doesn't have an attachment then just display the name or "none"
if (territoryLastEntered == null || TerritoryAttachment.get(territoryLastEntered) == null) {
territoryInfo.add(message, new GridBagConstraints(0, 0, 1, 1, 0, 0, GridBagConstraints.WEST,
GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0));
territoryInfo.revalidate();
territoryInfo.repaint();
return;
Copy link
Member

Choose a reason for hiding this comment

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

Not requesting a change here, but since all these lines have been re-indented anyway, would it be cleaner to have an if-else (with a very large else block) so that the repaint logic is isolated at the end of the method?

Basically, with whitespace changes ignored:

diff --git a/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java b/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
index 15b2d8c57..f4ee22dae 100644
--- a/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
+++ b/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
@@ -608,11 +608,7 @@ public class TripleAFrame extends MainGameFrame {
       if (territoryLastEntered == null || TerritoryAttachment.get(territoryLastEntered) == null) {
         territoryInfo.add(message, new GridBagConstraints(0, 0, 1, 1, 0, 0, GridBagConstraints.WEST,
             GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0));
-        territoryInfo.revalidate();
-        territoryInfo.repaint();
-        return;
-      }
-
+      } else {
         // Display territory effects, territory name, and resources
         final TerritoryAttachment ta = TerritoryAttachment.get(territoryLastEntered);
         final List<TerritoryEffect> territoryEffects = ta.getTerritoryEffect();
@@ -660,6 +656,7 @@ public class TripleAFrame extends MainGameFrame {
               new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST, GridBagConstraints.NONE,
                   new Insets(0, 0, 0, 0), 0, 0));
         }
+      }
       territoryInfo.revalidate();
       territoryInfo.repaint();
     }

Alternatively, a try-finally might work, as well:

diff --git a/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java b/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
index 15b2d8c57..a6155755a 100644
--- a/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
+++ b/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
@@ -601,15 +601,13 @@ public class TripleAFrame extends MainGameFrame {
 
     void refresh() {
       territoryInfo.removeAll();
-
+      try {
         message.setText((territoryLastEntered == null) ? "none" : territoryLastEntered.getName());
 
         // If territory is null or doesn't have an attachment then just display the name or "none"
         if (territoryLastEntered == null || TerritoryAttachment.get(territoryLastEntered) == null) {
           territoryInfo.add(message, new GridBagConstraints(0, 0, 1, 1, 0, 0, GridBagConstraints.WEST,
               GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0));
-        territoryInfo.revalidate();
-        territoryInfo.repaint();
           return;
         }
 
@@ -660,9 +658,11 @@ public class TripleAFrame extends MainGameFrame {
               new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST, GridBagConstraints.NONE,
                   new Insets(0, 0, 0, 0), 0, 0));
         }
+      } finally {
         territoryInfo.revalidate();
         territoryInfo.repaint();
       }
+    }
   };
 
   void clearStatusMessage() {

Normally, I'm a fan of guard clauses to short-circuit a method, but something about the duplication of the revalidate/repaint (and because the two occurrences are so far apart) makes me think we're going to miss that in a future change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought about both those options as well and decided to go with duplicating those calls. Just felt the second block was so large that using else or finally made it hard to know anything else happened for the first block (gets lost IMO). Could probably argue to break this method up some but didn't want to clutter up a relatively small bug fix.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I can see how the distance can go both ways.

}

territoryInfo.add(message, new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST,
GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0));
// Display territory effects, territory name, and resources
final TerritoryAttachment ta = TerritoryAttachment.get(territoryLastEntered);
final List<TerritoryEffect> territoryEffects = ta.getTerritoryEffect();
int count = 0;
final StringBuilder territoryEffectText = new StringBuilder();
for (final TerritoryEffect territoryEffect : territoryEffects) {
try {
final JLabel territoryEffectLabel = new JLabel();
territoryEffectLabel.setIcon(uiContext.getTerritoryEffectImageFactory().getIcon(territoryEffect, false));
territoryEffectLabel.setBorder(BorderFactory.createEmptyBorder(0, 0, 0, 10));
territoryInfo.add(territoryEffectLabel,
new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST, GridBagConstraints.NONE,
new Insets(0, 0, 0, 0), 0, 0));
} catch (final IllegalStateException e) {
territoryEffectText.append(territoryEffect.getName() + ", ");
}
}

if (territoryEffectText.length() > 0) {
territoryEffectText.setLength(territoryEffectText.length() - 2);
final JLabel territoryEffectTextLabel = new JLabel();
territoryEffectTextLabel.setText(" (" + territoryEffectText + ")");
territoryInfo.add(territoryEffectTextLabel,
new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST,
GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0));
}
territoryInfo.add(message, new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST,
GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0));

final int production = ta.getProduction();
final ResourceCollection resourceCollection = ta.getResources();
final IntegerMap<Resource> resources = new IntegerMap<>();
if (production > 0) {
resources.add(new Resource(Constants.PUS, data), production);
}
if (resourceCollection != null) {
resources.add(resourceCollection.getResourcesCopy());
}
for (final Resource resource : resources.keySet()) {
final JLabel resourceLabel =
uiContext.getResourceImageFactory().getLabel(resource, resources);
resourceLabel.setBorder(BorderFactory.createEmptyBorder(0, 10, 0, 0));
territoryInfo.add(resourceLabel,
new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST, GridBagConstraints.NONE,
new Insets(0, 0, 0, 0), 0, 0));
}
}
if (territoryEffectText.length() > 0) {
territoryEffectText.setLength(territoryEffectText.length() - 2);
final JLabel territoryEffectTextLabel = new JLabel();
territoryEffectTextLabel.setText(" (" + territoryEffectText + ")");
territoryInfo.add(territoryEffectTextLabel,
new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST,
GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0));
}

final int production = ta.getProduction();
final ResourceCollection resourceCollection = ta.getResources();
final IntegerMap<Resource> resources = new IntegerMap<>();
if (production > 0) {
resources.add(new Resource(Constants.PUS, data), production);
}
if (resourceCollection != null) {
resources.add(resourceCollection.getResourcesCopy());
}
message.setText(buf.toString());
for (final Resource resource : resources.keySet()) {
final JLabel resourceLabel =
uiContext.getResourceImageFactory().getLabel(resource, resources);
resourceLabel.setBorder(BorderFactory.createEmptyBorder(0, 10, 0, 0));
territoryInfo.add(resourceLabel,
new GridBagConstraints(count++, 0, 1, 1, 0, 0, GridBagConstraints.WEST, GridBagConstraints.NONE,
new Insets(0, 0, 0, 0), 0, 0));
}
territoryInfo.revalidate();
territoryInfo.repaint();
}
};
Expand Down Expand Up @@ -856,9 +862,9 @@ public boolean getOkToLetUnitsDie(final Collection<Territory> unitsCantFight, fi
final String[] options = {cancel, ok};
this.mapPanel.centerOn(unitsCantFight.iterator().next());
final int choice = EventThreadJOptionPane.showOptionDialog(this, buf.toString(),
"Units cannot fight", JOptionPane.YES_NO_OPTION,
JOptionPane.WARNING_MESSAGE, null, options, cancel,
getUiContext().getCountDownLatchHandler());
"Units cannot fight", JOptionPane.YES_NO_OPTION,
JOptionPane.WARNING_MESSAGE, null, options, cancel,
getUiContext().getCountDownLatchHandler());
return choice == 1;
}

Expand Down