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

Change: Make order window hotkeys toggle for load & unload variants #8669

Merged
merged 1 commit into from Feb 13, 2021

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Feb 13, 2021

Motivation / Problem

Closes #8661
Order window hotkeys for load & unload behave unexpectedly - most of them only set, not toggle

Description

Makes all the variants toggle between the default (load - "load if possible", unload - "unload if possible") and whatever setting is desired

Limitations

  • Transfer orders set "no loading" as well as its unload state, so this also alters load state (change default transfer orders #3905). Might be unexpected (but is unchanged from current behaviour)
  • No changes to the UI. Still only shows the 'primary' load/unload variant on the buttons.

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')

src/order_gui.cpp Outdated Show resolved Hide resolved
src/order_gui.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Feb 13, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8669 February 13, 2021 19:05 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8669 February 13, 2021 19:16 Inactive
src/order_gui.cpp Outdated Show resolved Hide resolved
@TrueBrain
Copy link
Member

TrueBrain commented Feb 13, 2021

Basically, before this PR:
When you selected any load option, the button/hotkey would first remove the load option; on next click select the value.

With this PR:
Clicking the button/hotkey now always sets load to what-ever the hotkey does / button says.

This makes a lot more sense to me, especially as the button isn't pressed if you select anything not on the button.

In general, really funky mechanics here, but okay .. that is old news :D

@DorpsGek DorpsGek temporarily deployed to preview-pr-8669 February 13, 2021 19:38 Inactive
@TrueBrain
Copy link
Member

Some more testing, two observations:

  1. if you don't have a load selected, and you click in the dropdown the top entry, you get an error.
  2. if you select any other, you open the dropdown again and select it again, it unselects.

Both changes are introduced by this PR. 1) is a bug, and about 2) I am not sure that makes sense. It feels a bit odd, honestly. Wouldn't it be better to only have this behaviour for hotkeys, and not do this for the dropdown? Would be easy enough in the code by adding a parameter to "force" or to "toggle" (depending if you make the dropdown or the hotkey the exception).

Was 2) intended, and what do you think about it yourself?

@DorpsGek DorpsGek temporarily deployed to preview-pr-8669 February 13, 2021 19:57 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8669 February 13, 2021 20:02 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8669 February 13, 2021 20:07 Inactive
@@ -1262,15 +1244,15 @@ struct OrdersWindow : public Window {

case WID_O_FULL_LOAD:
if (this->GetWidget<NWidgetLeaf>(widget)->ButtonHit(pt)) {
this->OrderClick_FullLoad(-1);
this->OrderClick_FullLoad(OLF_FULL_LOAD_ANY);
Copy link
Member

@TrueBrain TrueBrain Feb 13, 2021

Choose a reason for hiding this comment

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

these should be toggle=true too I think (and the one with UNLOAD too, ofc :P)

@DorpsGek DorpsGek temporarily deployed to preview-pr-8669 February 13, 2021 20:26 Inactive
@LordAro LordAro merged commit f1fc083 into OpenTTD:master Feb 13, 2021
@LordAro LordAro deleted the bug8661 branch February 13, 2021 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Orders not showing correctly in the UI
3 participants