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

Fixed issue 2.6.89 by calling JOptionPane in a Swing event thread #9091

Merged
merged 2 commits into from
Apr 9, 2021
Merged

Fixed issue 2.6.89 by calling JOptionPane in a Swing event thread #9091

merged 2 commits into from
Apr 9, 2021

Conversation

bkelly1984
Copy link
Contributor

@bkelly1984 bkelly1984 commented Apr 6, 2021

Only one commit and one method modified.

Testing

I was able to verify the problem in my own save game on a World War II Classic map with Java 15.0.2 running on Windows 10.

To reproduce, load this save game. Then...

Move infantry from Panama to West Panama Sea Zone
Move transport and battleships from West Panama Sea Zone to Mexico Sea Zone
Move infantry from Mexico Sea Zone to Mexico
Press "Done" to end the combat phase

This does not work with the current master branch. It does work in my modification. I also verified that confirmation dialogs from Swing event threads still work, such as requesting to exit the program.

Screens Shots

Failure
Success

Additional Notes to Reviewer

First pull request. Please let me know if anything is incorrect.

Release Note

FIX|Fixed a bug that caused confirmation dialogs to show no content.

… and JDialog constructor in a Swing event thread if not in one presently. This fixes issue 2.6.89.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Brian Kelly seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov
Copy link

codecov bot commented Apr 6, 2021

Codecov Report

Merging #9091 (eaec0ad) into master (85a52f8) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #9091      +/-   ##
============================================
- Coverage     28.36%   28.35%   -0.02%     
+ Complexity     8371     8369       -2     
============================================
  Files          1303     1303              
  Lines         81041    81054      +13     
  Branches      11057    11059       +2     
============================================
- Hits          22991    22986       -5     
- Misses        55871    55888      +17     
- Partials       2179     2180       +1     
Impacted Files Coverage Δ Complexity Δ
...java/org/triplea/swing/EventThreadJOptionPane.java 19.58% <0.00%> (-3.04%) 6.00 <0.00> (ø)
...a/ui/unit/scroller/AvatarCoordinateCalculator.java 69.23% <0.00%> (-15.39%) 5.00% <0.00%> (-1.00%)
...rc/main/java/games/strategy/net/nio/NioReader.java 82.50% <0.00%> (-3.75%) 17.00% <0.00%> (-1.00%)
...lea/ai/flowfield/influence/InfluenceTerritory.java 96.96% <0.00%> (-3.04%) 13.00% <0.00%> (-1.00%)
.../src/main/java/games/strategy/net/nio/Decoder.java 75.86% <0.00%> (ø) 14.00% <0.00%> (ø%)
...strategy/net/nio/ServerQuarantineConversation.java 91.22% <0.00%> (+1.75%) 13.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85a52f8...eaec0ad. Read the comment docs.

@DanVanAtta
Copy link
Member

Thank you for submitting a patch, great to see!

@bkelly1984 , did you have issues signing the CLA? It is reporting something like a username mismatch, is the automatic CLA tool broken?

Second, when testing the dialogs, did you double check they are still not-modal? IE: always-on-top but you can still scroll the map?

I re-triggered the build for unit-tests, it looks like it may have failed due to a flaky test.

Copy link
Member

@DanVanAtta DanVanAtta left a comment

Choose a reason for hiding this comment

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

Congrats on first PR 💯

@DanVanAtta DanVanAtta merged commit 568957a into triplea-game:master Apr 9, 2021
@bkelly1984
Copy link
Contributor Author

@bkelly1984 , did you have issues signing the CLA? It is reporting something like a username mismatch, is the automatic CLA tool broken?

Not that I know of. My account was missing a profile name. I also clicked on the CLA link again and accepted it. When I look again, there is no option. Is it fixed?

Second, when testing the dialogs, did you double check they are still not-modal? IE: always-on-top but you can still scroll the map?

I did. Thought about removing the setModal(false) line but found one could no longer scroll the map.

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.

3 participants