Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Second attempt: Sell Parts from Parts in Use Dialog #4792

Merged
merged 7 commits into from
Sep 13, 2024

Conversation

Algebro7
Copy link
Collaborator

@Algebro7 Algebro7 commented Sep 5, 2024

This PR re-implements selling parts from the parts in use dialog in a (hopefully) much more robust way. Rather than storing strings representing the name and looking up the parts in the warehouse, which was incredibly unreliable due to inconsistent naming under the hood, references to the legit spare part objects are stored in the PartInUse row when building out the table. These objects are rebuilt everytime the table refreshes or is re-opened so there should be no concern about the extra persistent state.

I also added a getSellableQuantity() method to the Part class, primarily to be overridden by AmmoStorage and Armor, which track their quantities different than all other parts. This allows the caller to not have to explicitly check the type of the Part being sold.

Closes #4723

@Algebro7 Algebro7 requested a review from IllianiCBT September 5, 2024 15:26
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.76%. Comparing base (2dbc50b) to head (51714d2).
Report is 69 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4792      +/-   ##
============================================
- Coverage     10.76%   10.76%   -0.01%     
+ Complexity     5668     5667       -1     
============================================
  Files           948      948              
  Lines        132553   132569      +16     
  Branches      19048    19052       +4     
============================================
- Hits          14272    14269       -3     
- Misses       117041   117061      +20     
+ Partials       1240     1239       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@IllianiCBT IllianiCBT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking this as approved, despite a minor bit of feedback. Overall the code is clean and looks functional. I would definitely want this in the hands of QA so they can vigorously test it, so once it’s merged please flag it in the QA channel and ask them to break it. With this being such a high demand function, I wanna make sure it hits release in a 10/10 state, if that makes sense.

MekHQ/src/mekhq/campaign/parts/Part.java Show resolved Hide resolved
@Algebro7 Algebro7 force-pushed the refactor-parts-in-use-dialog branch from 80ef8ce to 0dd549a Compare September 8, 2024 19:04
@IllianiCBT
Copy link
Collaborator

This has caught some conflicts following the recent name changes.

@Algebro7 Algebro7 force-pushed the refactor-parts-in-use-dialog branch from 0dd549a to 51714d2 Compare September 13, 2024 19:39
@IllianiCBT IllianiCBT merged commit b2ec5e2 into MegaMek:master Sep 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants