From 7553aa4c0717bd414eeab1ef17f43de46eee801a Mon Sep 17 00:00:00 2001 From: IllianiCBT Date: Thu, 9 Jan 2025 22:33:01 -0600 Subject: [PATCH 1/2] Fix role eligibility checks and ensure proper role assignment Updated role eligibility logic to prevent assigning ineligible roles and removed redundant `Person` parameter in `canPerformRole`. Ensured consistent behavior when validating and updating both primary and secondary roles, resolving a long-standing bug. Improved GUI role selection to validate against all selected personnel. --- .../src/mekhq/campaign/personnel/Person.java | 38 +++++++++++-------- .../adapter/PersonnelTableMouseAdapter.java | 34 +++++++++++++---- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/MekHQ/src/mekhq/campaign/personnel/Person.java b/MekHQ/src/mekhq/campaign/personnel/Person.java index 0de2c9e071..3a581836ed 100644 --- a/MekHQ/src/mekhq/campaign/personnel/Person.java +++ b/MekHQ/src/mekhq/campaign/personnel/Person.java @@ -894,7 +894,7 @@ public String getSecondaryRoleDesc() { return getSecondaryRole().getName(isClanPersonnel()); } - public boolean canPerformRole(LocalDate today, Person person, final PersonnelRole role, final boolean primary) { + public boolean canPerformRole(LocalDate today, final PersonnelRole role, final boolean primary) { if (primary) { // Primary Role: // We only do a few here, as it is better on the UX-side to correct the issues @@ -930,7 +930,7 @@ public boolean canPerformRole(LocalDate today, Person person, final PersonnelRol } } - if (person.isChild(today)) { + if (isChild(today)) { return false; } @@ -943,8 +943,7 @@ public boolean canPerformRole(LocalDate today, Person person, final PersonnelRol case NAVAL_VEHICLE_DRIVER -> hasSkill(SkillType.S_PILOT_NVEE); case VTOL_PILOT -> hasSkill(SkillType.S_PILOT_VTOL); case VEHICLE_GUNNER -> hasSkill(SkillType.S_GUN_VEE); - case MECHANIC -> hasSkill(SkillType.S_TECH_MECHANIC) - && (getSkill(SkillType.S_TECH_MECHANIC).getExperienceLevel() > SkillType.EXP_ULTRA_GREEN); + case MECHANIC -> hasSkill(SkillType.S_TECH_MECHANIC); case VEHICLE_CREW -> Stream.of(SkillType.S_TECH_MEK, SkillType.S_TECH_AERO, SkillType.S_TECH_MECHANIC, SkillType.S_TECH_BA, SkillType.S_DOCTOR, SkillType.S_MEDTECH, SkillType.S_ASTECH) @@ -958,18 +957,11 @@ public boolean canPerformRole(LocalDate today, Person person, final PersonnelRol case VESSEL_CREW -> hasSkill(SkillType.S_TECH_VESSEL); case VESSEL_GUNNER -> hasSkill(SkillType.S_GUN_SPACE); case VESSEL_NAVIGATOR -> hasSkill(SkillType.S_NAV); - case MEK_TECH -> - hasSkill(SkillType.S_TECH_MEK) - && (getSkill(SkillType.S_TECH_MEK).getExperienceLevel() > SkillType.EXP_ULTRA_GREEN); - case AERO_TEK -> - hasSkill(SkillType.S_TECH_AERO) - && (getSkill(SkillType.S_TECH_AERO).getExperienceLevel() > SkillType.EXP_ULTRA_GREEN); - case BA_TECH -> - hasSkill(SkillType.S_TECH_BA) - && (getSkill(SkillType.S_TECH_BA).getExperienceLevel() > SkillType.EXP_ULTRA_GREEN); + case MEK_TECH -> hasSkill(SkillType.S_TECH_MEK); + case AERO_TEK -> hasSkill(SkillType.S_TECH_AERO); + case BA_TECH -> hasSkill(SkillType.S_TECH_BA); case ASTECH -> hasSkill(SkillType.S_ASTECH); - case DOCTOR -> hasSkill(SkillType.S_DOCTOR) - && (getSkill(SkillType.S_DOCTOR).getExperienceLevel() > SkillType.EXP_ULTRA_GREEN); + case DOCTOR -> hasSkill(SkillType.S_DOCTOR); case MEDIC -> hasSkill(SkillType.S_MEDTECH); case ADMINISTRATOR_COMMAND, ADMINISTRATOR_LOGISTICS, ADMINISTRATOR_TRANSPORT, ADMINISTRATOR_HR -> hasSkill(SkillType.S_ADMIN); @@ -2715,6 +2707,22 @@ public static Person generateInstanceFromXML(Node wn, Campaign c, Version versio if (retVal.getRecruitment() == null) { retVal.setRecruitment(c.getLocalDate()); } + + // This resolves a bug squashed in 2025 (50.03) but lurked in our codebase + // potentially as far back as 2014. The next two handlers should never be removed. + if (!retVal.canPerformRole(c.getLocalDate(), retVal.getPrimaryRole(), true)) { + retVal.setPrimaryRole(c, PersonnelRole.NONE); + logger.info(String.format("%s was found to be ineligible for their" + + " primary role. That role has been removed and they were assigned the" + + " NONE role.", retVal.getFullTitle())); + } + + if (!retVal.canPerformRole(c.getLocalDate(), retVal.getSecondaryRole(), false)) { + retVal.setSecondaryRole(PersonnelRole.NONE); + logger.info(String.format("%s was found to be ineligible for their" + + " secondary role. That role has been removed and they were assigned the" + + " NONE role.", retVal.getFullTitle())); + } } catch (Exception e) { logger.error("Failed to read person {} from file", retVal.getFullName(), e); retVal = null; diff --git a/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java b/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java index aaa398abb4..6de25e1813 100644 --- a/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java +++ b/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java @@ -1489,14 +1489,24 @@ protected Optional createPopupMenu() { } final PersonnelRole[] roles = PersonnelRole.values(); - menu = new JMenu(resources.getString("changePrimaryRole.text")); + menu = new JMenu(resources.getString("changePrimaryRole.text")); for (final PersonnelRole role : roles) { - if (person.canPerformRole(gui.getCampaign().getLocalDate(), person, role, true)) { - cbMenuItem = new JCheckBoxMenuItem(role.getName(person.isClanPersonnel())); + boolean allCanPerform = true; + + for (Person selectedPerson : getSelectedPeople()) { + if (!selectedPerson.canPerformRole(gui.getCampaign().getLocalDate(), role, true)) { + allCanPerform = false; + } + } + + if (allCanPerform) { + cbMenuItem = new JCheckBoxMenuItem(role.toString()); cbMenuItem.setActionCommand(makeCommand(CMD_PRIMARY_ROLE, role.name())); - cbMenuItem.setSelected(person.getPrimaryRole() == role); cbMenuItem.addActionListener(this); + if (oneSelected && role == person.getPrimaryRole()) { + cbMenuItem.setSelected(true); + } menu.add(cbMenuItem); } } @@ -1504,11 +1514,21 @@ protected Optional createPopupMenu() { menu = new JMenu(resources.getString("changeSecondaryRole.text")); for (final PersonnelRole role : roles) { - if (person.canPerformRole(gui.getCampaign().getLocalDate(), person, role, false)) { - cbMenuItem = new JCheckBoxMenuItem(role.getName(person.isClanPersonnel())); + boolean allCanPerform = true; + + for (Person selectedPerson : getSelectedPeople()) { + if (!selectedPerson.canPerformRole(gui.getCampaign().getLocalDate(), role, false)) { + allCanPerform = false; + } + } + + if (allCanPerform) { + cbMenuItem = new JCheckBoxMenuItem(role.toString()); cbMenuItem.setActionCommand(makeCommand(CMD_SECONDARY_ROLE, role.name())); - cbMenuItem.setSelected(person.getSecondaryRole() == role); cbMenuItem.addActionListener(this); + if (oneSelected && role == person.getSecondaryRole()) { + cbMenuItem.setSelected(true); + } menu.add(cbMenuItem); } } From 1545cb6d6bad0322dc8ed71924bfcb167fd2f979 Mon Sep 17 00:00:00 2001 From: IllianiCBT Date: Thu, 9 Jan 2025 22:36:38 -0600 Subject: [PATCH 2/2] Improve performance by breaking loops on role check failure Added `break` statements to terminate loops early when a selected person cannot perform the specified role. This prevents unnecessary checks and enhances efficiency in role validation. --- MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java b/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java index 6de25e1813..ebec401b80 100644 --- a/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java +++ b/MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java @@ -1497,6 +1497,7 @@ protected Optional createPopupMenu() { for (Person selectedPerson : getSelectedPeople()) { if (!selectedPerson.canPerformRole(gui.getCampaign().getLocalDate(), role, true)) { allCanPerform = false; + break; } } @@ -1519,6 +1520,7 @@ protected Optional createPopupMenu() { for (Person selectedPerson : getSelectedPeople()) { if (!selectedPerson.canPerformRole(gui.getCampaign().getLocalDate(), role, false)) { allCanPerform = false; + break; } }