-
Notifications
You must be signed in to change notification settings - Fork 391
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 and inappropriate retreat options. Bump version and fix old version processing #1621
Conversation
…date duplicated code
Don't ask for submerge or retreat when only defenseless transports remain
…propriately offered
… a combat is in progress
Don't ask for submerge or retreat when only defenseless transports remain
…propriately offered
… a combat is in progress
…384ecfdc54d6096f16cc56ea8360044ca993' into RemovePrompts2 Old Jar 1.9.0 made with above commit 4ca993
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observations...
@@ -815,9 +833,24 @@ private void pushFightLoopOnStack(final boolean firstRun) { | |||
|
|||
@Override | |||
public void execute(final ExecutionStack stack, final IDelegateBridge bridge) { | |||
if( Match.someMatch( m_attackingUnits, Matches.unitHasAttackValueOfAtLeast(1) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a Checkstyle whitespace violation (just run the Eclipse formatter to fix it).
latest_version_new.properties
Outdated
@@ -0,0 +1,4 @@ | |||
LATEST=1.9.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the background on why we need a latest_version_new.properties
?
@@ -31,6 +32,9 @@ protected GameEnginePropertyFileReader(final File propertyFile) { | |||
|
|||
@Override | |||
public String readProperty(final GameEngineProperty propertyKey) { | |||
if (propertyKey == GameEngineProperty.ENGINE_VERSION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug? It looks like object equality is being checked instead of verifying values. ie: this if
statement will only be true when propertyKey and GameEnginerProperty.ENGINE_VERSION are the same instance.
@simon33-2 some blockers:
@simon33-2 given the above, what are your thoughts and intent on how to move forward? |
We need to go to 1.9.1 anyway. We already have at least one change in the master code line which can break serialisation in some cases. Besides, this improvement is worth breaking backwards compatibility. Re: latest_version file. My idea of going to a new file would be that would allow 1.9.1 users to be notified when that version is deprecated. However, there is a better idea which is possible; I don't know how this was handled in 1.8 although it does appear that the jar file did indeed know what version it was. By incorporating the version in the source we can completely remove the latest_version* in later versions. If you are worrying about this, I think one option is to just have a 1.9.1 think that the latest version is 1.9.0 in the first build and then quickly update it before deploying to triplea-game.org or marking it as the latest release. Would that make you happy about this aspect? Keen to hear your thoughts. Re: GameEnginerProperty.ENGINE_VERSION |
I see, I'm not that opposed to going to 1.9.1 We are in a tough spot though since the intended update mechanism is broken. My hope/intent was we could work out what was needed to ensure we could do that upgrade. The game is supposed to notify you when there is a new version available and prompt you to download it. The engine actually does know which version it is by parsing the game_engine.properties file. Note that the game engine prints out the current version on the load game menus. The new file is an interesting idea. We will fail to notify users of the new version going that route. Since that is broken anyways, i can understand the work around. I was thinking if we are explicit enough with some lobby messaging and let enough time go by with a patched version available for release, that those who are 'stuck' will be few and will hopefully re-download. |
So which way are you leaning:
We need to avoid having nearly 100% of installed users hang IMO. When we get the majority of users with a functional latest version check, we can start to think about breaking the system for those old users. |
New file would have the problem that it would be difficult to migrate off of it. If we had a lot of time, I would lean towards two, non functional. Migrating to a new file is a bit more effort, but helps with a shorter term rollout. New file and planning to stay with that file is perhaps the best migration path available. |
I'm inclined to agree, given that I'm suggesting a single point of truth solution. I'll push that up shortly. |
Hmm, having a small amount of trouble with git. Once a commit has been pushed I'm not sure how to remove it from a given branch, so I killed "RemovePrompts2"
Previous attempts are #1599 & #1615.
Basically, this resolves most or all of #1270. Also removes a redundant prompt for submerging subs against only air units.
This PR and #1583 break serialisation in certain cases so move the version number on and establish a new file for the latest version. If there is a better way of doing this then I am all ears.
The major reason why isn't it achievable to download the actual source file which contains the version number is because that needs to be merged in two phases. The build will fail if code which refers to the source is merged at the same time as the source being updated, if you follow me. This is a one time problem though. If this is merged, I reckon just update EngineVersionProperties to refer to GameEnginePropertyFileReader. Then the version is stored in a single place and a jar file knows what version it is, if not which build.