[patch] "transfer" with shift, "go via" with ctrl+shift

[Eagle_rainbow]

Hi 3298,

I need you to test something. 'Cause this patch is quite trunk-ready now, at least if I caught all the bugs. The big new thing I coded this weekend was that setting to bring back the old window.

Great work! You have done it even so perfectly that I had to look twice for the differences when toggling around with the setting option. Having tested around a bit, I found two things which feel a bit strange to me:

• The "Go non-stop to" button changes to "Go non-stop via", if you select this kind of command. However, it doesn't change the label (or more precise: it changes back to "Go non-stop to", not being lowered), if you select "Go via". Comparing with the other choices this behaviour feels odd.
• There still seems to be something fishy with your quick_goto and conditionals: Click on "conditional goto", select one entry. An new order item is inserted. The cursor still is in "goto" mode, but the "Goto" button is not lowered. Yet, if you click on a station, no new item is inserted.
  My expectation would have been here that the "goto" button is lowered and the click on the station would lead to a new item in the list (same behaviour as on "goto nearest station"). Yet, you could argue that a conditional order should not trigger "quick goto", as you most likely want to adjust the conditions of the order. In that case the cursor should have the proper mode. Which of the two variants is the better one, I don't know, as I am not using conditionals after all.

I've pondered about merging our patch with adf88's (especially his first half, permitting to use Shift in hotkeys issuing DoCommands), because then better hotkeys would be possible (example: K was unload, I assigned Ctrl+K to No Unload; that should have been Shift+K, and Transfer - currently Ctrl+U - should have been Ctrl+K instead). What do you think?

To be honest, I wouldn't do it right now. Doing it would risk your good stability. Moreover, I am a person who likes "less keys", so CTRL+K is even one key too much for me That is why the first thing which I did on the patch was to change the hotkey of "Transfer" to "T". Yet, having asked for Shift+Click in the beginning of this thread, I surprisingly got used to it in the meantime.
The merge of this patch with "adf88's Shift-DoCommands patch" can still be done afterwards as well: It then will just provide the capability to also allow SHIFT as a modifier for hotkeys. In my mind, the focus of this (series of) patch(es) currently is the provide hotkeys for all commands in the order window (also ahead of clicking) and to rework the order view (including a advanced configuration switch). And that's what it currently does well! It really provides a benefit in my eyes - and that's the reason why I think it belongs to trunk now (modulo bugfixing).

[3298]

The "Go non-stop to" button changes to "Go non-stop via", if you select this kind of command. However, it doesn't change the label (or more precise: it changes back to "Go non-stop to", not being lowered), if you select "Go via". Comparing with the other choices this behaviour feels odd.

I apparently forgot something when if-ing out all that stuff for the new layout. Will have a look tomorrow later today after sleeping.

There still seems to be something fishy with your quick_goto and conditionals: Click on "conditional goto", select one entry. An new order item is inserted. The cursor still is in "goto" mode, but the "Goto" button is not lowered. Yet, if you click on a station, no new item is inserted.

Hm, strange. Looks like some forgotten ResetObjectToPlace() or something similar. Another thing to hunt down.

My expectation would have been here that the "goto" button is lowered and the click on the station would lead to a new item in the list (same behaviour as on "goto nearest station"). Yet, you could argue that a conditional order should not trigger "quick goto", as you most likely want to adjust the conditions of the order. In that case the cursor should have the proper mode. Which of the two variants is the better one, I don't know, as I am not using conditionals after all.

The intention was to switch to OPOS_GOTO (or how was it called again?) to facilitate quickly writing down an entire order list by choosing the modifiers first and the destination after that for each order; without quick goto it should switch to OPOS_NONE.
On the other hand, we could have it only fire for the new layout, because in the old layout you still have to change the order modifiers afterwards.

To be honest, I wouldn't do it right now. Doing it would risk your good stability. Moreover, I am a person who likes "less keys", so CTRL+K is even one key too much for me That is why the first thing which I did on the patch was to change the hotkey of "Transfer" to "T". Yet, having asked for Shift+Click in the beginning of this thread, I surprisingly got used to it in the meantime.

Okay, maybe keep it as an option to consider later. T would have been nice if it wasn't already taken by the chat, so I tried to collect all the unload hotkeys on K, just like the goto modes are on G. Though at least U without Ctrl would be free to use. But if we are postponing the DoCommands with Shift, the hotkeys should default to no key so we have the option to set them to a Shift+something combination later without confusing users due to changed hotkeys (how many people actually use non-default hotkeys? I'd guess a dozen or two worldwide).
So first things first: I'll spend a few hours sleeping before squashing bugs.

Edit, a few hours of sleep and a breakfast with three cups of tea later (to get my brain working): It doesn't even compile because I made a function const that really isn't const. What was I thinking? At least the fix for that was trivial.
Edit2: Found and fixed the quick goto quirk. For some reason the exact same code snippet worked for nearest depot, but it failed on conditional orders. I had to swap the assignment this->goto_type = OPOS_GOTO and the call to SetObjectToPlaceWnd because the latter calls OnPlaceObjectAbort which resets the goto_type to OPOS_NONE. Don't ask me why it worked for nearest depot...
Edit3: I've gone ahead and restricted the quick goto changes to the new layout. But before uploading the update I'd like to fix that non-stop glitch, and I can't figure out how to reproduce it. What did you do exactly?

[Eagle_rainbow]

I've gone ahead and restricted the quick goto changes to the new layout. But before uploading the update I'd like to fix that non-stop glitch, and I can't figure out how to reproduce it. What did you do exactly?

mmmh... I can reproduce it here easily by performing the following steps (I am in "new design mode"):

• Create an order list as usual.
• Reopen it.
• Click on an order.
• Use the Dropdown button for "Go non-stop to".
• Select "Go via" from the list.
• Observe that the order has changed accordingly, but the button has changed back to "Go non-stop via" and is not lowered.
• Use the Dropdown again.
• Select "Go non-stop via" this time.
• Observe that the button now is lowered (and "Go non-stop via" is shown).
• Use the Dropdown again.
• Select "Go non-stop to" this time.
• Observe that also here the button is lowered and the label has changed accordingly.

So, there is "something special" with "Go via" which shouldn't

Are our states of patches still consistent with each other? Not that I make you chasing a phantom...

[3298]

I followed your procedure, and I observed the same results. But what I see is working as intended. The Non-stop button shall behave like this:

• The text shows either non-stop to or non-stop via. This shows whether the vehicle will stop at the destination or drive through.
• The button is lowered if the vehicle will stop at stations on the way to the destination.

This is as consistent with the other dropdowns as I could manage: the button is lowered if a non-default option is selected, and it shows the text of that non-default option; otherwise it shows the text of the suggested non-default option without being lowered. For the load, unload and service dropdowns the defaults are "Load if available", "Unload if available" and "Always go", the respective suggested options are "Full load any cargo", "Unload all" and "Service". The non-stop dropdown is special because it has two default options: "Go to" and "Go via". Each of them has its own suggested option where the non-stop flag is enabled. This is all due to the fact that the options in that dropdown are the combinations of two related, but still sort-of independent flags: non-stop and via. One old hotkey toggles the non-stop flag, and one new hotkey toggles the other one.
In trunk the button is lowered if the non-stop flag is enabled; I only added the changing text. It's easy enough to make "Go via" and "Go non-stop via" non-default non-suggested options, but I'm not sure whether that's better. At least I'm glad this "bug" wasn't actually a bug, but a difference between our expectations.

Edit: And that means I can upload the new version without worrying about uploading something with a known issue. It's just the fixes I already did last weekend, plus a rebase on top of current trunk. (Not that this rebase should matter, it's just a few different hashes in the diffs, where it says "index <something>..<something> 10644".)
In case you want to reorganize the changes or nag the devs or whatever, I'll dump the commit messages from my repository here to guide you through the individual diffs. I tried to mimic the devs' commit message style.

Code: Select all

-Codechange: add the option to return a result from the refit window to a parent window without a DoCommand
-Codechange: move some code setting an order's DepotOrderType and DepotActionType from a DepotAction into a separate function
-Codechange: move GetOrderCmdFromTile from static scope into struct OrdersWindow
-Codechange: introduce member variables in OrdersWindow holding the order modifiers for new orders
-Add: alternate order window with different button layout so that all goto order modifier buttons can be shown simultaneously
-Change: make the order modifier dropdowns in the alternate layout show the current order modifiers as texts on them
-Feature: allow selecting order modifiers before choosing the destination in the alternate order window
-Add: some more hotkeys for the order window
-Change: switch to goto mode when a nearest depot or conditional order has been inserted in the new order window layout and quick goto is on

[Eagle_rainbow]

It's easy enough to make "Go via" and "Go non-stop via" non-default non-suggested options, but I'm not sure whether that's better. At least I'm glad this "bug" wasn't actually a bug, but a difference between our expectations.

At least I would vote to do so. With the new order window layout this feels unlogical. But I do understand that it would break compatibility, so I can also live with it as it is right now.

Code: Select all

[...]
-Add: some more hotkeys for the order window

Mmmh, without wanting to nag at you, but perhaps it might be worthwhile making this commit short summary a bit more telling What do you think about

Code: Select all

-Feature: Hotkeys for order modifiers goto, loading, conditional, transfer and servicing

[adf88]

I will no longer maintain this patch, it has a successor: [patch]Run-to (fast go-to order creation)