[DONE][Suggestion] Manual Promotions

[Meridian]

though if you demote to rookie, you cannot promote back up again.

I like this.

• To what degree is feedback and/or negative feedback desirable? IE, if you select a rank that you cannot promote to, what should happen?

Nothing happens.
Just like in the video.
(positive feedback is that the rank icon changes, negative feedback is that the dialog window doesn't close)

• Should ranks that are ineligible for promotion even show up on the list?

Yes, I think it looks nicer than a mostly empty list.

• I do not think any feedback is necessary when you successfully promote someone, the result is obvious, but I'm open to other opinions.

I agree.

• Obviously it should have an on-off switch. But it occurs to me that you might want automatic promotions on and manual promotions on. Since if demotion is possible, you can just change who is promoted anyways. So it wouldn't just be a binary switch of automatic promotions off, manual promotions on, but two switches. Thoughts about this?"

Sounds OK to me.

What I need is a setting, which provides vanilla compatibility (i.e. the off switch).
Other options are open for discussion.

• It also occurs to me though, that if there is a on/off switch for automatic promotions, it would make sense for that option to be combined with field promotions (which implies automatic promotions), as that option doesn't make sense without automatic promotions being on. However, actually implementing a 3 value switch is somewhat more complicated, and it would be somewhat difficult to do in a backwards compatible manner.

Please leave the field promotions option as it is on the GUI now (and make it apply only when relevant).

Yeah, I am all for that, and plan to look into implementing something like that after I finish this. But the two features I feel are independent.

Yes, one feature = one PR.

"One feature = many PRs" is unwanted.
"Many features = one PR" is also unwanted.

[MaxMahem]

Okay my version is more or less feature complete I think. Pull request sent. Here is a demo

.

Some minor style changes from the clip posted before. Ranks now show in the color2 color (configurable) if they aren't eligible for promotion. I removed the notation of if a rank was a promotion or a demotion, since it's kind of obvious and the ranks on the screen anyways. Quicksearch works, and you can middle-click on a rank to bring up the ufopedia article, if any.

I blather on more in the pull request, but the demo above shows how everything works.

The only other major design decision I made was to enforce rank limits in both promotion and demotion. Currently, you can still demote down to rookie, but then you cannot promote back up again afterward. I could go either way on this decision. I can imagine some people wanting to do it, but the behavior is somewhat surprising, I think.

[Yankes]

Could you check how it will work for mods like xpiratez? I recall that it have more custom ranks than normal game.

[MaxMahem]

Could you check how it will work for mods like xpiratez? I recall that it have more custom ranks than normal game.

I've tested it with XPZ (that's the main mod I'm playing), no issues. Custom rank titles are reflected and perform normally. In addition, soldier types with limited sets of rank titles (like named dogs) have a limited set of promotion opportunities, as expected.

[Yankes]

Ok. Your code look clean and I do not see anything against adding it to OXCE.

[Meridian]

I will review later and do some cosmetic changes, as usual.

[MaxMahem]

Hey, sorry to ask a million questions, just want to get a better picture of what ya'll want in patches going forward. Like, when making this feature the dialog is basically a carbon copy of the existing Armor choice dialog. So it's an obvious candidate for refactoring both of them (and possibly others) into a shared "listItem" class or something, with the differences handled via composition or inheritance. I refrained from doing so, because I wanted to be minimally invasive, but would such refactoring be desirable? Or would you prefer I leave well enough alone for the most part?

[Meridian]

So it's an obvious candidate for refactoring both of them (and possibly others) into a shared "listItem" class or something, with the differences handled via composition or inheritance.

It's not "obvious"

I refrained from doing so, because I wanted to be minimally invasive, but would such refactoring be desirable?

From my POV, no.

Or would you prefer I leave well enough alone for the most part?

Yes.

[Yankes]

I think current version is fine, if we would have more screens like this we could consider refactoring it.
I would more worry about game logic duplication that some screens that once created stay same for very long time.
This is because we constantly change game logic and each change need to propagate to multiple places, and every duplication make it harder to do.
In case of screens, most changes only affect one screen at time.

[Meridian]

I've added a review on github.

Some things didn't work at all... for example when working with custom rank strings.
Mostly copypaste errors, off-by-one, etc.

TFTD support was missing.
en-GB was missing and translations were not in correct places.

The rest was not so important, but even cosmetic issues can pile up

I will make the changes, and merge manually.
Probably tomorrow.

[MaxMahem]

Thanks for the detailed review. Sorry for all the typos, sperling is not my strong suit. I'll push a patch in a bit that should resolve all the ones marked resolved. The few remaining are either ones I didn't know the right fix for (I don't have TFTD installed right now to test) or wasn't sure your preferred resolution of.

[Meridian]

Here's the explanation for the one that was confusing you.
I should have written a better comment already the first time.

[Meridian]

Merged.

Edit: build failed, because of case-sensitive includes, fix here: https://github.com/MeridianOXC/OpenXcom/commit/35971be2346914468d6c2829b4147568bf813e7b

[Ethereal]

Is it possible to disable restrictions for Rank 5 in UFO using this function?

Those who love vanilla, they will not activate this. In other cases, this restriction looks illogical for a long time.

[Aldorn]

Really appreciate this new feature, congrats !