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

[3298]

Regarding additional buttons: a quick look at the code revealed that the list of additional (dropdown-)buttons I would have to introduce is smaller than I feared. The non-stop stuff (not visible for ships and aircraft) does not need to be duplicated, as does the refit stuff. This leaves just one button (service) with its dropdown. (Should have found that earlier, that shows I didn't play OpenTTD much lately. ) Maybe squeezing all modifiers into one row isn't that bad after all.

In my opinion the popup (I assume you mean a popup like the bridge one) is bad. adf88 expressed his concerns about that one already a few posts before, I don't need to repeat that.

Yes, that is quite close to what I meant. However, besides the judgement that it is "wired" (by adf88), which I underline, if used with a "long click" (sounds very "tabloid"ish to me ), I have not heard your arguments yet why you think that way. Especially in regards that the popup/context menu is already known to the player (for selecting the bridge type for example) and combining it with the CTRL modifier key to enable it at all, I slowly start feeling comfortable with the idea again (though not being convinced altogether yet). Moreover, this paradigm is also known to the experienced user - see what happens if you want build a station directly next to another one, but it should not be a joint one. Whilst it also solves our question on which type of object the user has already clicked, the selection is not that far away from the pointer of the mouse. Also from other programs you may see such a UI paradigm, for example if you are asked to specify how you want to paste your copied data ("only text", "with formatting", ...).
Could you put a bit beef around it, why both of you dislike the approach?

Okay, as you wish: the popup introduces an additional click/keypress compared to the selection of modifiers from the order window before selecting the destination or the (CTRL+)SHIFT keys in adf88's patch. And putting it on CTRL+Click in place of an already existing shorter shortcut isn't something I would want either. The bridge selection feels a bit clunky to me too, I would rather want something like the signal GUI for them, with an error message if the chosen bridge type doesn't support the length for my bridge. For station joining it seems unavoidable, so I will live with it.

On a related note, hotkeys do their actions when you press the key, not when you release it, and without repeating. Treating the order modifier hotkeys as keys you hold while clicking the destination would do no harm as the effect is exactly the same as shortly pushing the key before selecting the destination.

As far as I know, there is no code in OpenTTD to handle long clicks. Actually, I have encountered only very few applications on non-touchscreen devices that can handle long clicks. I know Borderlands 2 has it, and on my HP graphing calculator with its overloaded keyboard and the possibility to configure the keyboard assignments there is a 3rd-party library that adds this and double clicks to make even more functions accessible without menus. Double clicks are available at least in the save/load windows, but I wouldn't write something relying on double/long clicks of these if I can do without them. Just personal preference, though.

[Eddi]

if you're rearranging the GUI, maybe consider splitting the "go to/via" from the "non-stop/with stop" dropdowns

[adf88]

Basically we have two related problems:

1. Fast and mistake proof creation of orders of different types
2. Easy editing of already created order lists

I think most of us will agree that editing orders by the drop-downs is an uncomfortable task. This should change and the GUI need to be redesigned. Making the list more easily editable (2) will have positive impact on order creation (1). Thus in the first place I would give a try to a nicer order list editing. This could be enough. KISS.

With the new GUI, process of creating orders will be easier. But not ultra fast like some desire. Perhaps the new GUI needs just a simple complement with some (hot)key solution. Preferred is a one which doesn't require a new key modifier (e.g. shift) and doesn't introduce many new keys. I was thinking ... if this needs to be just a complement to already nice solution, maybe just allow to assign few user-defined order types (those used mostly) to some few (hot)keys. We have two (hot)keys already (click and CTRL+click), two more actions would be enough. I have a nice idea on how to modify the hotkeys. CTRL+GOTO can open GOTO tool in alternate (user defined) mode. As simple as that . Cursor would be different a little, each order type can have it's own sprite. Additionally CTRL+wheel could switch modes while the GOTO tool is opened.

[3298]

Yes, I know, I am a few days late, but I finally got around to implementing my proposal so you guys can test it.

if you're rearranging the GUI, maybe consider splitting the "go to/via" from the "non-stop/with stop" dropdowns

And make the order window even wider? No, sorry, that's what I want to avoid. The combination of non-stop and to/via is not optimal, but I don't plan on changing it.

My modified order window.

newordergui.png (2.93 KiB) Viewed 5140 times

This is how my order window looks. As you can see, it is not exactly small, and it gets even wider when the texts on the buttons are longer. While testing with the default configuration (screen size=640x480) I noticed that with German texts it gets wider than 640 pixels.
The modifiers can be selected before the destination is chosen. In the screenshot I selected "Service", so when I choose a depot after that, a service order is inserted. (Of course, Ctrl+Click still works. It overrides the modifiers chosen in the window.) When clicking something else (e.g. a station), the "Service" modifier will be discarded.
In the screenshot you can see that the refit dropdown is disabled. This is something I'm still working on - I need to convince the refit window to not issue a DoCommandP when the goto tool is active, but to somehow return the selected cargo type to the order window. The next issue with refitting is depot refit vs. autorefit - some vehicles only allow refits in the depot, so the cost estimate in the refit window may be off in some cases because during selection the game doesn't know whether the refit will be at a station or in a depot. I think I'll just give the depot estimate and accept that it may be wrong - you usually don't mix and match autorefit and non-autorefit cars in a train, so the refit would just be discarded when a non-autorefit train gets a station order.

Anyway, here is the patch for my interpretation of "make order creation faster" in its current state.

[Eagle_rainbow]

Anyway, here is the patch for my interpretation of "make order creation faster" in its current state.

As promised, I just played around with your patch a bit. Also with the hotkeys, the stuff is working fine for me and would solve the majority of my points. The width of the window is no problem for me, as I am always playing on a desktop machine with high res. However, I agree to your concern that it will raise trouble for low res devices. My ideas to address this currently are going into the direction to reposition the "non-stop" button to the left of the "go-to" button, thus having two lines with four buttons each. However, if that is such a brilliant idea, I don't know - I am also using this feature very seldom, so perhaps I am not the best person to judge that.
One thing which I came across is that when using the hotkey for "transfer", the unload button does not read "transfer" and is also not lowered. That happens as well, if I had selected the transfer modifier via mouse. As you do not get any feedback from the UI, it feels a bit clumsy and you don't know if you clicked it properly or not. You only get to know that everything worked, as soon as you created the order (by clicking on a station, for instance).
Moreover, what currently gives me a pain is the fact that today's "transfer" is no longer the old "transfer" which I was used to - as I assume that it was changed some time before, I expect that this is not related to your patch: Transfer is "loading transfer" today and defaults to "full load". However, as transfer is mostly being used for feeder services, this put things upside down for me. Defining a second hotkey is possible (via "no_load" in hotkeys.cfg) and also solves that, but my initial attempt was to reduce the number of buttons/keys to press, and not to increase it So, in general, I would really appreciate to see another hotkey option for the combination "transfer and no load" in hotkey.cfg -- but that'll be another patch to request and shouldn't be "a big thing".

If you want me to also have a look at the coding side, just let my know (I did not read through it, just tested the patch at runtime).

[3298]

Well, I simply forgot to tell my patch to set "no load" even when "transfer" is chosen before the destination click. Modifying an existing order is fine.
Changing the button texts is a good idea, I think I'll do that.
The code is not finished yet, refit is not yet working the way I want it to, so you don't need to look at it now, maybe later.

Edit: Still no changing button texts, but the refit seems to work now, the transfer has been fixed (now selects no load as well), and the non-stop button has been moved to the bottom row, as you suggested, just to the right of the goto button instead of to the left.

[Eagle_rainbow]

Edit: Still no changing button texts, but the refit seems to work now, the transfer has been fixed (now selects no load as well), and the non-stop button has been moved to the bottom row, as you suggested, just to the right of the goto button instead of to the left.

For me the "via"-button in the second row looks better to me. I now played approx. 1h with the (new) patch and did not stumble over anything major. Yet, the missing texts are making things more error-prone, but it's already much better than before (without the patch)... In short: I think we are on a good track

[Eagle_rainbow]

Hi folks,

just found some time to work on OTTD again a bit. As a start I updated this patch to the most current svn revision (i.e. 26184). Only minor modifications were necessary. No changes in features of the patch. This means that

Still no changing button texts

is still missing.

[Eagle_rainbow]

Still no changing button texts

I think I managed to get that one also addressed with the patch attached to this post.
Moreover, this apparently fixed an another issue which always puzzled me since long, but I couldn't describe what it was: I always had trouble to understand what I did when having used the order modifier buttons. The lowering state of the buttons never showed what I expected. Having had a closer look at the coding now, I noticed that apparently the coding was not updated on having multiple types of loading/unloading. Thus, only half of the cases were covered properly, and only "occasionally" the lowering of the buttons has been done properly. As said, I think that issue is also fixed with this patch.

For those of you who want to test the patch, here's how to enable it (after applying and compiling ):
Edit your hotkeys.cfg file. In section "order" you may now define three additional identifiers:

• transfer
• no_unload
• no_load

In short, you may press the keys you define there before you click on the next station to create your order. Though, using them, when having selected an order in the order_gui, works as well and modifies the loading/unloading state.

I'd appreciate if someone else could also test it as well.
NB: >90% of the credits go to 3298

[3298]

Oh, thanks for updating it. It always feels good when someone else appreciates your own patch enough to keep it updated.
I found some free time now and resumed work on this. For consistency I want to extend the changes to conditional orders. It just managed to compile, and it didn't crash yet, but I found some bugs, so I'll keep it for myself until those are squashed.
On the way I rewrote some parts of the patch, so I don't know whether I fixed that issue over here, but in case I didn't, could you tell me what you changed so I can copy the fix?
The button text part is still not done (shame on me ), but if I'm right we'd just need to throw some SetTooltip calls into UpdateButtonState, similar to how text and tooltip of the refit/autorefit dropdown change. Well, that and keeping them pushed when a non-default option from the dropdown is active, which then makes it necessary to change OnClick to switch back to default if the button is pressed, not just when the option corresponding to the text shown on the raised button is active. We could (maybe?) also turn them into normal dropdowns like the condition variable and comparator, what do you think? Or maybe replace the action for clicking the raised Unload button to Transfer because that's probably used more often?

[Eagle_rainbow]

Oh, thanks for updating it. It always feels good when someone else appreciates your own patch enough to keep it updated.

You are welcome. I am interested in getting this thing into trunk - sooner or later. And for this, we need an up-to-date version of the patch - otherwise it won't make it at all.

I found some free time now and resumed work on this. For consistency I want to extend the changes to conditional orders. It just managed to compile, and it didn't crash yet, but I found some bugs, so I'll keep it for myself until those are squashed.

Do so. And when you are ready, just post the new version of the patch here. I'll then have a second look at it and will test it.

On the way I rewrote some parts of the patch, [...] could you tell me what you changed so I can copy the fix?

It wasn't much, what I did. But it took me several hours to find the root cause, which - as usual - was trivial at the end (i.e. only some lines with a logical error) As this is just an operational issue, I'll send you the delta, which I did, as diff via PM. Then you may incorporate it into your newest version at will.

The button text part is still not done (shame on me ), but if I'm right we'd just need to throw some SetTooltip calls into UpdateButtonState, similar to how text and tooltip of the refit/autorefit dropdown change.

No shame on you
I also had a look at that part of the topic and tried to implement the button change text thingy -- and came to the conclusion to not do it (yet). The background is the following: The Strings for the label text of the button and the drop down Strings are physically not the same. That is to say: someone intentionally has made them different. As user, we only get puzzled, as the text on the label is the same as one of the commands - by chance. Having seen this, I suddenly knew why I always was confused when using the buttons.

[...]We could (maybe?) also turn them into normal dropdowns like the condition variable and comparator, what do you think?

I think this would be a good idea: currently, it's a button with a dropdown attached, but it better should be a classical dropdown, as some sort of state is compulsory to every single order anyway. However, I think that this would be a "bit larger wheel to turn". I personally feel that this is "another, new feature" - and I have learned here that mixing multiple features into one big patch is close to a guarantee not to get into trunk Thus, I would recommend to postpone this topic for a minute, fix the "old issues" and better to provide a second patch on top of it which then deals with the button texts/replacement with a dropdown.

Or maybe replace the action for clicking the raised Unload button to Transfer because that's probably used more often?

I don't think so: The modifiers are inherent to any kind of order. I always had problems with it that they behaved like buttons - so let's better replace them with "classical dropdowns". I think that's better in the end.

PS: @All: Warning - I just detected that my latest patch versions above somehow break conditional orders. I will have a closer look into it soon, but as you, 3298, are working on an adjustment there anyway, I don't know how much value my efforts will have.
EDIT: Found the issue - it's a merge clash with r25894 where OrderClick_Conditional() and OrderClick_Share() have been integrated into OrderClick_Goto(). It appears to me that this is just a problem with my patches above. I will try to fix it and come back.

[Eagle_rainbow]

EDIT: Found the issue - it's a merge clash with r25894 where OrderClick_Conditional() and OrderClick_Share() have been integrated into OrderClick_Goto(). It appears to me that this is just a problem with my patches above. I will try to fix it and come back.

Fixed. Find a new patch version attached.

@3298: When merging with the current trunk branch, you may stumble over the same issue as well (if you not already did ). The issue is with

Code: Select all

void OrderClick_Goto(OrderPlaceObjectState type)

where the stuff

Code: Select all

	this->ToggleWidgetLoweredState(WID_O_GOTO);
	if (this->IsWidgetLowered(WID_O_GOTO)) {
		SetObjectToPlaceWnd(ANIMCURSOR_PICKSTATION, PAL_NONE, HT_RECT | HT_VEHICLE, this);
		this->goto_type = OPOS_GOTO;
		this->InitOrderModifiers();
	} else {
		ResetObjectToPlace();
	}

has now moved to OnClick(). Yet, mind that

Code: Select all

this->InitOrderModifiers();

did not move (as it is being introduced with our patch).

[3298]

Looks like my stuff is working now. I had no problems with the changed OrderClick_Goto because when I migrated to Git, this change had just been committed. I did some experiments, but left the branch in an unfinished state. I returned to it no sooner than yesterday, and it's still too messy to share the entire branch.
Coding on Dec 31 ... this reminds me of a contest several years ago (was it 2009? don't even remember accurately) where I helped my brother get his prototype of a graphing calculator game into a working state - the deadline was midnight. But meh, the firework starts no earlier than that.

About the dropdown behavior: There is a default action (e.g. Load if available), a suggested non-default action accessible with a single mouse click (e.g. Full load any), and some other options. A normal dropdown that opens even if you hit the button would increase the necessary mouse clicks for the suggested action to two, so I'd keep the button+dropdown. Especially with touchscreens the click count matters. (Though I don't plan on getting a touchscreen device. )
When heading for trunk, this should get an advanced option to revert to the old window layout because players are used to it and might dislike the new layout. Unfortunately that means quite some bloat to support old stuff.
I also fixed a small trunk issue which I will report on the bugtracker soon have reported on the bugtracker. This fix breaks multiplayer-compatibility with trunk. Of course the patch is still multiplayer-compatible with itself. Well, you had to change the version information to make trunk and patched build accept each other in multiplayer sessions anyway.

The changing button texts are not optimal due to the difference between the button text and the dropdown texts in trunk, but it works. Of course you are free to change everything, i just coded my view of an order window enhancement. By the way, STR_ORDERS_GO_TO_BUTTON is unused, but I didn't remove it yet to keep the diff small.

Edit: I found a couple of small issues, the smallest of which originates from trunk. (Yes, another one.) The file attached to this post has been updated.

[Eagle_rainbow]

I also fixed a small trunk issue which I will report on the bugtracker soon have reported on the bugtracker. This fix breaks multiplayer-compatibility with trunk.

Just for housekeeping: 3298 raised FS #5845

[Eagle_rainbow]

Looks like my stuff is working now.

Yep, mainly I can confirm. I just found an issue: Define a new conditional order; thereafter the cursor is in the wrong state (appears to be in Goto state, but the mode is not active).

Coding on Dec 31 ... this reminds me of a contest several years ago (was it 2009? don't even remember accurately) where I helped my brother get his prototype of a graphing calculator game into a working state - the deadline was midnight. But meh, the firework starts no earlier than that.

And how was the coding? Typically, I write the best coding after 10pm

About the dropdown behavior: There is a default action (e.g. Load if available), a suggested non-default action accessible with a single mouse click (e.g. Full load any), ...

Ok, agreed.

When heading for trunk, this should get an advanced option to revert to the old window layout because players are used to it and might dislike the new layout. Unfortunately that means quite some bloat to support old stuff.

I see what you mean. I started to do this by splitting up the diff into smaller chunks:

• Get RefitWindow ready
• Isolate SetDepotAction
• Move GetOrderFromTile from static scope to class OrdersWindow

Now comes the big part and there I am currently uncertain what is the right approach: Better to try integrating it in the existing coding (which leads to many if statements) or to better derive OrdersWindow and depending on a central if-condition in ShowOrdersWindow() decide which one to show.

The changing button texts are not optimal due to the difference between the button text and the dropdown texts in trunk, but it works. Of course you are free to change everything, i just coded my view of an order window enhancement. By the way, STR_ORDERS_GO_TO_BUTTON is unused, but I didn't remove it yet to keep the diff small.

Yep, saw it in the coding. I think it would be the fifth patch in the patchset to perform the cleanup then. But this is then just texts...

[3298]

I just found an issue: Define a new conditional order; thereafter the cursor is in the wrong state (appears to be in Goto state, but the mode is not active).

That's one of the issues that should be addressed in my ninja-update. Did you download the patch before or after that? Anyway, throw an UpdateButtonState() before the else of the if (_settings_client.gui.quick_goto) in OrderClick_NearestDepot (yes, that had the same issue as well) and in a similar-looking place somewhere near the beginning of OnClick. I don't know whether the SetObjectToPlaceWnd was already there in the broken version, but it should set the cursor mode to the goto one (HT_RECT | HT_VEHICLE).

And how was the coding? Typically, I write the best coding after 10pm

Not bad, I found and fixed some issues, then I made that ninja-update. Usually my code quality is not time-dependent, but increases if I have a cup of tea nearby. Preferably a particular east-Frisian one. No, coffee doesn't help at all, for me it's tea.

About the dropdown behavior: There is a default action (e.g. Load if available), a suggested non-default action accessible with a single mouse click (e.g. Full load any), ...

Ok, agreed.

Small addendum: The non-stop dropdown doesn't behave like that, it's more like two dropdowns with two options each merged into one, where the button toggles between the values for one of them. I just hope the texts displayed on the button help explain that to players. But it's been special that way before.

I see what you mean. I started to do this by splitting up the diff into smaller chunks:

• Get RefitWindow ready
• Isolate SetDepotAction
• Move GetOrderFromTile from static scope to class OrdersWindow

Now comes the big part and there I am currently uncertain what is the right approach: Better to try integrating it in the existing coding (which leads to many if statements) or to better derive OrdersWindow and depending on a central if-condition in ShowOrdersWindow() decide which one to show.

I think you can go further than that before diverging from trunk behavior. The OrderWindow class gets some private variables for the order modifiers; they are initialized when the corresponding goto tool is selected, and they are used when creating the order, which is in GetOrderFromTile for normal goto orders (that's the entire reason to move it, it needs access to the private variables) and in the first part of OnClick (before the DoCommandP) for conditional orders. If there's now way to modify the variables, the new functionality is not yet available. I think it's best to keep a single OrderWindow class, but probably with different widget sets for the different layouts. On that topic, the train/rv and ship/plane widget sets could be merged into one with the new layout, with just a NWID_SELECTION to choose whether the non-stop dropdown should exist. I doubt NC_EQUALSIZE likes disappearing widgets, but we could just swap the entire row with the NWID_SELECTION, like it's done for the other row to switch between normal and conditional order buttons.

I think it would be the fifth patch in the patchset to perform the cleanup then. But this is then just texts...

That's more patches than my messy branch, I'm coding a bit more feature-oriented (implement something, fix all the broken things), and devs don't like deliberately broken revisions, so everything ends up in a single commit. Oh, and "just texts": look at what files were changed in r26192 (=git commit 10e5681, "-Fix (r24878): off-by one error ")... @Rubidium: Good choice on the commit message, I like it.

One more note to moderators: Because this is strictly not on the topic of adf88's patch anymore, it might be appropriate to split some posts off to a new topic. My post from Oct 13 looks like a good split point, but on the other hand it was born as an alternative implementation. It's up to you to decide.

[3298]

That's it, the patch is split up following Eagle_rainbow's suggestion. Here is a list of what each part contains:
1: the fix I posted as FS #5845 (both of those tiny patches merged into one)
2: some enhancements for quick goto - switch to goto after nearest depot or conditional order creation.
3-5: the codechanges Eagle_rainbow listed, in exactly the same order.
6: introduce order modifier member variables in OrdersWindow
The last three parts need some work because the setting to switch to the old menu is still missing.
7: reorder the buttons at the bottom
8: display the texts for the current order modifiers on their dropdown buttons (todo: mess with the strings)
9: finally allow write access to the variables introduced in part 6, i.e. enable the faster order creation possibilities.

Parts 1 and 2 may be unnecessary, but both are actually connected to the rest. Omitting 1 would lead to an inconsistency where the user-visible fixes are still present when selecting order modifiers before the destination, and omitting 2 gives you two small rejects because the code blocks added in 2 get an additional UpdateButtonState call in 9 to make the buttons update properly after nearest depot or conditional order creation with quick goto enabled. In my opinion these excuses are sufficient for their presence in this patch series.

Before you try it: Merging the widget sets into one with a selection widget involves making unique IDs for all widgets inside the selection - if one part has a WID_O_GOTO in it, the other one may not have it, otherwise you get strange results. I expected GetWidget, SetWidgetDisabledState, SetWidgetLoweredState and friends to intelligently choose the visible one, but no, they will always take the same even if it's hidden. Giving the copies different IDs and going through all the places where the widgets to be swapped are referenced (adding code to choose the visible one) is not worth the trouble IMO. But meh, even with old or new layout, groundvehicle or ship/aircraft we still have just four possible combinations in addition to the button-less one for foreign companies. Not the end of the world, just add them.

I hope this patch archive helps interested developers (mainly Eagle_rainbow, of course). There should be no user-visible changes compared to the last version.

[3298]

I feel like I'm talking to myself here. Eagle_rainbow, where are you? 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. There are a few subtle differences, though (for example: for waypoint orders only "go via" and "go non-stop via" are visible in trunk, the other entries aren't visible at all; with the old window in the patch the other options are just grayed out).
I also fiddled with the hotkeys. There are two new ones (go_via and conditional, guess what they do), the existing ones are now all bound to actual keys (wasn't exactly a pretty thing, I wanted to have some key combinations with Shift, but all Shift-hotkeys triggering a DoCommand will just tell you that it costs $0 without doing anything - I guess adf88's patch in the first post of this thread would help with that), and I made all the mode hotkeys toggle between two modes instead of only some (the remaining keys, for example goto and transfer, didn't do anything when their mode was active already). Of course these changes affect both the old and the new window.
And then there are two more changes. 1. FS #5845 has been accepted recently, so I could drop it. (On that topic, I said something about breaking multiplayer compatibility; that was actually wrong according to some wiki article about OpenTTD's network code.) 2. I moved the changes to the quick goto behavior (when quick goto is on, switch to goto mode after a nearest depot or conditional order has been inserted; also affects the old window) to the end of the patch series to make omitting it easier.

The setting to to switch between the old and new window is right below quick goto, but it is not visible when filtering for basic settings, you need to set advanced or expert/all settings in the filter. It defaults to the old window.

[Eagle_rainbow]

Hi 3298,

I feel like I'm talking to myself here. Eagle_rainbow, where are you?

Sorry for being absent for quite some time, but my job has been (and partly still is) eating me up, so I couldn't find the time continuing on this patch, yet. Though, I still feel committed to it. I will try to free up some spare time (hopefully this weekend?) and have a closer look at it...

Will revert back to you as soon as time permits!

[3298]

No problem, I've been busy as well. I tend to have a few minutes of spare time scattered all over the day (my way to the university includes 20 minutes on the train and 20 more on the tram, that order of magnitude), but usually not enough to seriously work on a patch. And looking back, I should have inserted a newline after the first sentence; the "talking to myself" was directed at my triple-post. (Editing wasn't possible anymore. )
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?
Oh, and if there is another player out there using this patch, some feedback would be great. What doesn't work / could be improved?