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

Remove redundant prompts #1599

Closed
wants to merge 14 commits into from
Closed

Conversation

simon33-2
Copy link
Contributor

@simon33-2 simon33-2 commented Mar 31, 2017

Don't prompt for retreat when only one retreat territory is valid

Don't ask for submerge or retreat when only defenseless transports remain

EDIT: Also don't allow retreat when only an AA Gun or defenseless transport remains as per #1270.

Breaks serialisation when a battle is in progress so just move to 1.9.1. #1583 also causes issues with serialization in certain situations. Trouble is that the old jar mechanism doesn't quite work for 1.9, although 1.9 can spawn 1.809. I've got a fix for this one but I'm open to a better fix. Reopen because I messed with the successor PR too.

Simon added 3 commits March 24, 2017 22:02
@simon33-2
Copy link
Contributor Author

Hold on, I'll see if I can figure where those tests fail.

@panther2
Copy link
Contributor

Does this address and solve #1270 ?

@simon33-2
Copy link
Contributor Author

Partly. Doesn't do anything about the remaining AA Gun but does prevent the retreat prompt when a sea battle leaves a non fighting TT. Not 100% sure about the submarine vs air problem. I'll have to test it.

@RoiEXLab
Copy link
Member

One Test is still failing...

@simon33-2
Copy link
Contributor Author

Fixed now.

Copy link
Member

@RoiEXLab RoiEXLab left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I'd like you to chnage a couple things

@@ -1,4 +1,4 @@
engine_version = 1.9.0.0
engine_version = 1.9.0.2
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is going to break anything...

