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

Add new player option giveUnitControlInAllTerritories #4808

Merged
merged 2 commits into from Apr 11, 2019

Conversation

Projects
None yet
3 participants
@ron-murhammer
Copy link
Member

ron-murhammer commented Apr 9, 2019

Overview

Example

Can do this:

        <attachment name="playerAttachment" attachTo="French_Africa" javaClass="PlayerAttachment" type="player">
            <option name="giveUnitControl" value="France"/>
            <option name="giveUnitControlInAllTerritories" value="true"/>
        </attachment>

Avoids having to have this on every territory:

        <attachment name="territoryAttachment" attachTo="Morocco" javaClass="TerritoryAttachment" type="territory">
            <option name="production" value="2"/>
            <option name="changeUnitOwners" value="France:Germany:Austria-Hungary:Ottomans:Italy:Usa"/>           
        </attachment>

Manual Testing Performed

if (inAllTerritories || (ta != null && !ta.getChangeUnitOwners().isEmpty())) {
final List<PlayerId> newOwners = (ta != null && !ta.getChangeUnitOwners().isEmpty())
? ta.getChangeUnitOwners()
: bridge.getData().getPlayerList().getPlayers();

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Apr 9, 2019

Author Member

These few lines above are the core change where setting giveUnitControlInAllTerritories to true avoids needing a list of players in each territory if map makers want units to change to another player in all territories.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 9, 2019

Codecov Report

Merging #4808 into master will decrease coverage by 0.22%.
The diff coverage is 14.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4808      +/-   ##
============================================
- Coverage      23.2%   22.97%   -0.23%     
+ Complexity     6151     6149       -2     
============================================
  Files           862      862              
  Lines         69791    70465     +674     
  Branches      11173    11342     +169     
============================================
- Hits          16193    16192       -1     
- Misses        51573    52246     +673     
- Partials       2025     2027       +2
Impacted Files Coverage Δ Complexity Δ
...tegy/triplea/delegate/AbstractEndTurnDelegate.java 7.5% <0%> (-0.05%) 12 <0> (ø)
...strategy/triplea/attachments/PlayerAttachment.java 34.73% <42.85%> (+0.2%) 23 <0> (ø) ⬇️
...rategy/triplea/attachments/UnitTypeComparator.java 46.15% <0%> (-7.7%) 12% <0%> (-1%)
.../src/main/java/games/strategy/net/nio/Decoder.java 66.07% <0%> (-1.79%) 12% <0%> (ø)
.../strategy/triplea/odds/calculator/DummyPlayer.java 34.83% <0%> (-1.13%) 9% <0%> (-1%)
...rg/triplea/game/client/ui/javafx/MainMenuPane.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...ain/java/games/strategy/triplea/ui/NotesPanel.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
.../main/java/games/strategy/triplea/ui/MapPanel.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...n/java/games/strategy/triplea/ui/TripleAFrame.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...va/games/strategy/triplea/ui/menubar/HelpMenu.java 0% <0%> (ø) 0% <0%> (ø) ⬇️

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 fae5d66...d6c53cc. Read the comment docs.

@RoiEXLab
Copy link
Member

RoiEXLab left a comment

👍
Just this one thing I'm not sure about

this::setGiveUnitControlInAllTerritories,
this::setGiveUnitControlInAllTerritories,
this::getGiveUnitControlInAllTerritories,
this::resetGiveUnitControlInAllTerritories))

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Apr 9, 2019

Member

I'm not 100% sure anymore, but I think there's a method that allows to provide a simple mapping function (getBool in this case) instead of another setter for string values.

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Apr 10, 2019

Author Member

Not that I'm aware of. I don't see any other boolean properties using an alternative.

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Apr 10, 2019

Member

In this case it's probably just an idea I had at some point

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Apr 10, 2019

Member

Nevermind, I found it:

public static <T> MutableProperty<T> ofMapper(
final ThrowingFunction<String, T, Exception> mapper,
final ThrowingConsumer<T, Exception> setter,
final Supplier<T> getter,
final Supplier<T> defaultValue) {
return new MutableProperty<>(setter, o -> setter.accept(mapper.apply(o)), getter, () -> {
try {
setter.accept(defaultValue.get());
} catch (final RuntimeException e) {
throw e;
} catch (final Exception e) {
throw new IllegalStateException("Unexpected Error while resetting value", e);
}
});
}

This would reduce the amount of boilerplate code slightly

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Apr 11, 2019

Author Member

Updated.

@RoiEXLab RoiEXLab merged commit 6ad8982 into master Apr 11, 2019

3 checks passed

LGTM analysis: Java No new or fixed alerts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@RoiEXLab RoiEXLab deleted the GiveUnitControl_Add_Option_For_All_Territories branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.