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

Add drop down key selection support to Keyboard Manager UI (dev/build-features) #2140

Conversation

arjunbalgovind
Copy link
Contributor

@arjunbalgovind arjunbalgovind commented Apr 15, 2020

Summary of the Pull Request

This PR adds support for the user to select the keys from a drop down for the Edit Keyboard and Edit Shortcuts windows.

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Key list is arranged as Modifier keys, character keys, special keys (like Enter, Space, etc) and then unknown keys (which dont have a name just a virtual code). We can choose to not show the unknown keys in the future. In case of Edit shortcuts there is an additional "None" key at the start which is used to remove the drop down.
  • Since in the current key names some key representations could be repeated (for example there are two / keys on the keyboard), key selections are made with a second vector which has the corresponding virtual key code for each key name, rather than searching from the keyboard map. This way we ensure a one-to-one mapping since we search index to key code and not key name to key code.
  • KeyType helper functions added to categorize keys depending on the type of modifier or if it is an action key.
  • Flyout warnings have been added to manage all the scenarios in the drop down input for Edit Shortcuts. These include warnings for making sure that there is atleast 2 keys, there must be exactly one action key, there needs to be a modifier key, action key must be at the end, and a modifier cannot be repeated (example LCtrl and RCtrl).
  • Changed the logic for creating the custom controls to dynamic allocation using unique_ptr since there were cases where the objects were going out of scope and variables were being referenced to result in garbage values.

Validation Steps Performed

From the landing page try both the redefine a shortcut and remap keyboard options and use drop downs as well as detect UI.
keyboardManagerWarnings

@arjunbalgovind arjunbalgovind requested a review from a team April 15, 2020 15:52
@crutkas crutkas added the Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager label Apr 15, 2020
@traies traies self-requested a review April 16, 2020 21:26
Copy link
Member

@traies traies left a comment

Choose a reason for hiding this comment

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

The dropdowns dont update when the keyboard layout changes. I think this could be solved by updating the dropdowns when they are opened, e.g. by setting a callback to DropDownOpened.

src/modules/keyboardmanager/ui/ShortcutControl.cpp Outdated Show resolved Hide resolved
shortcutDropDown.ContextFlyout().SetAttachedFlyout((FrameworkElement)shortcutDropDown, warningFlyout);

// drop down selection handler
shortcutDropDown.SelectionChanged([&, rowIndex, colIndex, parent, warningMessage](IInspectable const& sender, SelectionChangedEventArgs const&) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add a Handler to the DropDownOpened event, where the current Keyboard Layout is checked and the keys shown are updated accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. There was also the case where when you use "Type key" the corresponding drop down has to be set, and if there was a language change while on the type key window it would not be reflected on the drop down since it wasn't opened yet. This was fixed by resetting the drop down item source whenever the user types in a key, that way the latest language is used and it does not happen too often.

src/modules/keyboardmanager/ui/ShortcutControl.cpp Outdated Show resolved Hide resolved
src/modules/keyboardmanager/ui/ShortcutControl.h Outdated Show resolved Hide resolved
Copy link
Member

@traies traies left a comment

Choose a reason for hiding this comment

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

LGTM

src/modules/keyboardmanager/common/LayoutMap.cpp Outdated Show resolved Hide resolved
}

// Constructor for shortcut drop down
KeyDropDownControl(size_t rowIndex, size_t colIndex, std::vector<std::vector<Shortcut>>& shortcutRemapBuffer, std::vector<std::unique_ptr<KeyDropDownControl>>& keyDropDownControlObjects, StackPanel parent)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe moving this to a method would be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the common part in both the constructors to a separate function. The only thing defined in the two constructors are the selectionchanged handlers.


ShortcutControl(const int& rowIndex, const int& colIndex)
ShortcutControl(const size_t rowIndex, const size_t colIndex)
Copy link
Member

Choose a reason for hiding this comment

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

Why const size_t here? Seems unnecesary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's size_t since I'm passing in a vector.size() as argument while creating the object.

@arjunbalgovind arjunbalgovind merged commit 0417b62 into microsoft:dev/build-features Apr 18, 2020
traies pushed a commit that referenced this pull request Apr 20, 2020
…-features) (#2140)

* Added combobox

* Formatted and removed unused code

* Added drop down support for Edit Keyboard window

* Reordered the displayed key list

* Add shortcut stack panels and drop downs linked to detect shortcut

* Add more selected item logic

* Added complete dropdown support for edit shortcuts window

* Added Flyout warning for incorrect drop down input

* Tweaked warnings

* Removed MainWindow code

* Changed SelectedValue toSelectedIndex

* Removed unnecessary assignments

* Added a warning for two dropdowns and the first one is changed to an action key

* Added function comments in cpp file

* Fixed some comments

* Fixed all allocation and out of scope issues

* Fix most issues except reloading shortcuts

* Fixed issue while reloading shortcuts

* Fixed type cast warnings

* Changed delete to delete[]

* tweaked
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants