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

New names cleanup and potential bugfixes #5043

Merged
merged 5 commits into from
Oct 13, 2024

Conversation

Setsul
Copy link
Contributor

@Setsul Setsul commented Oct 13, 2024

That should be the last of MekHQ updated to follow the new "IS name (clan name) model" naming scheme. MM and MML still have some bits.
Please excuse the funny list of commits, only bd0e259 is actually new.

This will prevent custom refits of clan units from reusing already taken model names. I don't think that bug was reported though.
The change from loadMekData() to refreshUnitData() might help with #4554 and potentially fix #4791 but I haven't tested either and the proper way for the former would still be to enable the (un)loading of a single specified file.

The rest is some incidental cleanup and refactoring to prepare for implementing #4978

Kludge to handle blank model names in populateRefit()
Makes the refit GUI properly query for duplicates using the new naming
scheme.
Changes the filenames of custom refits to follow the "IS name (clan
name) model.mtf" scheme.
Refactoring of the file output code in preparation for future changes.
Refactoring to remove retrieveOriginalUnit(Entity newE) and properly use
refreshUnitData() instead of loadMekData to refresh the cache after
saving new custom refits.
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.

How much testing has the cache portion of this had? The code looks fine, I’m just wary of exacerbating the existing refit issues.

@IllianiCBT
Copy link
Collaborator

Also, this is approved pending the JavaDoc error being fixed

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.43%. Comparing base (3e8168d) to head (47350d2).
Report is 21 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #5043   +/-   ##
=========================================
  Coverage     10.43%   10.43%           
- Complexity     6022     6027    +5     
=========================================
  Files           952      952           
  Lines        133810   133861   +51     
  Branches      19432    19429    -3     
=========================================
+ Hits          13961    13969    +8     
- Misses       118498   118547   +49     
+ Partials       1351     1345    -6     

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

@Setsul
Copy link
Contributor Author

Setsul commented Oct 13, 2024

How much testing has the cache portion of this had? The code looks fine, I’m just wary of exacerbating the existing refit issues.

"It works on my machine"
Not very extensive, but the few refits I've tried did show up and it didn't lag or freeze.
refreshUnitData() is used elsewhere so if it's broken we need to fix it anyway.

@IllianiCBT
Copy link
Collaborator

That was my assumption, but I’ve learned not to assume. When this gets merged let’s flag it with the QA folks so they know to test it, speaking of which I should probably give you to contributor role on Discord…

@HammerGS HammerGS merged commit 4486c6c into MegaMek:master Oct 13, 2024
4 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
Development

Successfully merging this pull request may close these issues.

units.cache gaining extra duplicates of refits and sometimes the entire unit database
4 participants