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

Separate out non retreatable units in the casualty selector #8947

Conversation

trevan
Copy link
Contributor

@trevan trevan commented Mar 1, 2021

The UI needs to be worked on but this will separate out units that can retreat from units that can't retreat.

Testing

Screens Shots

Additional Notes to Reviewer

Release Note

@trevan
Copy link
Contributor Author

trevan commented Mar 1, 2021

The UnitChooser is the code that builds the UI. It is used in several places, not just the casualty selection. It already handles separating out units by their dependents (such as transports), by their transport cost, and by their left over movement. I added a new case to handle the non-retreat state but it shows the image.

@trevan trevan force-pushed the separate-non-retreatable-units-in-casualty-selector branch from 7dd4b26 to b06885f Compare March 1, 2021 05:33
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #8947 (16e4ad0) into master (e1cf9ee) will decrease coverage by 0.24%.
The diff coverage is 45.45%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8947      +/-   ##
============================================
- Coverage     28.38%   28.13%   -0.25%     
- Complexity     8370     9454    +1084     
============================================
  Files          1300     1442     +142     
  Lines         80931    91966   +11035     
  Branches      11053    13826    +2773     
============================================
+ Hits          22975    25879    +2904     
- Misses        55781    63770    +7989     
- Partials       2175     2317     +142     
Impacted Files Coverage Δ
.../java/games/strategy/triplea/ui/BattleDisplay.java 0.00% <0.00%> (ø)
...in/java/games/strategy/triplea/ui/BattlePanel.java 0.00% <0.00%> (ø)
...in/java/games/strategy/triplea/ui/UnitChooser.java 0.00% <0.00%> (ø)
...ava/games/strategy/triplea/ui/mapdata/MapData.java 2.92% <0.00%> (+0.13%) ⬆️
...lea/delegate/battle/casualty/CasualtySelector.java 67.76% <100.00%> (+4.75%) ⬆️
...java/games/strategy/triplea/util/UnitCategory.java 85.91% <100.00%> (ø)
...ava/games/strategy/triplea/util/UnitSeparator.java 85.10% <100.00%> (+4.25%) ⬆️
...a/games/strategy/triplea/settings/GameSetting.java 50.00% <0.00%> (-50.00%) ⬇️
...s/strategy/triplea/settings/PathClientSetting.java 71.42% <0.00%> (-28.58%) ⬇️
...rc/main/java/org/triplea/io/ContentDownloader.java 36.78% <0.00%> (-22.05%) ⬇️
... and 309 more

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 e1cf9ee...16e4ad0. Read the comment docs.

@Cernelius
Copy link
Contributor

Is this more likely to be a long term project or more likely to be featured in TripleA 2.6 (asking because it is relevant to a map of mine which I'm going to update soon)?

@stale
Copy link

stale bot commented Mar 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. We are eager to see this work completed, please update and re-open as soon as possible.

@stale stale bot added the Stale label Mar 26, 2021
@DanVanAtta DanVanAtta removed the Stale label Mar 26, 2021
@DanVanAtta DanVanAtta self-assigned this May 31, 2021
@Cernelius
Copy link
Contributor

Cernelius commented Jun 1, 2021

So, is it safe to say that TripleA 2.6 will not allow picking between retreatable and non?

EDIT: Nevermind: I just noticed the recent assignation, which I guess means the intention to get this in the first 2.6 release.

@stale
Copy link

stale bot commented Jun 16, 2021

This pull request has been automatically marked as stale because it has not had recent activity. We are eager to see this work completed, please update and re-open as soon as possible.

@stale stale bot added the Stale label Jun 16, 2021
@DanVanAtta DanVanAtta removed the Stale label Jun 16, 2021
@stale
Copy link

stale bot commented Jun 30, 2021

This pull request has been automatically marked as stale because it has not had recent activity. We are eager to see this work completed, please update and re-open as soon as possible.

@RaiNova
Copy link
Contributor

RaiNova commented Sep 12, 2021

@trevan my pull request #9509 depends on this one. Do you already have a plan when to finalize your pull request?

@trevan
Copy link
Contributor Author

trevan commented Sep 12, 2021

@RaiNova , this was a draft PR because there was still discussion about the UI and I don't have much Java UI experience. So I committed the logic part and then expected someone else to try and work on the UI. You can see the discussion in the forum at https://forums.triplea-game.org/topic/2677/allow-user-to-specifically-choose-amphibious-offloaded-units-in-battle-chooser/29?_=1631472565173&lang=en-US.

@RaiNova
Copy link
Contributor

RaiNova commented Sep 17, 2021

@RaiNova , this was a draft PR because there was still discussion about the UI and I don't have much Java UI experience. So I committed the logic part and then expected someone else to try and work on the UI. You can see the discussion in the forum at https://forums.triplea-game.org/topic/2677/allow-user-to-specifically-choose-amphibious-offloaded-units-in-battle-chooser/29?_=1631472565173&lang=en-US.

@trevan As you mentioned in the forum, I should now build on your pull request. What is the easiest way? Lacking much experience with github, it looks to me the easiest way would be you finalize your pull request, I reveive it by an update of the head and the integrate the UI. The code I am working on includes my pull reuqest #9509. I‘d like to keep me work consolidate in that pull request.

Would that work?

@trevan
Copy link
Contributor Author

trevan commented Sep 17, 2021

@RaiNova , I don't think we want to merge this until the UI is done because this will change the behavior of the game. I'd recommend you cherry-pick the commit into your branch and then we close this PR.

@DanVanAtta
Copy link
Member

@RaiNova this was waiting for me to finish the UI components. I haven't been able to manage the time/effort despite my better wishes. If you're willing to, would you want to finish this PR? Judging by the screenshots in the forums, it looks like you are far along the way.

Cherry-picking commits into a new branch could be a good way to go.

@trevan , which changes to behavior are you referring to (more specifically)? Is that referring to how the non-retreat units will be split out in UI, or are there further changes you were thinking of?

@trevan
Copy link
Contributor Author

trevan commented Sep 18, 2021

@DanVanAtta , the changes I'm referring to is how the non-retreat units will be split out in the UI. But there won't be any indication on why they are split. So users wouldn't be able to know which of the rows is the non-retreat units and which are the retreat units. Nor would they be able to tell why there are two rows of the same unit.

@DanVanAtta
Copy link
Member

@RaiNova could you confirm if you are taking over this effort / PR? If so, do you have a plan or expectation for which changes would land when?

@RaiNova
Copy link
Contributor

RaiNova commented Oct 30, 2021

@RaiNova could you confirm if you are taking over this effort / PR? If so, do you have a plan or expectation for which changes would land when?

Confirmed. I'll try to complete this by the end of November. It's functionally complete already, but I need to do more testing

RaiNova added a commit to RaiNova/triplea that referenced this pull request Nov 1, 2021
PR triplea-game#8947 can be closed.

Automated UI testing is not included, because I wrote it in Kotlin and don't want to put too much into this PR.
@RaiNova
Copy link
Contributor

RaiNova commented Nov 1, 2021

This PR is now included in #9509 and can be closed. But I can't close it - the close button doesn't show left to the comment button.

@DanVanAtta
Copy link
Member

Closing in favor #9509

@DanVanAtta DanVanAtta closed this Nov 27, 2021
RaiNova added a commit to RaiNova/triplea that referenced this pull request Jan 27, 2022
PR triplea-game#8947 can be closed.

Automated UI testing is not included, because I wrote it in Kotlin and don't want to put too much into this PR.
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.

None yet

4 participants