Add separate case for adding just territory name if null attachment

[ron-murhammer]

Addresses #3168

Recommend reviewing with ?w=1

Adds separate case for just adding the territory name if attachment is null. Otherwise on maps like NWO which have no territory attachment for SZ, the name doesn't get added to the panel.

After the fix:

[codecov-io]

Codecov Report

Merging #3171 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #3171      +/-   ##
============================================
+ Coverage     20.94%   20.95%   +0.01%     
- Complexity     5666     5668       +2     
============================================
  Files           824      824              
  Lines         72954    72956       +2     
  Branches      11880    11880              
============================================
+ Hits          15283    15291       +8     
+ Misses        55640    55634       -6     
  Partials       2031     2031

Impacted Files	Coverage Δ	Complexity Δ
...n/java/games/strategy/triplea/ui/TripleAFrame.java	0% <0%> (ø)	0 <0> (ø)	⬇️
.../main/java/games/strategy/net/ClientMessenger.java	36.29% <0%> (+0.74%)	14% <0%> (+1%)	⬆️
...rc/main/java/games/strategy/net/nio/NioWriter.java	70.33% <0%> (+3.38%)	20% <0%> (ø)	⬇️
...tegy/engine/message/unifiedmessenger/EndPoint.java	70.51% <0%> (+3.84%)	15% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3becff2...8236054. Read the comment docs.

[ssoloff]

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.

[ron-murhammer]

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.

[ssoloff]

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

[ssoloff]

Tested on NWO and confirmed.