Camops Paid Recrutiment Removal #5007
Conversation
|
@Algebro7 do you have review permissions? If so, I'll leave this for you to review as it's your stomping ground. If not, when you're available can you take a look and just reply whether you approve the changes or not. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5007 +/- ##
============================================
- Coverage 10.47% 10.45% -0.03%
+ Complexity 6029 6014 -15
============================================
Files 950 950
Lines 133397 133408 +11
Branches 19384 19386 +2
============================================
- Hits 13974 13948 -26
- Misses 118076 118111 +35
- Partials 1347 1349 +2 ☔ View full report in Codecov by Sentry. |
|
Not sure but I'll take a look ASAP! |
| @@ -229,6 +232,10 @@ public Component getListCellRendererComponent(JList<?> list, Object value, int i | |||
| } else { | |||
| radioNormalRoll.setSelected(true); | |||
| } | |||
| } else { | |||
| // Turn off paid recruitment if it's not available | |||
There was a problem hiding this comment.
I think you mentioned this in the PR discussion, but right now when the user switches the market to CamOps in Campaign Options while having paid recruitment rolls enabled and advances days, paid recruitment rolls still happen until you open the market dialog. I think this will lead to a confusing user experience, so we probably need to update the campaign options to disable paid recruitment rolls whenever the options are saved with Camops selected.
I know I said to try and avoid messing with Campaign Options since they're being refactored by @IllianiCBT , but IMO we need to address this as part of this PR. You'd want to look around in https://github.com/MegaMek/mekhq/blob/master/MekHQ/src/mekhq/gui/panes/CampaignOptionsPane.java#L9480 for this, which is executed when the campaign options are saved.
There was a problem hiding this comment.
Make the necessary changes, I'll deal with the conflicts :)
There was a problem hiding this comment.
Ok, I've added something to diable paid recruitment when the campaign options are processed, but that doesn't handle the case of someone loading a game with paid recruitment active. Though with #5006 I'm not entirely sure that PersonnelMarket.java is loading paid recruitment FROM the save file?
There was a problem hiding this comment.
I should have read this message before putting in my review :D
|
I've approved this PR, so it should be good to merge. I am a little concerned that we're not sanitizing saves as users load into the client. However, so long as #5006 is causing saves to load in with paid recruitment disabled, I'm not too bothered. |
This fixes the real issue behind #4986: that paid recruitment is even available when camops markets are selected.
It adds camops markets as a reason not to show the paid recruitment controls. It also makes sure paid recruitment is turned if the controls are turned off, though this won't come into effect until the window is opened.
Added bonus QoL: Since camops markets only have one person at a time, select the first person in the list automatically when opened. One less click and it doesn't really hurt other market types either.