Enhance Route Message With Background and Fuel Costs #3189

ron-murhammer · 2018-02-28T03:48:15Z

Addresses point 3 of https://forums.triplea-game.org/topic/558/fuel-enhancements

Functional Changes

Route movement message now has a background and displays fuel costs
Adjusted some colors and sizes as well to improve consistency and readability

Before

After

codecov-io · 2018-02-28T04:00:19Z

Codecov Report

Merging #3189 into master will decrease coverage by 2.21%.
The diff coverage is 58.2%.

@@             Coverage Diff              @@
##             master    #3189      +/-   ##
============================================
- Coverage     20.94%   18.73%   -2.22%     
- Complexity     5665     5732      +67     
============================================
  Files           823      866      +43     
  Lines         72913    82301    +9388     
  Branches      11871    13866    +1995     
============================================
+ Hits          15274    15420     +146     
- Misses        55606    64837    +9231     
- Partials       2033     2044      +11

Impacted Files	Coverage Δ	Complexity Δ
...s/strategy/triplea/image/ResourceImageFactory.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
.../main/java/games/strategy/triplea/ui/MapPanel.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...java/games/strategy/triplea/ui/MapRouteDrawer.java	`68.36% <62.9%> (-3.51%)`	`27 <5> (+2)`
...framework/startup/login/Md5CryptAuthenticator.java	`93.93% <0%> (-0.8%)`	`18% <0%> (+9%)`
...trategy/engine/lobby/client/ui/LobbyGamePanel.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...strategy/engine/lobby/client/login/LobbyLogin.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...games/strategy/engine/chat/PlayerChatRenderer.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...a/games/strategy/triplea/ui/menubar/LobbyMenu.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...es/strategy/engine/data/DynamicallyModifiable.java	`100% <0%> (ø)`	`3% <0%> (+2%)`	⬆️
...main/java/games/strategy/triplea/ui/StatPanel.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
... and 51 more

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 2d8280f...eb482c3. Read the comment docs.

ssoloff

Really looks good, @ron-murhammer.

Probably want a review from @RoiEXLab, too, since he's done a lot of work in MapRouteDrawer.

ssoloff · 2018-02-28T05:44:23Z

game-core/src/test/java/games/strategy/triplea/ui/RouteTest.java

