[WIP] Change: add support for next/previous railtype global hotkeys #7851

grossws · 2019-12-01T22:57:13Z

This patch adds support for next/prev railtype global hotkeys which are basically autorail + special modifier which selects previous or next available railtype.

Such behaviour is useful for players who use PURR, Useless Tracks and similar NewGRFs for quick selection.

I use PURR with semantic tracks coloring (e.g. white purr for waiting bays, red purr for prio zones etc).

Current patch binds this feature to GLOBAL+[ and GLOBAL+].

UPD If user explicitly selected some widget in railway build toolbar it won't switch to autorail.

Signed-off-by: Konstantin Gribov grossws@gmail.com

src/widgets/rail_widget.h

nielsmh · 2019-12-02T12:15:24Z

This needs to be tested with keyboard layouts where the [ ] characters are on AltGr combinations with the number row. (Several continental European layouts.)

grossws · 2019-12-02T21:22:21Z

@nielsmh, I tested it on Linux w/ SDL2 and default German layout where [/] are on AltGr+8/9. Hotkeys work as expected when PR #7850 is applied, on same keys where [/] are on US layout.

grossws · 2019-12-07T18:55:25Z

Some additional thoughts about this feature. Currently it switches to autorail but better in UX terms would be to stay with same chosen widget if one is selected.

Then you can just switch railtype on the fly with say converse or depot instead of switching railtype and choosing converse/depot/whatever after that. Behavior when rail building tools are inactive should stay the same: choose railtype and activate autorail.

src/rail_gui.cpp

grossws · 2019-12-24T12:15:51Z

@LordAro, ok, I'll fix things mentioned above. In case of code style I just did by example and got smacked by CI once)

Anyway, I will mark this as work-in-progress since I wish to change some logic (see #7851 (comment))

TrueBrain · 2020-12-24T20:21:09Z

hi @grossws : exactly a year later, as if I planned this! (I did not :D). I was wondering if you are still interested in seeing this PR through? It seems you have a few comments open that needs resolving .. would be cool if you could! Tnx!

andythenorth · 2020-12-24T20:24:38Z

Is it extensible to roadtypes also? :D No problem if not, just curious.

grossws · 2020-12-28T14:06:00Z

@TrueBrain, I can possibly look into this and update PR but after Jan, 7th when I return from vacation. Quite eventful year it was ,)

@andythenorth, didn't look into that, but it may be possible if it has similar implementation. I don't recall different road types at all, could you point what additional GRFs should I have to check that?

grossws · 2021-01-14T00:42:54Z

Possible modification: instead of adding RailToolbarWidgetModifiers enum just add prev/next_railtype to RailToolbarWidgets with explicit constants like 0xfd/0xfe.

Initially I put them into separate enum because I thought that it would be confusing to have WID_RAT_* enum values not represented in toolbar as a button. But caption already exists, so maybe it's not the case.

Also, GHK_MOD_RAT_* became simple special hotkeys and not actually modifiers for WID_RAT_AUTORAIL, another point to putting them into RailToolbarWidgets enum.

grossws · 2021-02-09T16:59:14Z

@TrueBrain, did you have some time to review this PR?

src/rail_gui.cpp

This patch adds support for next/prev railtype global hotkeys which are basically autorail + special modifier which selects previous or next available railtype. If user selected any widget in railway build toolbox it won't switch to autorail but keep current widget instead. Such behaviour is useful for players who use PURR, Useless Tracks and similar NewGRFs for quick selection. I use PURR with semantic tracks coloring (e.g. white purr for waiting bays, red purr for prio zones etc). Current patch binds this feature to `GLOBAL+[` and `GLOBAL+]`. Signed-off-by: Konstantin Gribov <grossws@gmail.com>

LordAro

Go for it

ldpl

I disagree with @LordAro and think this implementation is quite messy and can cause some problems in the future.

Also, I'd expect these hotkeys to loop around tbh, not just stop at first/last railtype.

ldpl · 2021-02-19T09:54:52Z

src/widgets/rail_widget.h

@@ -10,6 +10,15 @@
 #ifndef WIDGETS_RAIL_WIDGET_H
 #define WIDGETS_RAIL_WIDGET_H

+/** Global hotkeys modifiers for #RailToolbarWidgets */
+enum RailToolbarWidgetModifiers {
+	GHK_MOD_RAT_PREV_RAILTYPE = 9, ///< bit added to WID_RAT_AUTORAIL to select previous railtype


Constant prefix (GHK_) doesn't match enum name (RailToolbarWidgetModifiers)

What naming convention would you recommend for it: WID_MOD_RAT_*, `WIDM_RAT_* or something else?

ldpl · 2021-02-19T09:56:40Z

src/widgets/rail_widget.h

+	GHK_MOD_RAT_PREV_RAILTYPE = 9, ///< bit added to WID_RAT_AUTORAIL to select previous railtype
+	GHK_MOD_RAT_NEXT_RAILTYPE = 10, ///< bit added to WID_RAT_AUTORAIL to select next railtype
+
+	GHK_MODB_RAT_PREV_RAILTYPE = 1 << GHK_MOD_RAT_PREV_RAILTYPE, ///< bitmask for GHK_MOD_RAT_PREV_RAILTYPE


Why are you using bitshifting? You're not packing anything else there and only need a regular constant.

I wanted them way out of WID_RAT_* enum values since originally I used them as GHK_MODB_RAT_PREV_RAILTYPE | WID_RAT_AUTORAIL in Hotkey definition.

I didn't want to put them into RailToolbarWidgets since this actions have no corresponding buttons in toolbar itself and are a bit more global.

ldpl · 2021-02-19T09:57:26Z

src/rail_gui.cpp

+	extern RailType _last_built_railtype;
+	RailType rt = _last_built_railtype;
+	bool change_rt = false;
+	if (HasBit(hotkey, GHK_MOD_RAT_PREV_RAILTYPE)) {


Why HasBIt/ClrBit? It's just a regular value so use ==

Ok, they aren't bitmasks anymore

ldpl · 2021-02-19T09:57:52Z

src/rail_gui.cpp

+		ClrBit(hotkey, GHK_MOD_RAT_PREV_RAILTYPE);
+		change_rt = true;
+		do {
+			rt--;


This can underflow if rt is already 0 (aka RAILTYPE_BEGIN)

ldpl · 2021-02-19T09:59:15Z

src/rail_gui.cpp

@@ -793,6 +832,8 @@ static Hotkey railtoolbar_hotkeys[] = {
 	Hotkey('T', "tunnel", WID_RAT_BUILD_TUNNEL),
 	Hotkey('R', "remove", WID_RAT_REMOVE),
 	Hotkey('C', "convert", WID_RAT_CONVERT_RAIL),
+	Hotkey(WKC_L_BRACKET | WKC_GLOBAL_HOTKEY, "prev_railtype", GHK_MODB_RAT_PREV_RAILTYPE),
+	Hotkey(WKC_R_BRACKET | WKC_GLOBAL_HOTKEY, "next_railtype", GHK_MODB_RAT_NEXT_RAILTYPE),


Why are these hotkeys in BuildRailToolbarWindow window that only ever deals with a single rail type? Switching railtypes is a function of main toolbar so hotkeys should go to MainToolbarWindow.

Ok, and what about implementation? Should it stay in rail_gui.cpp?

But are the hotkeys active when the rail toolbar isn't open? I would suggest they act only when the rail toolbar is open, to allow those hotkeys to be used for other uses like switching roadtypes when the road toolbar is open.

ldpl · 2021-02-19T10:01:14Z

src/rail_gui.cpp

+	if (change_rt) {
+		// get previous selected widget from current build toolbar if present
+		auto *w = dynamic_cast<BuildRailToolbarWindow *>(FindWindowById(WC_BUILD_TOOLBAR, TRANSPORT_RAIL));
+		hotkey = (w == nullptr || w->last_user_action < WID_RAT_BUILD_NS) ? WID_RAT_AUTORAIL : w->last_user_action;


You're not copying the bulldozer button state (WID_RAT_REMOVE).

Oh, and also it selects autoraill if no widgets were previously selected.

You're not copying the bulldozer button state (WID_RAT_REMOVE).

Could you elaborate on that? It will use w->last_user_action for any WID_RAT_* except WID_RAT_CAPTION.

And selection autorail was by design, however I can remove it to make it orthogonal. I think select autorail is useful from UX perspective.

ldpl · 2021-02-19T10:05:27Z

src/rail_gui.cpp

+			rt--;
+		} while (rt > RAILTYPE_BEGIN && !HasRailtypeAvail(_local_company, rt));
+	} else if (HasBit(hotkey, GHK_MOD_RAT_NEXT_RAILTYPE)) {
+		ClrBit(hotkey, GHK_MOD_RAT_NEXT_RAILTYPE);


A lot of duplicated code here, only real difference between these branches is loop direction. Pretty sure it can be implemented much simpler. At the very least you can get rid of change_rt (and ClrBit ofc).

I'll think on it. Originally I did it to avoid complex and error-prone logic conditions. If I can reduce it to something simple and easy to follow -- ok)

grossws · 2021-03-06T14:49:03Z

@ldpl, I'll comment on most of the review comment here.

I [snip] think this implementation is quite messy and can cause some problems in the future.

Quite possible. I mark PR as [WIP] again.

Also, I'd expect these hotkeys to loop around tbh, not just stop at first/last railtype.

It was deliberate. First time I tried with wrapping around it was no as good ux as with current behavior. When it saturates at simple rail/last available rail it's easy to press [/] several times and return back to required from there without looking at widget at all.

Thanks for your review and looking forward for your reply ,)

dismiss for now

2TallTyler · 2022-11-09T19:56:06Z

@grossws Are you still interested in working on this? I'm going through the PR backlog to find low-hanging fruit and this is on my list.

grossws · 2022-11-09T20:44:48Z

@2TallTyler, yeah. I'd like some answers for @ldpl's review above about constant naming and in which file implementation should live.

Likely I'll have a bit of time to update the PR and fix issues mentioned above this or next weekend

2TallTyler · 2022-11-09T21:01:19Z

I'd look at other widget enums for naming conventions, and I've seen other hotkey implementations (#10110) stay in the corresponding *_gui.cpp file so I'd imagine that would be fine.

grossws changed the title ~~Change: [add support for next/previous railtype global hotkeys~~ Change: add support for next/previous railtype global hotkeys Dec 1, 2019

grossws force-pushed the change-prev-next-railtype-hotkeys branch from ff01b43 to f29dd26 Compare December 1, 2019 23:18

James103 reviewed Dec 1, 2019

View reviewed changes

src/widgets/rail_widget.h Outdated Show resolved Hide resolved

grossws force-pushed the change-prev-next-railtype-hotkeys branch from f29dd26 to c42996b Compare December 1, 2019 23:52

LordAro requested changes Dec 23, 2019

View reviewed changes

src/rail_gui.cpp Outdated Show resolved Hide resolved

src/rail_gui.cpp Outdated Show resolved Hide resolved

src/rail_gui.cpp Outdated Show resolved Hide resolved

src/rail_gui.cpp Outdated Show resolved Hide resolved

src/rail_gui.cpp Outdated Show resolved Hide resolved

grossws changed the title ~~Change: add support for next/previous railtype global hotkeys~~ [WIP] Change: add support for next/previous railtype global hotkeys Dec 24, 2019

TrueBrain added candidate: yes size: small labels Dec 14, 2020

grossws force-pushed the change-prev-next-railtype-hotkeys branch 3 times, most recently from 5b9e88a to 9f690f6 Compare January 14, 2021 00:29

grossws changed the title ~~[WIP] Change: add support for next/previous railtype global hotkeys~~ Change: add support for next/previous railtype global hotkeys Jan 14, 2021

grossws requested a review from LordAro January 14, 2021 00:43

grossws force-pushed the change-prev-next-railtype-hotkeys branch from 9f690f6 to 6064c3f Compare January 16, 2021 15:09

LordAro reviewed Feb 13, 2021

View reviewed changes

src/rail_gui.cpp Outdated Show resolved Hide resolved

grossws force-pushed the change-prev-next-railtype-hotkeys branch from 6064c3f to 12a00a9 Compare February 15, 2021 12:55

grossws requested a review from LordAro February 18, 2021 14:58

LordAro previously approved these changes Feb 18, 2021

View reviewed changes

ldpl suggested changes Feb 19, 2021

View reviewed changes

grossws changed the title ~~Change: add support for next/previous railtype global hotkeys~~ [WIP] Change: add support for next/previous railtype global hotkeys Feb 19, 2021

2TallTyler added the work: needs more work label Dec 28, 2022

github-actions bot added the work: needs rebase label Feb 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Change: add support for next/previous railtype global hotkeys #7851

[WIP] Change: add support for next/previous railtype global hotkeys #7851

grossws commented Dec 1, 2019 •

edited

Loading

nielsmh commented Dec 2, 2019

grossws commented Dec 2, 2019 •

edited

Loading

grossws commented Dec 7, 2019

grossws commented Dec 24, 2019

TrueBrain commented Dec 24, 2020

andythenorth commented Dec 24, 2020 •

edited

Loading

grossws commented Dec 28, 2020

grossws commented Jan 14, 2021

grossws commented Feb 9, 2021

LordAro left a comment

ldpl left a comment •

edited

Loading

ldpl Feb 19, 2021

grossws Mar 6, 2021

ldpl Feb 19, 2021

grossws Mar 6, 2021

ldpl Feb 19, 2021

grossws Mar 6, 2021

ldpl Feb 19, 2021

ldpl Feb 19, 2021 •

edited

Loading

grossws Mar 6, 2021

2TallTyler Dec 23, 2023

ldpl Feb 19, 2021

ldpl Feb 19, 2021

grossws Mar 6, 2021

ldpl Feb 19, 2021

grossws Mar 6, 2021

grossws commented Mar 6, 2021

2TallTyler commented Nov 9, 2022

grossws commented Nov 9, 2022

2TallTyler commented Nov 9, 2022

[WIP] Change: add support for next/previous railtype global hotkeys #7851

Are you sure you want to change the base?

[WIP] Change: add support for next/previous railtype global hotkeys #7851

Conversation

grossws commented Dec 1, 2019 • edited Loading

nielsmh commented Dec 2, 2019

grossws commented Dec 2, 2019 • edited Loading

grossws commented Dec 7, 2019

grossws commented Dec 24, 2019

TrueBrain commented Dec 24, 2020

andythenorth commented Dec 24, 2020 • edited Loading

grossws commented Dec 28, 2020

grossws commented Jan 14, 2021

grossws commented Feb 9, 2021

LordAro left a comment

Choose a reason for hiding this comment

ldpl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldpl Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grossws commented Mar 6, 2021

2TallTyler commented Nov 9, 2022

grossws commented Nov 9, 2022

2TallTyler commented Nov 9, 2022

grossws commented Dec 1, 2019 •

edited

Loading

grossws commented Dec 2, 2019 •

edited

Loading

andythenorth commented Dec 24, 2020 •

edited

Loading

ldpl left a comment •

edited

Loading

ldpl Feb 19, 2021 •

edited

Loading