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

Jump to bottom

Fix: Buttons in order window are disabled, but not raised #5845

Closed
DorpsGek opened this issue Dec 31, 2013 · 3 comments
Closed

Fix: Buttons in order window are disabled, but not raised #5845

DorpsGek opened this issue Dec 31, 2013 · 3 comments
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

3298 opened the ticket and wrote:

Steps to reproduce the issue:

  1. Get a train or road vehicle with a station order.
  2. Select the station order in the order window.
  3. Push the Full Load and/or the Unload button.
  4. Select Go Via or Go Non-stop Via from the Non-stop dropdown.
    Result: The button(s) pushed in step 3 are still lowered, but disabled because the vehicle does not stop at the destination station.
    My suggested fix is to modify line 1405 of src/order_cmd.cpp so it forces the load and unload options (in addition to the refit cargo, which is already forced to CT_NO_REFIT) to their default values if the vehicle does not stop at the destination.

For convenience, I attached the diff of the suggested fix. Other methods to fix are raising the buttons while keeping the order untouched, or enabling them like the refit button when a refit is set but now impossible due to a changed consist, but this fix is consistent with how the refit is handled when switching to Go Via.

Attachments

Reported version: trunk
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/5845
@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 1, 2014

3298 wrote:

I noticed another related inconsistency in the behavior of the order window: When changing the variable of a conditional order to Unconditionally the value is reset to 0 and its button is disabled, but when changing the variable to Requires Service instead, the button is disabled only, the value is not reset. I fixed it locally by changing CmdModifyOrder to, well, reset the value when OCV_REQUIRES_SERVICE is chosen.
Also in CmdModifyOrder I found two cases where CMD_ERROR should be returned, but isn't. MOF_COND_DESTINATION makes no sense for station orders, but was allowed, and with a conditional order that has the variable OCC_REQUIRES_SERVICE changing the value was possible. While that does not hurt because the value is not checked for this condition, it still should be forbidden.
You find all three fixes in the diff I'm attaching.
Edit: Apparently I messed up when editing useless stuff out of the diff, and I can't figure out how to replace an attached file. Seems to apply with -p0, though - usually my patches are git-style (-p1) patches.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5845#comment12884

@DorpsGek
Copy link
Member Author

3298 wrote:

Yesterday I played with the MOF_COND_DESTINATION bit of the fixes. I'm able to exploit it with a modified client such that everyone trying to view the tainted order in the order window crashes. The timetable window is fine because it doesn't display the load/unload modifiers (which is the part of the order I can reach; Order::flags stores the conditional jump destination or the load/unload flags, depending on the order type). The game logic seems to be robust enough to simply ignore the invalid parts of the order modifiers, so starting the vehicle with the broken order is harmless.
This missing validation originates from r13752, when MOF_COND_DESTINATION was introduced.
Attached to this comment is a proper git-style diff containing exactly the same stuff as the orderbuttonissue.diff and orderbuttonissue2.diff combined.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5845#comment13085

@DorpsGek
Copy link
Member Author

frosch closed the ticket.

Reason for closing: Fixed

in r26357. Thanks for the patches!

This comment was imported from FlySpray: https://bugs.openttd.org/task/5845

@DorpsGek DorpsGek added component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek