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

Fix 6593: counterbattery icon hidden in double blind #6607

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Feb 24, 2025

Fixes two issues:

  1. Counter-battery fire arrow icon being hidden due to overzealous entity pruning on the game manager side;
  2. The arrow icon being hidden by the Unit Display dialog when undocked.

Testing:

  • Ran various repros (from original issue and new)
  • Ran all 3 projects' unit tests.

Close #6593

@Sleet01 Sleet01 requested a review from rjhancock February 24, 2025 00:10
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.09%. Comparing base (75e7787) to head (09e57e7).
Report is 48 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6607      +/-   ##
============================================
- Coverage     29.11%   29.09%   -0.02%     
+ Complexity    15295    15277      -18     
============================================
  Files          2837     2837              
  Lines        279718   279737      +19     
  Branches      49286    49291       +5     
============================================
- Hits          81429    81403      -26     
- Misses       192820   192878      +58     
+ Partials       5469     5456      -13     

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

Copy link
Collaborator

@rjhancock rjhancock left a comment

Choose a reason for hiding this comment

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

A minor nitpick to ensure what is being compared is actually being compared.

Ideal world these conditions would be read as (I hope) as intended to do the math first THEM compare but it could be read as either.

if (GUIP.getUnitDisplayEnabled() && GUIP.getUnitDisplayLocaton() == 0) {
// Move arrow inward if either side overlaps with the Unit Display
Rectangle udRectangle = clientgui.getUnitDisplayDialog().getBounds();
if ((myRectangle.x + myRectangle.width + 5 > udRectangle.x && myRectangle.x <= udRectangle.x + udRectangle.width) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some parenthesis to prevent ambiguity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I popped out to a show.
I was on the fence about adding more parentheses: on the one hand, unambiguous; on the other, longer and more difficult to read.
Luckily the operator precedence is with us this time:
https://pages.cs.wisc.edu/~willb/cs302/java-operator-precedence.pdf

But in the future I'll add the parentheses.

Copy link
Member

@HammerGS HammerGS left a comment

Choose a reason for hiding this comment

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

Checked with Co-Pilot and IDEA AI no issues detected

@HammerGS HammerGS merged commit 8110992 into MegaMek:master Feb 24, 2025
6 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
4 participants