@@ -83,7 +87,10 @@ public void testCorrectParameterHandling() {
    final Shape mockShape = mock(Shape.class);
    final Graphics2D mockGraphics = mock(Graphics2D.class);
    when(mockShape.contains(any(Point2D.class))).thenReturn(true);
-    routeDrawer.drawRoute(mockGraphics, dummyRouteDescription, "2");
+    final ResourceCollection mockResourceCollection = mock(ResourceCollection.class);
+    when(mockResourceCollection.getResourcesCopy()).thenReturn(new IntegerMap<Resource>());


When I delete the IntegerMap generic type (i.e. new IntegerMap<>()), it appears to compile successfully. Are you not seeing the same thing?

Yep, no idea why I put the type. Updated.

ssoloff · 2018-02-28T05:47:33Z

game-core/src/main/java/games/strategy/triplea/ui/MapPanel.java

@@ -89,6 +90,7 @@
  private final TileManager tileManager;
  private BufferedImage mouseShadowImage = null;
  private String movementLeftForCurrentUnits = "";
+  private ResourceCollection movementFuelCost;


This field is not initialized in the constructor, and only appears to be initialized via setMouseShadowUnits(). Should it be initialized to an empty ResourceCollection just in case paint() gets called before setMouseShadowUnits() and triggers an NPE?

Good point. It doesn't seem to ever happen but not sure I can prove it isn't possible. Updated.

ssoloff · 2018-02-28T05:57:15Z

game-core/src/main/java/games/strategy/triplea/ui/MapRouteDrawer.java

+    final String unitMovementLeft = (maxMovement == null || maxMovement.trim().length() == 0) ? "" : "/" + maxMovement;
+    final int imageWidth = findMovementLeftImageWidth(curMovement, unitMovementLeft, movementFuelCost);
+    final BufferedImage image = new BufferedImage(imageWidth, MESSAGE_HEIGHT, BufferedImage.TYPE_INT_ARGB);
+    final Graphics2D graphics = image.createGraphics();


We should probably be disposing graphics before returning from this method (reference). Probably best done with a try-finally block.

Good point. Seems that some of the existing graphics code does use dispose() and other places it doesn't. Something that could probably be improved. Other areas seem to just be calling dispose() at the end of the usage without a try-finally. I think that is probably ok given this is as important as say closing a stream. I'll stick with that pattern for now for consistency and we can always revisit adding dispose() to all graphics areas and whether try-finally would be better.

ssoloff · 2018-02-28T05:59:19Z

game-core/src/main/java/games/strategy/triplea/ui/MapRouteDrawer.java

+
+    // Create temp graphics to calculate necessary image width based on font sizes
+    final BufferedImage tempImage = new BufferedImage(1, 1, BufferedImage.TYPE_INT_ARGB);
+    final Graphics2D tempGraphics = tempImage.createGraphics();


Ditto on calling Graphics#dispose() before returning.

ssoloff · 2018-02-28T06:10:49Z

game-core/src/main/java/games/strategy/triplea/ui/MapRouteDrawer.java

+    graphics.drawRoundRect(0, 0, imageWidth - 2, rectHeight, rectHeight, rectHeight);
+
+    // Draw current movement
+    graphics.setColor(new Color(255, 120, 0));


Just noting that I find this foreground color a bit hard to distinguish between the background color. It looks like it's a medium orange on sand. Not a big deal, just wanted to mention it.

I adjusted a couple of the colors so that its more of a light gray background and a darker orange.

ssoloff · 2018-02-28T06:13:37Z

game-core/src/main/java/games/strategy/triplea/ui/MapRouteDrawer.java

+      final ResourceCollection movementFuelCost, final ResourceImageFactory resourceImageFactory) {
+
+    // Create and configure image
+    final String unitMovementLeft = (maxMovement == null || maxMovement.trim().length() == 0) ? "" : "/" + maxMovement;


I'm not sure if " / " is a better separator than "/" here. The latter seems a bit too cramped to me, but some may think the former is too spread out. 🤷‍♂️ Regardless, it's better than the old separator:

" /"

Not that important, but maxMovement should really be an int with a value of -1 or something for the null replacement if necessary

I tried that as well but felt without spaces was more coherent and tied the current moves to max moves better.

Yeah, some of the string parameters are a bit interesting but I decided I didn't want to start messing with a bunch of types as they are passed through down to this level.

RoiEXLab

Just some minor suggestions.

RoiEXLab · 2018-02-28T07:41:27Z

game-core/src/main/java/games/strategy/triplea/ui/MapPanel.java

@@ -568,7 +570,9 @@ public void paint(final Graphics g) {
      g2d.drawImage(mouseShadowImage, t, this);
    }
    if (routeDescription != null) {
-      routeDrawer.drawRoute(g2d, routeDescription, movementLeftForCurrentUnits);
+      routeDrawer.drawRoute(g2d, routeDescription, movementLeftForCurrentUnits, movementFuelCost,


It would be nice if drawRoute would continue to only accept 3 arguments, the third one being a wrapper object to wrap the last 3 objects.
This would keep the related stuff together and the parameter declarations relatively clean.

I think as a whole there could be some refactoring of parameters across a lot of the MapRouteDrawer and MapPanel. Both in terms of wrapper objects and types used. I decided not to dive into that for this PR. I think looking at even existing objects like RouteDescription which might be expanded to include these. But for now, I think that it would be better in a separate PR.

RoiEXLab · 2018-02-28T07:42:26Z

game-core/src/main/java/games/strategy/triplea/ui/MapRouteDrawer.java

+      final ResourceCollection movementFuelCost, final ResourceImageFactory resourceImageFactory) {
+
+    // Create and configure image
+    final String unitMovementLeft = (maxMovement == null || maxMovement.trim().length() == 0) ? "" : "/" + maxMovement;


Not that important, but maxMovement should really be an int with a value of -1 or something for the null replacement if necessary

ron-murhammer · 2018-02-28T18:49:53Z

@ssoloff @RoiEXLab Should have addressed all the comments. I'm open to thoughts on colors though don't want to end up in a rabbit hole of trying tons of different things as we can always adjust based on community feedback.

ssoloff

Yeah, new colors are a bit easier to read.

ron-murhammer · 2018-03-01T02:26:09Z

Gonna go ahead and merge this so testing can begin.

ron-murhammer added 4 commits February 27, 2018 21:42

Enhance Route Message With Background and Fuel Costs

3b5d31c

Update failing unit test

04aa595

Add final to constant

888b34b

Improve spacing

2fa1712

ssoloff reviewed Feb 28, 2018

View reviewed changes

RoiEXLab reviewed Feb 28, 2018

View reviewed changes

Adjust some colors and a few code improvements

eb482c3

ssoloff approved these changes Feb 28, 2018

View reviewed changes

ron-murhammer merged commit 12d55d7 into master Mar 1, 2018

RoiEXLab deleted the Enhance_Route_Message branch March 2, 2018 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Route Message With Background and Fuel Costs #3189

Enhance Route Message With Background and Fuel Costs #3189

ron-murhammer commented Feb 28, 2018 •

edited

Loading

codecov-io commented Feb 28, 2018 •

edited

Loading

ssoloff left a comment

ssoloff Feb 28, 2018

ron-murhammer Feb 28, 2018

ssoloff Feb 28, 2018

ron-murhammer Feb 28, 2018

ssoloff Feb 28, 2018

ron-murhammer Feb 28, 2018

ssoloff Feb 28, 2018

ron-murhammer Feb 28, 2018

ssoloff Feb 28, 2018

ron-murhammer Feb 28, 2018

ssoloff Feb 28, 2018

RoiEXLab Feb 28, 2018

ron-murhammer Feb 28, 2018 •

edited

Loading

RoiEXLab left a comment

RoiEXLab Feb 28, 2018

ron-murhammer Feb 28, 2018

RoiEXLab Feb 28, 2018

ron-murhammer commented Feb 28, 2018

ssoloff left a comment

ron-murhammer commented Mar 1, 2018

Enhance Route Message With Background and Fuel Costs #3189

Enhance Route Message With Background and Fuel Costs #3189

Conversation

ron-murhammer commented Feb 28, 2018 • edited Loading

codecov-io commented Feb 28, 2018 • edited Loading

Codecov Report

ssoloff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ron-murhammer Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

RoiEXLab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ron-murhammer commented Feb 28, 2018

ssoloff left a comment

Choose a reason for hiding this comment

ron-murhammer commented Mar 1, 2018

ron-murhammer commented Feb 28, 2018 •

edited

Loading

codecov-io commented Feb 28, 2018 •

edited

Loading

ron-murhammer Feb 28, 2018 •

edited

Loading