Refactor mek hq unit selector#7085
Merged
Merged
Conversation
…refactorMekHQUnitSelector # Conflicts: # MekHQ/src/mekhq/gui/dialog/MekHQUnitSelectorDialog.java
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7085 +/- ##
============================================
- Coverage 12.53% 12.48% -0.05%
Complexity 7363 7363
============================================
Files 1128 1133 +5
Lines 145643 146220 +577
Branches 22394 22422 +28
============================================
Hits 18262 18262
- Misses 125503 126082 +579
+ Partials 1878 1876 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…refactorMekHQUnitSelector # Conflicts: # MekHQ/src/mekhq/gui/dialog/MekHQUnitSelectorDialog.java
…refactorMekHQUnitSelector
rjhancock
previously approved these changes
May 24, 2025
rjhancock
previously requested changes
May 24, 2025
rjhancock
left a comment
Collaborator
There was a problem hiding this comment.
Copyright header and the javadoc are the two fixes I spotted.
Scoppio
suggested changes
May 24, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
Refactors the MekHQ unit selector dialog by separating and properly managing the buy/add-GM buttons, cleaning up the filtering logic, and adding documentation.
- Extracts buy and GM-add buttons from the abstract class and initializes them independently
- Moves complex filtering logic into a new
isAllowedUnithelper method - Adds Javadoc for key methods and cleans up the
includeoverride
Comments suppressed due to low confidence (2)
MekHQ/src/mekhq/gui/dialog/MekHQUnitSelectorDialog.java:253
- Rename parameter 'NoOP' to follow lowerCamelCase and convey its purpose (e.g. 'unused' or '_').
protected void select(boolean NoOP) {
MekHQ/src/mekhq/gui/dialog/MekHQUnitSelectorDialog.java:192
- [nitpick] Consider renaming 'isBadSelection' to something like 'isInvalidSelection' for clearer intent.
private boolean isBadSelection() {
Scoppio
approved these changes
May 25, 2025
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The MekHQUnitSelectorDialog had a lot of inconsistencies and bad design choices. This updates the code to add the 'buy' and 'add gm' buttons separately from the abstract class, correctly instantiates them during initialization, updates their state to match the desired states on init and during runtime changes, and logic was cleaned up in the 'include' function override. A LOT of documentation was added to clearly state what's going on. Still needs more documentation, likely.