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 #8922: Show vehicle window for single vehicle in shared order grouping #8926

Merged
merged 1 commit into from Apr 6, 2021

Conversation

@btzy
Copy link
Contributor

@btzy btzy commented Apr 1, 2021

Motivation / Problem

Fixes #8922 - shared order list could have only one vehicle, which leads to a crash later when that vehicle is deleted.

Description

When viewing by shared order groups, if a vehicle is not shared, don't open the shared order list (of just a single vehicle). Instead, open the vehicle window.

Limitations

None

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@btzy btzy changed the title Fix: Show vehicle window for single vehicle in shared order grouping Fix #8922: Show vehicle window for single vehicle in shared order grouping Apr 1, 2021
@LordAro
Copy link
Member

@LordAro LordAro commented Apr 1, 2021

Does this fix the issue? If a shared group window is opened, then the list size is reduced from 2+ to none, it will still crash, no?

@btzy
Copy link
Contributor Author

@btzy btzy commented Apr 2, 2021

If a shared group window is opened, then the list size is reduced from 2+ to none, it will still crash

Hmm, I'll check that later tonight. However, if this was the case, then this bug would have been in OpenTTD for much longer than my PR for grouping by shared orders (and hence surprising that it was only discovered now).

@btzy
Copy link
Contributor Author

@btzy btzy commented Apr 2, 2021

If a shared group window is opened, then the list size is reduced from 2+ to none, it will still crash, no?

I've just tested and it no longer crashes: In the linked savegame, I bought a new vehicle and cloned the orders of vehicle 289 to it, then sold all vehicles in the depot (i.e. both of them). The shared orders window disappears as expected; no crashes.

I believe the code that deletes the shared order window when there is only one vehicle left is this:

OpenTTD/src/vehicle.cpp

Lines 2790 to 2793 in 799eb31

if (this->orders.list->GetNumVehicles() == 1) {
/* When there is only one vehicle, remove the shared order list window. */
DeleteWindowById(GetWindowClassForVehicleType(this->type), vli.Pack());
InvalidateVehicleOrder(this->FirstShared(), VIWD_MODIFY_ORDERS);

Presumably selling all vehicles in the depot will delete vehicles one-by-one, which is probably why it works.

@btzy btzy force-pushed the no-shared-order-of-1-vehicle branch from 8cc6a32 to 087ab7c Apr 2, 2021
@btzy
Copy link
Contributor Author

@btzy btzy commented Apr 2, 2021

Reworded the commit message to conform to the style guide.

LordAro
LordAro previously approved these changes Apr 6, 2021
}
else {
Copy link
Member

@LordAro LordAro Apr 6, 2021

Choose a reason for hiding this comment

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

Oh wait, codestyle - this and below should be on one line

Copy link
Contributor Author

@btzy btzy Apr 6, 2021

Choose a reason for hiding this comment

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

Fixed.

@btzy btzy force-pushed the no-shared-order-of-1-vehicle branch from 087ab7c to 6a44914 Apr 6, 2021
LordAro
LordAro approved these changes Apr 6, 2021
@LordAro LordAro merged commit f0a24e9 into OpenTTD:master Apr 6, 2021
12 checks passed
LordAro added a commit to LordAro/OpenTTD that referenced this issue Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants