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

Improve usability and fix some issues with hotkey remap dialog #16816

Merged

Conversation

@dragunoff
Copy link
Contributor

commented Jul 20, 2019

Fix a number of usability issues and make some changes and improvements.

I opted not to inline dialog-hotkey.yaml into settings.yaml because that complicates things with the chrome logic. Ultimately the whole logic and yaml moved into SettingsLogic and settings.yaml.

There are a bunch of commits but I can squash/reorder if needed.

Closes #16812

@abcdefg30 abcdefg30 added this to the Next Release milestone Jul 20, 2019

@pchote
Copy link
Member

left a comment

In the long term we would ideally rewrite the HotkeyDialogLogic to work like MapPreviewLogic rather than having to destroy and recreate the panel each time a new key is pressed. Right now, however, the priority needs to be getting something that works well so we can ship a release.

One inline code comment, and a couple of more general requests:

  • Can you please move the HOTKEY_DIALOG definition from dialog-hotkey.yaml into settings.yaml? It doesn't need to be inline, keep it as its own block, just have it defined in the same file so its clear that it is a dialog within the settings window.

  • The focus behaviour for the hotkey widget is weird. It is focused by default when opening the panel, and is refocused when selecting any other hotkey. Most of the time it remains focused when you change the hotkey value, but sometimes it doesn't. The behaviour of the reset vs cancel buttons is quite unintuitive - they do basically the same thing, but cancel only works while the widget is focused and the change hasn't been committed.

    IMO it would be a lot more intuitive if the hotkey widget remained focused all of the time and changes were always committed immediately. The Cancel button could then be removed, then the entry widget and the remaining two buttons can be move to the right and the hotkey type label moved down so it is on the same line. This height of the bottom panel can then be reduced to make more space for the top scroll panel.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

The focus behaviour for the hotkey widget is weird. It is focused by default when opening the panel, and is refocused when selecting any other hotkey. Most of the time it remains focused when you change the hotkey value, but sometimes it doesn't. The behaviour of the reset vs cancel buttons is quite unintuitive - they do basically the same thing, but cancel only works while the widget is focused and the change hasn't been committed.

IMO it would be a lot more intuitive if the hotkey widget remained focused all of the time and changes were always committed immediately. The Cancel button could then be removed, then the entry widget and the remaining two buttons can be move to the right and the hotkey type label moved down so it is on the same line. This height of the bottom panel can then be reduced to make more space for the top scroll panel.

The widget remains focused only in case the new value is not valid (has a duplicate). Focus is lost if the new value is valid and that serves as feedback that it was commited. I'm not sure about an always-focused widget but I'll try it.

I agree that the cancel button is a bit unintuitive. It is actually a "remove-focus-if-hotkey-is-valid" button because I wanted the user to have the option to cancel an active rebinding. It works much better if there is an explicit "Save/OK" button which was the case in my first implementation. But in the case of an inline panel I'd be fine with removing the cancel button.

@dragunoff dragunoff force-pushed the dragunoff:feature/hotkey-remap-dialog-usability branch from a00ace1 to ad7bbff Aug 2, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Updated and rebased.

  • Moved the dialog inside settings.yaml
  • Removed cancel button
  • Rearranged the panel
  • Cleaned up some redundant logic

I've decided against an always focused widget - I think it's good to have feedback that an entry has been commited.

I'm not very fond of the overall layout of the hotkeys page but this is probably the best we can get at this point. If the flexbox layout engine gets in then a proper redesign can be considered.

ts

cnc

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Why does TD use icons, but the other mods text?

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Why does TD use icons, but the other mods text?

TD is the odd one when it comes to UI. And it has appropriate icons for both buttons. But I'd be fine switching to text only.

@pchote pchote added the PR: Needs +2 label Aug 6, 2019

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

The color that indicates conflicting hotkey bindings is not updated when you change one binding (the untouched binding stays colored, although no longer conflicting). Otherwise 👍

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

The color that indicates conflicting hotkey bindings is not updated when you change one binding (the untouched binding stays colored, although no longer conflicting). Otherwise 👍

Good catch. Should I fix this here or in a follow up?

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

I'd say do it here if it's an easy fix.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Updated with a fix for the color that indicates conflicting hotkey bindings.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Imho the remap dialogue shouldn't have auto-focus when opening the hotkey menu. It would be better if it only took focus when you selected a hotkey or entered the field yourself.

lawANDorder: i tend to agree with abcdefg30 here. when i only want to look up a hotkey it feels a bit dangerous that I'm suddenly in "edit mode"

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Yeah, I also think it's a little stressful to jump straight into edit mode. What should be the initial state then? Select first hotkey without focusing it OR have a blank state without any key selected (like it is before this PR).

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

I think both is fine, but the consensus on IRC was "Select first hotkey without focusing it".

@dragunoff dragunoff force-pushed the dragunoff:feature/hotkey-remap-dialog-usability branch from c388687 to 6c410fd Aug 14, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Rebased and updated:

  • Reorder and squash some of the commits
  • Hightlight the first hotkey but without focusing
  • Fix the reset button for the whole panel (was totaly broken)
  • Remove the dialog when switching to a different panel (otherwise the widget retains focus)

There is minor problem with the last fix - when switching away and back to the hotkeys panel, the remap dialog would be gone. I could not find a solution to that...

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

There is minor problem with the last fix - when switching away and back to the hotkeys panel, the remap dialog would be gone

Widget unfocusing is normally done inside the action returned by InitHotkeysPanel (see InitDisplayPanel for a proper example), which is why having the logic controlling the dialog split into its own chrome tree and chrome logic makes life really difficult.

@dragunoff dragunoff force-pushed the dragunoff:feature/hotkey-remap-dialog-usability branch from 6c410fd to cbb5d07 Aug 16, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Updated as discussed on IRC - moved the whole dialog logic into SettingsLogic. Fixed a number of bugs along the way and simplified the code a bit.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Updated to address pchote's comments. I added a fixup commit, will squash it once the PR is approved.

@pchote
Copy link
Member

left a comment

This is looking a lot better now, thanks :)

Just a few more optimizations, then this should be good to go..

OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Updated to address latest comments. Also shortened some local var names.

@pchote
Copy link
Member

left a comment

LGTM. Please squash the commits now.

@dragunoff dragunoff force-pushed the dragunoff:feature/hotkey-remap-dialog-usability branch from 94cb194 to d9adfec Aug 19, 2019

@reaperrr
Copy link
Contributor

left a comment

LGTM

@reaperrr reaperrr merged commit be1f820 into OpenRA:bleed Aug 23, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.