@@ -1821,13 +1824,13 @@ private void checkForUnitsThatCanRollLeft(final IDelegateBridge bridge, final bo
*/
private void submergeSubsVsOnlyAir(final IDelegateBridge bridge) {
// if All attackers are AIR submerge any defending subs ..m_defendingUnits.removeAll(m_killed);
if (Match.allMatch(m_attackingUnits, Matches.UnitIsAir) && Match.someMatch(m_defendingUnits, Matches.UnitIsSub)) {
if ( Match.allMatch(m_attackingUnits, Matches.UnitIsAir) && Match.someMatch(m_defendingUnits, Matches.UnitIsSub)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like you to remove this space

Copy link
Member

Choose a reason for hiding this comment

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

I'm probably stating something that everyone already knows, but if you're using Eclipse, there are TripleA formatter and cleanup configurations available here (they can be used from IntelliJ, too, with a little more work). I've gotten into the habit of running Source > Clean Up... on the files I've changed before committing so that I don't have to worry that I fat-fingered a couple of spaces (among other things like removing unused imports, making variables final, etc.). 😃

// Get all defending subs (including allies) in the territory.
final List<Unit> defendingSubs = Match.getMatches(m_defendingUnits, Matches.UnitIsSub);
// submerge defending subs
submergeUnits(defendingSubs, true, bridge);
// checking defending air on attacking subs
} else if (Match.allMatch(m_defendingUnits, Matches.UnitIsAir)
} else if ( Match.allMatch(m_defendingUnits, Matches.UnitIsAir)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like you to remove this space too

@simon33-2
Copy link
Contributor Author

This now also fixes AA Guns. I think it fixes the subs too although I'm not 100% sure I understand that issue. So should allow #1270 to be closed off.

Regarding the version, I have decreed that it is about time we moved on the version. 1.901 has already been used. If we don't update the latest_version quite yet, it doesn't break anything that I am aware of. We need to advertise that people should upgrade, then after we've given them some time, we can update the latest_version which will break the older versions of 1.9, albeit with a workaround of disabling the network connection while the version is being checked.

I have eclipse but I've forgotten how to configure it.

Positively hate parenthesis surrounding the condition in "if" statements! I don't know why people like it - I've seen it done in languages which don't require it. I also hate parenthesis used for grouping and functions. I want braces for functions and not blocks. Anyway, I guess that's enough esoteric argument for today.

@simon33-2
Copy link
Contributor Author

There was still an issue with subs vs air but that should be fixed now.

@simon33-2 simon33-2 mentioned this pull request Apr 1, 2017
@simon33-2
Copy link
Contributor Author

Note that this PR breaks serialisation in cases where a game is saved during a battle. That's why I've updated the version to 1.9.1.

We shouldn't update latest_version quite yet because it causes older builds to hang.

@simon33-2
Copy link
Contributor Author

Can I get some thoughts on moving to 1.9.1 @DanVanAtta @ron-murhammer ?

I note that the fix for the latest version check has not been migrated to triplea-game.org yet. I suggest that we change the filename checked. Why not include the latest version in the triplea_maps.yaml and check that every startup - is the 91kb that the file is an issue? Can we check the git commit hash perhaps?

@simon33-2
Copy link
Contributor Author

I'm going to have a look tomorrow how much of this can be achieved without breaking the compatibility.

@simon33-2 simon33-2 mentioned this pull request Apr 4, 2017
@simon33-2
Copy link
Contributor Author

simon33-2 commented Apr 5, 2017

Old jar mechanisms appears to be broken in 1.9

Seems that the 1.9 triplea.jar is unaware of what version it is but the 1.809 jar seems like it might be. In any event if I build 1.9.1 of Triple-A and put 1.9.0 of triple-A in the old jar directory, 1.9.0 has an identity crisis and thinks its 1.9.1 because it looks at the game_engine.properties. What sort of system is this? The jar should be self aware. Ideally, the build would examine the source and put that version number into the game_engine.properties.

@ron-murhammer
Copy link
Member

@simon33-2 Can you update the initial post of this with exactly what features are being changed and how to test them. Seems like a number of different things are being changed here along with updating to 1.9.1. Also referencing back to any issues that this is addressing helps as well.

protected final Collection<Territory> m_amphibiousAttackFrom = new ArrayList<>();

DependentBattle(final Territory battleSite, final PlayerID attacker, final BattleTracker battleTracker,
final boolean isBombingRun, final BattleType battleType, final GameData data) {
super(battleSite, attacker, battleTracker, isBombingRun, battleType, data);
m_attackingFromMap = new HashMap<>();
m_attackingFrom = new HashSet<>();
Copy link
Member

Choose a reason for hiding this comment

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

Same thing like in #1610...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. If we move on the version, that change shouldn't be needed.

Copy link
Member

@RoiEXLab RoiEXLab left a comment

Choose a reason for hiding this comment

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

You should run the eclipse formatting tool (Ctrl+Shift+F)

@simon33-2
Copy link
Contributor Author

Isn't there some configuration needed to get the formatting tool to work? I've forgotten how to do that.

@ssoloff
Copy link
Member

ssoloff commented Apr 6, 2017

@simon33-2

Isn't there some configuration needed to get the formatting tool to work?

Configure clean up

  • From Eclipse main menu: Window > Preferences
  • Expand node: Java > Code Style > Clean Up
  • Click Import...
  • Navigate to the following folder in your TripleA Git repo: /eclipse/format
  • Select file triplea_java_eclipse_cleanup.xml
  • Ensure TripleaCleanup is selected as the Active profile
  • Click OK or Apply

Configure formatter

  • From Eclipse main menu: Window > Preferences
  • Expand node: Java > Code Style > Formatter
  • Click Import...
  • Navigate to the following folder in your TripleA Git repo: /eclipse/format
  • Select file triplea_java_eclipse_format_style.xml
  • Ensure TripleaStyle is selected as the Active profile
  • Click OK or Apply

Configure import order

  • From Eclipse main menu: Window > Preferences
  • Expand node: Java > Code Style > Organize Imports
  • Click Import...
  • Navigate to the following folder in your TripleA Git repo: /eclipse/format
  • Select file triplea.importorder
  • Click OK or Apply

Usage

As @RoiEXLab said, you can use the default key binding, Ctrl+Shift+F, to run the formatter on any source file. However, I would recommend you consider running the whole Clean Up suite, which includes the formatter, among other useful things (e.g. organize imports, remove trailing whitespace, etc.). Run the Clean Up command using Source > Clean Up... from either the main menu or the context menu. There is no default key binding for this command, but you can add one from the General > Keys node in preferences.

@simon33-2
Copy link
Contributor Author

Close this for rebased PR #1615.

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.

5 participants