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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grossws
Copy link
Contributor

@grossws grossws commented Dec 1, 2019

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

@grossws 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 grossws force-pushed the change-prev-next-railtype-hotkeys branch from ff01b43 to f29dd26 Compare December 1, 2019 23:18
src/widgets/rail_widget.h Outdated Show resolved Hide resolved
@grossws grossws force-pushed the change-prev-next-railtype-hotkeys branch from f29dd26 to c42996b Compare December 1, 2019 23:52
@nielsmh
Copy link
Contributor

nielsmh commented Dec 2, 2019

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

@grossws
Copy link
Contributor Author

grossws commented Dec 2, 2019

@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
Copy link
Contributor Author

grossws commented Dec 7, 2019

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 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
Copy link
Contributor Author

grossws commented Dec 24, 2019

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

@grossws 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 TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Dec 14, 2020
@TrueBrain
Copy link
Member

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
Copy link
Contributor

andythenorth commented Dec 24, 2020

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

@grossws
Copy link
Contributor Author

grossws commented Dec 28, 2020

@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 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 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
Copy link
Contributor Author

grossws commented Jan 14, 2021

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 grossws requested a review from LordAro January 14, 2021 00:43
@grossws grossws force-pushed the change-prev-next-railtype-hotkeys branch from 9f690f6 to 6064c3f Compare January 16, 2021 15:09
@grossws
Copy link
Contributor Author

grossws commented Feb 9, 2021

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

src/rail_gui.cpp Outdated Show resolved Hide resolved
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>
@grossws grossws force-pushed the change-prev-next-railtype-hotkeys branch from 6064c3f to 12a00a9 Compare February 15, 2021 12:55
@grossws grossws requested a review from LordAro February 18, 2021 14:58
LordAro
LordAro previously approved these changes Feb 18, 2021
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Go for it

Copy link
Contributor

@ldpl ldpl left a comment

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, they aren't bitmasks anymore

ClrBit(hotkey, GHK_MOD_RAT_PREV_RAILTYPE);
change_rt = true;
do {
rt--;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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),
Copy link
Contributor

@ldpl ldpl Feb 19, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 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
@grossws
Copy link
Contributor Author

grossws commented Mar 6, 2021

@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 ,)

@LordAro LordAro dismissed their stale review March 6, 2021 16:15

dismiss for now

@2TallTyler
Copy link
Member

@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
Copy link
Contributor Author

grossws commented Nov 9, 2022

@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
Copy link
Member

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.

@2TallTyler 2TallTyler added the work: needs more work This Pull Request needs more work before it can be accepted label Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process work: needs more work This Pull Request needs more work before it can be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants