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

Remove hardcoded hotkey references from production, support powers, and viewport. #13966

Merged
merged 7 commits into from Sep 10, 2017

Conversation

Projects
None yet
3 participants
@pchote
Member

pchote commented Sep 2, 2017

Part 3 of my campaign to unhardcode and move gameplay-specific hotkeys to mod code.
This follows on from #13706 and #13711.

This converts all the remaining hotkey consumers except for WorldCommandWidget and the hardcoded "dev" hotkeys. Those two require more drastic changes that will come in their own PRs.

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

@rob-v

mostly optional nits.

public readonly NamedHotkey JumpToTopEdgeKey = new NamedHotkey();
public readonly NamedHotkey JumpToBottomEdgeKey = new NamedHotkey();
public readonly NamedHotkey JumpToLeftEdgeKey = new NamedHotkey();
public readonly NamedHotkey JumpToRightEdgeKey = new NamedHotkey();

This comment has been minimized.

@rob-v

rob-v Sep 3, 2017

Contributor

You removed 'Edge' from Settings keys (MapPushLeftEdge -> MapJumpToLeftKey), it could be removed also here for consistency.

@rob-v

rob-v Sep 3, 2017

Contributor

You removed 'Edge' from Settings keys (MapPushLeftEdge -> MapJumpToLeftKey), it could be removed also here for consistency.

Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Ingame/ProductionTooltipLogic.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Ingame/SupportPowerTooltipLogic.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Ingame/SupportPowerTooltipLogic.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/ProductionTabsWidget.cs
Show outdated Hide outdated OpenRA.Mods.Common/Lint/CheckChromeHotkeys.cs
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 4, 2017

Member

Updated as requested, and added a few more cleanups on top of your suggestions to the support/production tooltip logic.

Member

pchote commented Sep 4, 2017

Updated as requested, and added a few more cleanups on top of your suggestions to the support/production tooltip logic.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 5, 2017

Member

D'oh. Fixed.

Member

pchote commented Sep 5, 2017

D'oh. Fixed.

@rob-v

rob-v approved these changes Sep 5, 2017 edited

lgtm 👍

throw in LinterHotkeyNames is less convinient - emitError could be passed to use instead or added try/catch around LinterHotkeyNames call - to display only mesage and continue checking instead of stacktrace and 'crash'/stop on first occurrence.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 5, 2017

Member

Good idea, will do.

Member

pchote commented Sep 5, 2017

Good idea, will do.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 5, 2017

Member

Done. This also let me print the file and line numbers:

Testing Tiberian Sun mod MiniYAML...
Testing mod: Tiberian Sun
OpenRA.Utility(1,1): Error: ts|chrome/ingame-player.yaml:474 must define HotkeyPrefix if HotkeyCount > 0.
OpenRA.Utility(1,1): Error: common|chrome/editor.yaml:214 must define BookmarkRestoreKeyPrefix if BookmarkKeyCount > 0.
Member

pchote commented Sep 5, 2017

Done. This also let me print the file and line numbers:

Testing Tiberian Sun mod MiniYAML...
Testing mod: Tiberian Sun
OpenRA.Utility(1,1): Error: ts|chrome/ingame-player.yaml:474 must define HotkeyPrefix if HotkeyCount > 0.
OpenRA.Utility(1,1): Error: common|chrome/editor.yaml:214 must define BookmarkRestoreKeyPrefix if BookmarkKeyCount > 0.
@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 9, 2017

Contributor

Could you clarify the logic behind hotkeys defined in Widget vs Logic due to different configuration/implementation?

Contributor

rob-v commented Sep 9, 2017

Could you clarify the logic behind hotkeys defined in Widget vs Logic due to different configuration/implementation?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 9, 2017

Member

Keys that control or interact with a widget go on that widget. Everything else should be implemented as stand-alone logic classes.

Member

pchote commented Sep 9, 2017

Keys that control or interact with a widget go on that widget. Everything else should be implemented as stand-alone logic classes.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2017

Contributor

Note: it doesn't seem to be caused by this PR (hence the +1), but next/prev tab (page up/down) don't work on my system, neither with this PR nor on bleed.

Everything else works, but I'll wait for a reply (and rebase, obviously) before I merge this.

Contributor

reaperrr commented Sep 10, 2017

Note: it doesn't seem to be caused by this PR (hence the +1), but next/prev tab (page up/down) don't work on my system, neither with this PR nor on bleed.

Everything else works, but I'll wait for a reply (and rebase, obviously) before I merge this.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 10, 2017

Contributor

Page up/down works e.g. in TD to switch between all barracks,... iirc.

Contributor

rob-v commented Sep 10, 2017

Page up/down works e.g. in TD to switch between all barracks,... iirc.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2017

Contributor

Page up/down works e.g. in TD to switch between all barracks,... iirc.

Ah, I see. That works, so just needs a rebase then.

Contributor

reaperrr commented Sep 10, 2017

Page up/down works e.g. in TD to switch between all barracks,... iirc.

Ah, I see. That works, so just needs a rebase then.

pchote added some commits Jul 23, 2017

Clean up ProductionTooltipLogic:
- Avoid creating unnecessary bindings
- Fix time display not updating on low power
- Fix power usage color not updating on power changes
- Avoid duplicated text size calculations
Clean up SupportPowerTooltipLogic:
- Avoid creating unnecessary bindings
- Avoid duplicated text size calculations
- Relayout panel when (and only when) needed
- Color timer red when paused
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

Rebased.

Member

pchote commented Sep 10, 2017

Rebased.

@reaperrr reaperrr merged commit 1e4640d into OpenRA:bleed Sep 10, 2017

2 checks passed

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

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

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