Skip to content

/ OpenTTD

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
from
Open

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

wants to merge 1 commit into from

Conversation

@grossws
Copy link
Contributor

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+] but maybe I should leave them unbound, so only those who need it will bind them.

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 grossws:change-prev-next-railtype-hotkeys branch from ff01b43 to f29dd26 Dec 1, 2019
@James103
James103 reviewed Dec 1, 2019
View changes
src/widgets/rail_widget.h Outdated Show resolved Hide resolved
@grossws
Change: add support for next/previous railtype global hotkeys
Loading status checks…
c42996b 
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+]` but maybe I should leave them unbound, so only those who need it will bind them.

Signed-off-by: Konstantin Gribov <grossws@gmail.com>
@grossws grossws force-pushed the grossws:change-prev-next-railtype-hotkeys branch from f29dd26 to c42996b Dec 1, 2019
@nielsmh

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

grossws commented Dec 2, 2019 •
edited

@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

This comment has been minimized.

Sign in to view
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.
@LordAro
LordAro requested changes Dec 23, 2019
View changes
src/rail_gui.cpp
extern RailType _last_built_railtype;
RailType rt = _last_built_railtype;
if (HasBit(hotkey, GHK_MOD_RAT_PREV_RAILTYPE))
{

This comment has been minimized.

Sign in to view
Copy link
@LordAro

LordAro Dec 23, 2019

Member

codestyle - opening brace goes on the same line as the if
src/rail_gui.cpp
// RailType is byte (unsigned char), so it overflows after 0
if (rt >= RAILTYPE_END) rt = _last_built_railtype;
}
else if (HasBit(hotkey, GHK_MOD_RAT_NEXT_RAILTYPE))

This comment has been minimized.

Sign in to view
Copy link
@LordAro

LordAro Dec 23, 2019

Member

codestyle - else (if) goes on the same line as closing brace
src/rail_gui.cpp
if (HasBit(hotkey, GHK_MOD_RAT_PREV_RAILTYPE))
{
ClrBit(hotkey, GHK_MOD_RAT_PREV_RAILTYPE);
do { rt--; } while (rt > RAILTYPE_BEGIN && !HasRailtypeAvail(_local_company, rt));

This comment has been minimized.

Sign in to view
Copy link
@LordAro

LordAro Dec 23, 2019

Member

I get why you've done it this way, but I think I'd prefer this (and the other) to be styled "properly"

Suggested change
do { rt--; } while (rt > RAILTYPE_BEGIN && !HasRailtypeAvail(_local_company, rt));
do {
rt--;
} while (rt > RAILTYPE_BEGIN && !HasRailtypeAvail(_local_company, rt));
src/rail_gui.cpp
* Selects RailType for autorail based on _last_built_railtype and global modifiers from RailToolbarWidgetModifiers
*
* Without wrapping, so if you're on RAILTYPE_RAIL you'll stay there. Same for upper boundary.
* Also clears modifier bits to allow normal widget OhHotkey logic.

This comment has been minimized.

Sign in to view
Copy link
@LordAro

LordAro Dec 23, 2019

Member
Suggested change
* Also clears modifier bits to allow normal widget OhHotkey logic.
* Also clears modifier bits to allow normal widget OnHotkey logic.
src/rail_gui.cpp
* Without wrapping, so if you're on RAILTYPE_RAIL you'll stay there. Same for upper boundary.
* Also clears modifier bits to allow normal widget OhHotkey logic.
*
* @param hotkey Hotkey

This comment has been minimized.

Sign in to view
Copy link
@LordAro

LordAro Dec 23, 2019

Member
Suggested change
* @param hotkey Hotkey
* @param [out] hotkey Something more than one word please
@grossws

This comment has been minimized.

Sign in to view
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@James103 James103

@LordAro LordAro

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@grossws @nielsmh @LordAro @James103
You can’t perform that action at this time.