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

Add Statistics options hotkeys #13781

Merged
merged 1 commit into from Sep 24, 2017

Conversation

Projects
None yet
5 participants
@rob-v
Contributor

rob-v commented Aug 6, 2017

Closes #13743
New hotkeys for Statistics dialog:

image

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 6, 2017

Member

#13706 and #13711 rework the way hotkeys are defined and used. Can you please update this to use the new system?

Member

pchote commented Aug 6, 2017

#13706 and #13711 rework the way hotkeys are defined and used. Can you please update this to use the new system?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Aug 6, 2017

Contributor

Updated to use new hotkeys design.

Contributor

rob-v commented Aug 6, 2017

Updated to use new hotkeys design.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Aug 6, 2017

Contributor

Updated again, pushed only rebased stuff before. Now pushed also with adaptations for new hotkey design.

Contributor

rob-v commented Aug 6, 2017

Updated again, pushed only rebased stuff before. Now pushed also with adaptations for new hotkey design.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 6, 2017

Member

Note for reviewing: ddfb601?w=1 is the meat of this PR, ignoring the whitespace and my prereq changes.

The code changes look good, but I haven't yet reviewed the behavior or choice of keys.

Member

pchote commented Aug 6, 2017

Note for reviewing: ddfb601?w=1 is the meat of this PR, ignoring the whitespace and my prereq changes.

The code changes look good, but I haven't yet reviewed the behavior or choice of keys.

@netnazgul

This comment has been minimized.

Show comment
Hide comment
@netnazgul

netnazgul Sep 3, 2017

Contributor

@rob-v can this be prepped for Next+1?

Contributor

netnazgul commented Sep 3, 2017

@rob-v can this be prepped for Next+1?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 4, 2017

Contributor

Updated with hotkeys array as suggested.

Contributor

rob-v commented Sep 4, 2017

Updated with hotkeys array as suggested.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 16, 2017

Member

Sorry, but this needs a rebase to avoid another #14028 situation.

Member

pchote commented Sep 16, 2017

Sorry, but this needs a rebase to avoid another #14028 situation.

@pchote

Looks good overall, just two minor points (of which one is my fault) and a scope creep:

This feature would be much more useful if the tab hotkeys worked directly from the world without opening the panel first. It shouldn't be too hard to adapt:

var stats = widget.GetOrNull<MenuButtonWidget>("OBSERVER_STATS_BUTTON");
if (stats != null)
{
stats.IsDisabled = () => disableSystemButtons || world.Map.Visibility.HasFlag(MapVisibility.MissionSelector);
stats.OnClick = () => OpenMenuPanel(stats);
}

to open from any of the keys by following the approach of:

var keyOverrides = widget.GetOrNull<LogicKeyListenerWidget>("MODIFIER_OVERRIDES");
if (keyOverrides != null)
{
keyOverrides.AddHandler(e =>
{
// HACK: enable attack move to be triggered if the ctrl key is pressed
var modified = new Hotkey(e.Key, e.Modifiers & ~Modifiers.Ctrl);
if (attackMoveButton.Key.GetValue() == modified)
{
attackMoveButton.OnKeyPress(e);
return true;
}
return false;
});
}

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 16, 2017

Contributor

Updated.

Contributor

rob-v commented Sep 16, 2017

Updated.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 17, 2017

Contributor

Updated.

Contributor

rob-v commented Sep 17, 2017

Updated.

@pchote

Looks good, thanks. Just two more minor nits, and then can you also change the OBSERVER_STATS_BUTTON Key:'s to use StatisticsBasic instead of hardcoding F1?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 17, 2017

Contributor

Updated. All good ideas. Now much more useful to use hotkeys ingame to open directly a stats tab, though it is little annoying (discouraging to use) that it takes second(s) (since latest playtests) to open a/Stats dialog.

Re OBSERVER_STATS_BUTTON's Key - I removed it, as StatisticsBasicKey already works without this now. Is it ok or you prefer to keep it?

Contributor

rob-v commented Sep 17, 2017

Updated. All good ideas. Now much more useful to use hotkeys ingame to open directly a stats tab, though it is little annoying (discouraging to use) that it takes second(s) (since latest playtests) to open a/Stats dialog.

Re OBSERVER_STATS_BUTTON's Key - I removed it, as StatisticsBasicKey already works without this now. Is it ok or you prefer to keep it?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 17, 2017

Member

though it is little annoying (discouraging to use) that it takes second(s) (since latest playtests) to open a/Stats dialog.

What do you mean? Is this a recent regression?

Re OBSERVER_STATS_BUTTON's Key - I removed it, as StatisticsBasicKey already works without this now. Is it ok or you prefer to keep it?

Keep it please so that it will show the hotkey in the tooltip.

Member

pchote commented Sep 17, 2017

though it is little annoying (discouraging to use) that it takes second(s) (since latest playtests) to open a/Stats dialog.

What do you mean? Is this a recent regression?

Re OBSERVER_STATS_BUTTON's Key - I removed it, as StatisticsBasicKey already works without this now. Is it ok or you prefer to keep it?

Keep it please so that it will show the hotkey in the tooltip.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 17, 2017

Contributor

Updated.
I assumed it is well known, but didn't see more comments about this... #13526
Right, tooltip.

Contributor

rob-v commented Sep 17, 2017

Updated.
I assumed it is well known, but didn't see more comments about this... #13526
Right, tooltip.

@pchote

Sorry, a couple more and hopefully final set of polish issues:

Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated
Show outdated Hide outdated mods/ts/chrome/ingame-observer.yaml Outdated
@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Sep 17, 2017

Contributor

Updated.

Contributor

rob-v commented Sep 17, 2017

Updated.

@pchote

pchote approved these changes Sep 17, 2017

@reaperrr reaperrr merged commit c9b4568 into OpenRA:bleed Sep 24, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@netnazgul

netnazgul Oct 16, 2017

Contributor

Hm, so this was merged 3 weeks ago but didn't get into release?

Contributor

netnazgul commented Oct 16, 2017

Hm, so this was merged 3 weeks ago but didn't get into release?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 16, 2017

Member

This was merged to bleed, not prep.

Member

pchote commented Oct 16, 2017

This was merged to bleed, not prep.

@netnazgul

This comment has been minimized.

Show comment
Hide comment
@netnazgul

netnazgul Oct 17, 2017

Contributor

yes, @abcdefg30 already explained it in discord. So it will be re-checked for the future playtests, right?

I suppose there is a list of bleed PRs that you go through before proceeding with bug-clearing the coming release?

Contributor

netnazgul commented Oct 17, 2017

yes, @abcdefg30 already explained it in discord. So it will be re-checked for the future playtests, right?

I suppose there is a list of bleed PRs that you go through before proceeding with bug-clearing the coming release?

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Oct 17, 2017

Member

This PR, the same as all other PRs merged into bleed until a new prep branch is split off, go into the next playtest, yes.

Member

abcdefg30 commented Oct 17, 2017

This PR, the same as all other PRs merged into bleed until a new prep branch is split off, go into the next playtest, yes.

@rob-v rob-v deleted the rob-v:AddStatisticTabsHotkeys branch Oct 18, 2017

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