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

Keyboard Shortcuts for TM:PE #382

Merged
merged 37 commits into from
Jul 2, 2019

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Jun 16, 2019

Note to reviewers

Please review with "Ignore whitespace" (⚙️ button above the diff view)

Changes

GUI settings page for keyboard shortcuts.
TMPE main window and 6 most used tools can be used with keyboard.
Settings are automatically saved by ColossalFramework.
Tooltips are shown on the menu buttons for the current keyboard shortcut.
FIXME: Tooltip on main crown button is not shown.
Stay in lane Shift+S shortcut does work

Checklist

  • Secondary keyboard shortcut
  • +click mouse combinations allowing to select only modifier keys Moved to Future stuff for keybinds tab #402
  • Tooltips without a key should say nothing
  • 👑 button tooltip is missing

image
image

@krzychu124 krzychu124 self-requested a review June 16, 2019 01:06
@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 16, 2019

Working to add another commit to this, which will also allow customizing Shift+S stay in lane key shortcut. At the time of writing, it works if it is not Shift+S for me, so something else is blocking it.

@krzychu124
Copy link
Member

I think you forgot to add KeyMapping.cs to csproj because our CI is failing 😉

@FireController1847 FireController1847 changed the title Keyboard shortcuts for TMPE main window and 6 tools Keyboard Shortcuts for TM:PE Jun 16, 2019
@FireController1847 FireController1847 added Accessibility Color blindness, etc. feature A new distinct feature LABS TM:PE LABS branch STABLE TM:PE STABLE branch labels Jun 16, 2019
Copy link
Collaborator

@FireController1847 FireController1847 left a comment

Choose a reason for hiding this comment

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

This is only my review for language files, aubergine needs to review the code.

TLM/TLM/Resources/lang.txt Outdated Show resolved Hide resolved
TLM/TLM/Resources/lang.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@FireController1847 FireController1847 left a comment

Choose a reason for hiding this comment

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

Other than this one, it's pretty good. Don't forget to add translations for all the other languages.

TLM/TLM/State/Options.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@FireController1847 FireController1847 left a comment

Choose a reason for hiding this comment

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

Translations LGTM. Awaiting code review and it should be good to merge.

TLM/TLM/Resources/lang_pl.txt Outdated Show resolved Hide resolved
Adding PL translations
@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

I need some help with Shift+S shortcut, it seemed to not work for me.
It works if I use something other than Shift+S though. Could it possibly be a SavedInputKey limitation?

@originalfoo
Copy link
Member

I'm not sure how the game's own keymapping system works. Maybe it treats every shortcut as globally unique, rather than tool specific?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

I believe it is SavedInputKey.IsPressed(Event.current) which is failing. So maybe something with Shift+letter, i've no idea need to investigate.

@krzychu124
Copy link
Member

I will try to check what is going there when I'll be back home 😉

@krzychu124
Copy link
Member

Ok, I think I found possible cause... https://github.com/krzychu124/Cities-Skylines-Traffic-Manager-President-Edition/blob/1f671ed2d0025b7f3a0be551d4e8c5d5c0dcf4bc/TLM/TLM/UI/SubTools/LaneConnectorTool.cs#L191
This Event.current is not the same even as that triggered in OnGui() method... Inside OnGui() Event has KeyCode.S but this Event.Current doesn't (it has character field set to 83'S', instead of KeyCode enum).

Main part of source code of public bool IsPressed(Event e)

// keyCode is our KeyCode to test against
// e.keyCode comes from Event
return keyCode != KeyCode.None && e.keyCode == keyCode && ... //and more checks there

Another issue, when I switched to Polish lang, tabs are wider and Keybinds tab is not accessible/not visible at all. (I will try to change translation of Maintenance tab in Polish)

Additionally sometimes some key combination Ctrl+Alt+C is locking in-game movement with WASD and mouse middle button - I can't rotate camera. Maybe not related with this PR

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

Fixed Shift+S shortcut now works, also done Delete/Bkspace in the same way, but did not add a new saved inputkey for it.

@originalfoo
Copy link
Member

originalfoo commented Jul 2, 2019

Just reviewing changed files and noticed the Russian translations were updated - was that you Kvakvs? (for changelog mention)

And German too?

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

Lang update, other features working great, no issues with modal.

TLM/TLM/Resources/lang_pl.txt Outdated Show resolved Hide resolved
TLM/TLM/Resources/lang_pl.txt Outdated Show resolved Hide resolved
TLM/TLM/Resources/lang_pl.txt Outdated Show resolved Hide resolved
TLM/TLM/Resources/lang_pl.txt Outdated Show resolved Hide resolved
@originalfoo
Copy link
Member

One of the German locale strings is too long:

image

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

Overall looks really good! Lots of good code cleanup too :)

Spotted a couple of methods with Str() method name that isn't very descriptive. Some variance of using clause location (suggest outside namespace considering we only have one namespace per file).

Doing in-game testing now...

TLM/TLM/State/Keybinds/KeybindSetting.cs Outdated Show resolved Hide resolved
TLM/TLM/State/Keybinds/KeybindSetting.cs Show resolved Hide resolved
TLM/TLM/State/Keybinds/KeybindSetting.cs Outdated Show resolved Hide resolved
TLM/TLM/State/Options.cs Show resolved Hide resolved
@originalfoo
Copy link
Member

Keybind options tab UI feels very solid. Tried everything I can think of and so far not been able to break it. It feels nice and responsive too, and looks really great.

I noticed in code some stuff about row striping, but didn't see any on the UI? (not important, just mentioning it in case the rows were supposed to be striped)

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jul 2, 2019

@aubergine10

I noticed in code some stuff about row striping, but didn't see any on the UI? (not important, just mentioning it in case the rows were supposed to be striped)

The row striping code is still there. But in the old dialog the default background was grey-ish, and row striping code just cleared texture on some lines. In the new dialog i've no idea what the background sprite name is, so they are all black (i.e. not striped). Have to go into ModTool or UIHelper source and find the background sprite name which is used in other dialogs.

@originalfoo
Copy link
Member

LGTM!

@kvakvs Think we can merge this now - but there's some inline tasks (eg. in OP) - do you want to carry those over to another issue (eg. #402 )?

@originalfoo
Copy link
Member

Resolved merge conflict that was caused by #400

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jul 2, 2019

@aubergine10 You've already moved that +click task into #402 so that kind of removes it from here

@originalfoo originalfoo merged commit 66a9258 into CitiesSkylinesMods:master Jul 2, 2019
originalfoo added a commit that referenced this pull request Jul 2, 2019
- Blur fix (#404)
- `.idea/` added to .gitignore (#382)
Emphasia added a commit to Emphasia/CitiesSkylines-TrafficManagerP that referenced this pull request Jul 7, 2019
update Chinese translation of CitiesSkylinesMods/TMPE#382
@Emphasia Emphasia mentioned this pull request Jul 7, 2019
@originalfoo originalfoo added Keybinds Keyboard (and mouse) shortcuts Settings Road config, mod options, config xml labels Aug 13, 2019
@originalfoo originalfoo added the Toolbar The main TMPE toolbar label Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Color blindness, etc. feature A new distinct feature Keybinds Keyboard (and mouse) shortcuts LABS TM:PE LABS branch Localisation Localised text and features Settings Road config, mod options, config xml STABLE TM:PE STABLE branch Toolbar The main TMPE toolbar UI User interface updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants