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

Rework global and world hotkeys #13974

Merged
merged 9 commits into from Sep 15, 2017

Conversation

Projects
None yet
3 participants
@pchote
Member

pchote commented Sep 3, 2017

Hardcoding global hotkeys in the static order handling plumbing is fundamentally incompatible with the future mod-defined hotkey system: it is too low level to be exposed to mod code or to the linter. Fortunately we already have a pattern for mod-defined global hotkeys, as used by the music player.

This PR migrates the screenshot and mute (which previously only worked ingame) hotkeys to this new system, and removes the DevReloadChrome and HideUserInterface hotkeys. These latter two are hacks that don't (and shouldn't!) work from mod code, so they can't easily be made to work with the new system. Since they are both niche features that don't have (in my opinion) compelling use cases I felt that it was better to remove them than to let them stop progress towards the hotkey improvements.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 3, 2017

Member

I have pulled out a SingleHotkeyBaseLogic parent class to save duplicating boilerplate across N logic classes. I want to port all the other keys that will use this as part of this PR.

Member

pchote commented Sep 3, 2017

I have pulled out a SingleHotkeyBaseLogic parent class to save duplicating boilerplate across N logic classes. I want to port all the other keys that will use this as part of this PR.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 3, 2017

Member

Updated. This plus #13966 now complete the migration from hardcoded settings references to NamedHotkey.

Member

pchote commented Sep 3, 2017

Updated. This plus #13966 now complete the migration from hardcoded settings references to NamedHotkey.

@pchote pchote requested a review from rob-v Sep 3, 2017

@pchote pchote changed the title from Rework global hotkeys to Rework global and world hotkeys Sep 3, 2017

@rob-v

works fine ingame, some issues outside (in scope of this PR?)
not working:
music keys: in lobby, editor, editor menu
screenshot: in editor, menus
mute: in editor

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 7, 2017

Member

Fixed.

Re lobby keys not working: the lobby force-focuses the chat field which eats your input. If you switch to the map browser and back it will be unfocused and the global keys work.

Re editor keys not working: #11136 mistakenly added the key handler to the new map dialog only, and it wasn't caught during review (this was before we implemented the requirement for two 👍s)

Member

pchote commented Sep 7, 2017

Fixed.

Re lobby keys not working: the lobby force-focuses the chat field which eats your input. If you switch to the map browser and back it will be unfocused and the global keys work.

Re editor keys not working: #11136 mistakenly added the key handler to the new map dialog only, and it wasn't caught during review (this was before we implemented the requirement for two 👍s)

@rob-v

rob-v approved these changes Sep 7, 2017

lgtm 👍

little issue with renamed hotkey and regression from #13186 where we enabled screenshot hotkey also in dialogs.

protected override bool OnHotkeyActivated(KeyInput e)
{
Game.TakeScreenshot();
return true;

This comment has been minimized.

@rob-v

rob-v Sep 7, 2017

Contributor

really nit: in all other cases there is blank line before return

@rob-v

rob-v Sep 7, 2017

Contributor

really nit: in all other cases there is blank line before return

@@ -448,7 +448,8 @@ Action InitInputPanel(Widget panel)
{ "CycleStatusBarsKey", "Cycle status bars display" },
{ "TogglePixelDoubleKey", "Toggle pixel doubling" },
{ "ToggleMuteKey", "Toggle audio mute" },
{ "TogglePlayerStanceColorsKey", "Toggle player stance colors" }
{ "TogglePlayerStanceColorKey", "Toggle player stance colors" },

This comment has been minimized.

@rob-v

rob-v Sep 7, 2017

Contributor

nit: Renaming Colors to Color doesn't look like an improvement (I assumed you will change all to Colors), because we have Colors in hotkey description, in Game.UsePlayerStanceColors property and after this renaming custom user hotkey in settings.yaml will be broken.

@rob-v

rob-v Sep 7, 2017

Contributor

nit: Renaming Colors to Color doesn't look like an improvement (I assumed you will change all to Colors), because we have Colors in hotkey description, in Game.UsePlayerStanceColors property and after this renaming custom user hotkey in settings.yaml will be broken.

This comment has been minimized.

@pchote

pchote Sep 7, 2017

Member

This is only toggling one thing, so the "s" is inconsistent. These changes have already broken half of the hotkeys in settings.yaml, and will break the rest when they move to mod-specific settings.

@pchote

pchote Sep 7, 2017

Member

This is only toggling one thing, so the "s" is inconsistent. These changes have already broken half of the hotkeys in settings.yaml, and will break the rest when they move to mod-specific settings.

@reaperrr

Didn't encounter any regressions, and screenshot + mute hotkeys now work in main menu

@reaperrr reaperrr merged commit e04ae9a into OpenRA:bleed Sep 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 15, 2017

Contributor

broke bleed:

OpenRA.Mods.Common/Widgets/Logic/Ingame/CommandBarLogic.cs(208,18): error CS1061: Type OpenRA.Mods.Common.Widgets.LogicKeyListenerWidget' does not contain a definition forOnKeyPress' and no extension method OnKeyPress' of typeOpenRA.Mods.Common.Widgets.LogicKeyListenerWidget' could be found. Are you missing an assembly reference?
OpenRA.Mods.Common/Widgets/LogicKeyListenerWidget.cs(18,15): (Location of the symbol related to previous error)
Compilation failed: 1 error(s), 0 warnings
make: *** [mods/common/OpenRA.Mods.Common.dll] Error 1

Contributor

reaperrr commented Sep 15, 2017

broke bleed:

OpenRA.Mods.Common/Widgets/Logic/Ingame/CommandBarLogic.cs(208,18): error CS1061: Type OpenRA.Mods.Common.Widgets.LogicKeyListenerWidget' does not contain a definition forOnKeyPress' and no extension method OnKeyPress' of typeOpenRA.Mods.Common.Widgets.LogicKeyListenerWidget' could be found. Are you missing an assembly reference?
OpenRA.Mods.Common/Widgets/LogicKeyListenerWidget.cs(18,15): (Location of the symbol related to previous error)
Compilation failed: 1 error(s), 0 warnings
make: *** [mods/common/OpenRA.Mods.Common.dll] Error 1

@pchote pchote deleted the pchote:remove-hardcoded-hotkeys-part-4 branch Oct 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment