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

Fix #7017: Conditional orders enum updated without savegame version bump #7019

Merged
merged 1 commit into from Jan 6, 2019

Conversation

@nielsmh
Copy link
Contributor

nielsmh commented Jan 6, 2019

PR #7017 changed the conditional orders conditions enum by adding a value in the middle. This breaks loading old savegames that use conditional orders. Fix this by moving the new enum value to the end, while keeping the UI list order the same.

Saving this orders list in 1.8.0:
image

Loads as this orders list with the #7017 patch but without this fix:
image

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Jan 6, 2019

Would be better to avoid a savegame conversion if possible. Can we not just move enum value to the end, and accept that savegames from that specific revision are incorrect?

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Jan 6, 2019

Main issue with moving the value is UX. The options are presented in definition order in the UI, so moving "max reliability" (the new one) to the end separates it from "(current) reliability".

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Jan 6, 2019

Well that's just bad UI design :/

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Jan 6, 2019

No you're right, the order in the UI does not have to match the order in the enum, I just discovered. Going to change the patch to not bump savegame version.

@nielsmh nielsmh force-pushed the nielsmh:fix7017 branch from bfe0943 to 98749c2 Jan 6, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Jan 6, 2019

This is definitely a better fix, thanks.

@PeterN
PeterN approved these changes Jan 6, 2019
@nielsmh nielsmh merged commit 15a7f9d into OpenTTD:master Jan 6, 2019
1 check passed
1 check passed
OpenTTD CI Build #20190106.2 succeeded
Details
@nielsmh nielsmh deleted the nielsmh:fix7017 branch Jan 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.