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.

Sign up for GitHub

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

Jump to bottom

[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
+52 −2
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 43 additions & 2 deletions src/rail_gui.cpp
View file
Open in desktop
Expand Up @@ -762,6 +762,45 @@ struct BuildRailToolbarWindow : Window {
static HotkeyList hotkeys;
};

/**
* Selects RailType based on _last_built_railtype and global modifiers from RailToolbarWidgetModifiers
*
* If GHK_MOD_* modifiers were used hotkey replaced with last selected widget
* or WID_RAT_AUTORAIL (if toolbar was closed).
*
* Without wrapping, so if you're on RAILTYPE_RAIL you'll stay there. Same for upper boundary.
*
* @param [out] hotkey Replaced by last user selected widget or WID_RAT_AUTORAIL if prev/next railtype used
* @return selected RailType
*/
static RailType ChooseRailTypeOnGlobalHotkey(int &hotkey)
{
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)

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

change_rt = true;
do {
rt++;
} while (rt < RAILTYPE_END && !HasRailtypeAvail(_local_company, rt));
if (rt >= RAILTYPE_END) rt = _last_built_railtype;
}
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.

}
_last_built_railtype = rt;
return rt;
}

/**
* Handler for global hotkeys of the BuildRailToolbarWindow.
* @param hotkey Hotkey
Expand All @@ -770,8 +809,8 @@ struct BuildRailToolbarWindow : Window {
static EventState RailToolbarGlobalHotkeys(int hotkey)
{
if (_game_mode != GM_NORMAL) return ES_NOT_HANDLED;
extern RailType _last_built_railtype;
Window *w = ShowBuildRailToolbar(_last_built_railtype);
RailType railtype = ChooseRailTypeOnGlobalHotkey(hotkey);
Window *w = ShowBuildRailToolbar(railtype);
if (w == nullptr) return ES_NOT_HANDLED;
return w->OnHotkey(hotkey);
}
Expand All @@ -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 •
edited

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.

HOTKEY_LIST_END
};
HotkeyList BuildRailToolbarWindow::hotkeys("railtoolbar", railtoolbar_hotkeys, RailToolbarGlobalHotkeys);
Expand Down
9 changes: 9 additions & 0 deletions src/widgets/rail_widget.h
View file
Open in desktop
Expand Up @@ -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_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.

GHK_MODB_RAT_NEXT_RAILTYPE = 1 << GHK_MOD_RAT_NEXT_RAILTYPE, ///< bitmask for GHK_MOD_RAT_NEXT_RAILTYPE
};

/** Widgets of the #BuildRailToolbarWindow class. */
enum RailToolbarWidgets {
/* Name starts with RA instead of R, because of collision with RoadToolbarWidgets */
Expand Down