Clear designations when mothballing/mass mothballing#9010
Conversation
…othballing and mass mothballing.
IllianiBird
left a comment
There was a problem hiding this comment.
A couple points of feedback, but at its core this looks good. Great work :)
| int result = JOptionPane.showConfirmDialog( | ||
| gui.getFrame(), | ||
| "Also clear all assignments and designations?", | ||
| "Mothball Unit", | ||
| JOptionPane.YES_NO_OPTION |
There was a problem hiding this comment.
Let's use an ImmersiveDialog for this, rather than JOptionPane. Immersive dialog is a bit more steps to set up, but looks better and avoids some annoyances (like players being able to accidentally confirm/skip dialogs).
ImmersiveDialog also avoids a few annoying bugs that exist in JOptionPane.
| "Also clear all assignments and designations?", | ||
| "Mothball Unit", |
There was a problem hiding this comment.
All player-facing text must be in a resource bundle for i18n purposes.
| gbc.weighty = 0.1; | ||
| gbc.anchor = GridBagConstraints.CENTER; | ||
| clearDesignationsCheckbox = new JCheckBox(); | ||
| clearDesignationsCheckbox.setText("Clear all assignments and designations?"); |
There was a problem hiding this comment.
See earlier comment about player-facing text.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9010 +/- ##
============================================
- Coverage 15.23% 15.22% -0.01%
+ Complexity 9305 9301 -4
============================================
Files 1304 1304
Lines 172518 172545 +27
Branches 26063 26067 +4
============================================
- Hits 26277 26267 -10
- Misses 143198 143229 +31
- Partials 3043 3049 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Converted to draft pending @theyoungster21 addressing the feedback. |
…l replaced the JOption Pane for single mothballing with an ImmersiveDialog
|
Changes have been made, hopefully it's good! |
IllianiBird
left a comment
There was a problem hiding this comment.
One comment regarding the ImmersiveDialog. What you've got is good, I think we can do a tiny bit more polish though
In future, if I forget to do a follow-up review feel free to @ me on Discord. We all know of my famous good memory by now ;)
| MekHQ.triggerEvent(new UnitChangedEvent(selectedUnit)); | ||
| if (tech != null) { | ||
| if ((!selectedUnit.getActiveCrew().isEmpty()) || (selectedUnit.getCampaign().getFormationFor(selectedUnit)) != null) { | ||
| ImmersiveDialogCore clearAssignments = new ImmersiveDialogCore(selectedUnit.getCampaign(), |
There was a problem hiding this comment.
Take a look at, I think it's called ImersiveDialogSimple. That will fit your need better.
Also, let's add a speaker (as, the senior tech, or whoever was selected) and some IC text.
Generally the format for an ID is you want a speaker, a little bit of IC text and the OOC stuff at the bottom
There was a problem hiding this comment.
Don't worry! I'm just new to actually using github to work so I wasn't sure if I needed to push a notification or something. I'll take a look at the simple version and write some simple IC text to make it all "in universe-y" tonight!
|
Technically I get both an email from GitHub and we get the little notification on the Discord, so I don't have many excuses. :D |
…ng a few more immersive features, i.e. IC dialogue and the portrait of the tech doing the work
IllianiBird
left a comment
There was a problem hiding this comment.
Further polish, as discussed on Discord
| tempCrew.fillWithTempCrew=Fill with temp crew | ||
| tempCrew.removeTempCrew=Remove temp crew | ||
| mothballUnit.clearDesignationsDialog.text=Also clear all assignments and designations? | ||
| mothballUnit.clearDesignationsICMessage.text=Hey boss, did you want us to keep the personnel assigned to this unit on\ |
There was a problem hiding this comment.
Replace the hardcoded 'boss' with {0}. This is a placeholder key, it's going to do something cool for us.
| getFormattedTextAt(RESOURCE_BUNDLE, "mothballUnit.clearDesignationsCancelButton.text"), | ||
| getFormattedTextAt(RESOURCE_BUNDLE, "mothballUnit.clearDesignationsConfirmButton.text")), | ||
| getFormattedTextAt(RESOURCE_BUNDLE, "mothballUnit.clearDesignationsOOCMessage.text"), |
There was a problem hiding this comment.
We're not doing any formatting here, so we can actually do....
getTextAt(RESOURCE_BUNDLE, blah blah blah
However, (and this is true for all instances of the text getter you're using, you don't need to pass in a RESOURCE_BUNDLE at all.
If you use...
getText(blah blah blah) or getFormattedText(blah blah blah it actually defaults to GUI.properties
| ImmersiveDialogSimple clearAssignments = new ImmersiveDialogSimple(selectedUnit.getCampaign(), | ||
| tech, | ||
| null, | ||
| getFormattedTextAt(RESOURCE_BUNDLE, "mothballUnit.clearDesignationsICMessage.text"), |
There was a problem hiding this comment.
For this, we wanna add in the text that will replace the placeholder key we added earlier, change this line to...
getFormattedText(RESOURCE_BUNDLE, "mothballUnit.clearDesignationsICMessage.text", getCampaign().getCommanderAddress())
What that will do, is dynamically change the placeholder ({0}) with the return value of getCommanderAddress which changes based on commander gender and campaign type. For example, female pirate commanders get called "Bossma'am"
| gbc.weighty = 0.1; | ||
| gbc.anchor = GridBagConstraints.CENTER; | ||
| clearDesignationsCheckbox = new JCheckBox(); | ||
| clearDesignationsCheckbox.setText(getFormattedTextAt(RESOURCE_BUNDLE,"mothballUnit.clearDesignationsDialog.text")); |
There was a problem hiding this comment.
So because we're not passing in any formatting, just use getTextAt, instead
…se campaign.getCommanderAddress() instead of hardcoding the dialogue. As well, some slightly changed formatting from getFormattedTextAt() to getText()
|
Good work, we'll get this merged once the tests have finished :) |
This PR addresses this RFE: #7833
It slightly changes how both individual mothballing and mass mothballing are handled.
For individual mothballing, it adds a second confirmation box that allows the user to choose whether to strip all designations and crew assignments from a unit as it is being mothballed. It does not fire if the unit has no crew assigned and is not assigned to any formations.
For mass mothballing it adds a checkbox that only appears while mass mothballing (not while mass activating) which, when checked, removes all formation and crew assignments.
The initial idea was to use a checkbox for both so as to keep consistency of implementation, but I struggled to figure out how to add it to the single mothballing dialog, as it appears to handle many different pop-ups and I didn't want to risk breaking things/expanding the scope too much.