Skip to content

ShowKeybindDialog now renders a table.#188

Merged
josteink merged 5 commits into
workspacer:masterfrom
dalyIsaac:keybind-dialog-table
Feb 21, 2021
Merged

ShowKeybindDialog now renders a table.#188
josteink merged 5 commits into
workspacer:masterfrom
dalyIsaac:keybind-dialog-table

Conversation

@dalyIsaac
Copy link
Copy Markdown
Member

This adds KeyValueTable, which is used to replace TextBlockMessage for displaying the current keybindings.

Changes:

  • Added KeyValueTable
  • Added Capitalize string extension method
  • GetKeybindString capitalizes keys to aid readability
  • GetKeybindString adds support for KeyModifiers.Win

image

- Added `KeyValueTable`
- `GetKeybindString` capitalizes keys
- `GetKeybindString` adds support for `KeyModifiers.Win`
@N1x0
Copy link
Copy Markdown
Contributor

N1x0 commented Feb 20, 2021

I like the change and the addition of search functionality is very nice! Tested it and all key binds show and search works for both the description as well as the shortcuts. One issue however, is that if you open the dialog once and then close it you will run into an "object disposed" error if you try to open it again. I'd either include some error handling for this or change the way the KeyValueTable gets disposed. If you can fix this it'd be a fantastic change IMO. Thanks for the PR

@dalyIsaac
Copy link
Copy Markdown
Member Author

Thanks for pointing out that error - it should be fixed now.

Copy link
Copy Markdown
Contributor

@N1x0 N1x0 left a comment

Choose a reason for hiding this comment

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

The fix for for the KeyValueTable works as intended. Would like to add 1 minor change to the regex but other than that all looks good!

Comment thread src/workspacer/Keybinds/KeybindManager.cs Outdated
Copy link
Copy Markdown
Contributor

@N1x0 N1x0 left a comment

Choose a reason for hiding this comment

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

All good! Will have to wait for @josteink or @rickbutton for the merge though

Copy link
Copy Markdown
Member

@josteink josteink left a comment

Choose a reason for hiding this comment

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

That's awesome. Thanks for the patch!

@josteink josteink merged commit 99671b6 into workspacer:master Feb 21, 2021
@dalyIsaac dalyIsaac deleted the keybind-dialog-table branch February 21, 2021 07:57
@dalyIsaac dalyIsaac added this to the 0.9.11 milestone Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants