Add damage and icon for bombing target selection #2339
Conversation
|
Looks great! That's incredible progress for less than 24 hours. I mean having the damage represented over the actual unit image would be ideal, but this covers off the requirements of the issue and adds value with the unit imagery in the window. Well done! |
| @@ -924,6 +929,37 @@ public Unit getStrategicBombingRaidTarget(final Territory territory, final Colle | |||
| return selected.get(); | |||
| } | |||
|
|
|||
| @SuppressWarnings("serial") | |||
There was a problem hiding this comment.
You should have eclipse generate a serialVersionUID for you instead of supressing this warning
| final boolean isSelected, final boolean cellHasFocus) { | ||
|
|
||
| setText(unit.toString() + ", damage=" + TripleAUnit.get(unit).getUnitDamage()); | ||
| final Optional<ImageIcon> icon = uiContext.getUnitImageFactory().getIcon(unit.getType(), unit.getOwner(), |
There was a problem hiding this comment.
very minor, you can write the optional + if statement in one statement:
Matches.unitHasTakenSomeBombingUnitDamage().match(unit), Matches.unitIsDisabled().match(unit)).ifPresent(icon -> setIcon(icon));```
There was a problem hiding this comment.
Gonna leave it on separate lines as I think its a little easier to read
| } | ||
| setBorder(new EmptyBorder(0, 0, 0, 10)); | ||
|
|
||
| if (isSelected) { |
There was a problem hiding this comment.
I don't fully understand why we are setting foreground/background colors. Likely not terribly important, though a comment here may help to capture that context.
| } | ||
|
|
||
| @Override | ||
| public Component getListCellRendererComponent(final JList<? extends Unit> list, final Unit unit, final int index, |
There was a problem hiding this comment.
A quick comment here might be good, summarize that we are building a dialog with the unit image next to name.
There was a problem hiding this comment.
Added comment to class
| @@ -924,6 +929,37 @@ public Unit getStrategicBombingRaidTarget(final Territory territory, final Colle | |||
| return selected.get(); | |||
| } | |||
|
|
|||
| @SuppressWarnings("serial") | |||
| class UnitRenderer extends JLabel implements ListCellRenderer<Unit> { | |||
There was a problem hiding this comment.
This class is only used within TripleAFrame and can thus be private. That would also allow you to give the constructor default accessibility.
|
As a final note Cernel did bring up a valuable point regarding the addition of a scroll bar in the event there are more targets than available space in the pop up window. Seems like a prudent addition given that if it happens it will show up here again as a bug. |
|
@Heppisorus Scroll bar should already work if the list gets longer than around 10. Comments should be addressed. |
Codecov Report
@@ Coverage Diff @@
## master #2339 +/- ##
============================================
- Coverage 20.07% 20.05% -0.03%
- Complexity 5710 5711 +1
============================================
Files 807 807
Lines 73956 74041 +85
Branches 12478 12507 +29
============================================
+ Hits 14845 14846 +1
- Misses 57053 57137 +84
Partials 2058 2058
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good, @ron-murhammer.
Not sure if you saw @Cernelius's comment on the forum about adding max damage to the end of the string (e.g. , damage=0/6). That would be useful, but I don't know if it's easy to do via TripleAUnit#getHowMuchDamageCanThisUnitTakeTotal(). Don't worry about it if you think it's out of scope.
|
@ssoloff I did. Unfortunately, the method requires territory which requires some effort to wire through so I decided to skip it for now. |
RoiEXLab
deleted the
Add_Damage_And_Icon_For_Bombing_Target_Selection
branch
Fixes #2333.
If someone wants to play around with trying to get the damage number to appear over the image, feel free to. This is at least a step in the right direction.
Old:
New:
