Add setting to only display control characters#69296
Add setting to only display control characters#69296joaomoreno merged 8 commits intomicrosoft:masterfrom
Conversation
|
|
|
+1 |
|
Excited to try this! Does it handle multi-key shortcuts like Markdown preview too? |
|
It works for multi-key shortcuts, though screencast mode as a whole doesn't handle chords yet. |
| 'properties': { | ||
| 'screencastMode.onlyControlKeys': { | ||
| 'type': 'boolean', | ||
| 'description': nls.localize('screencastMode.onlyControlKeys', "Only show control keys in Screencast Mode."), |
There was a problem hiding this comment.
There was a problem hiding this comment.
Agreed, pushed a commit! Honestly would probably be better to tweak this so it was just all keyboard shortcuts - thoughts?
|
|
||
| if (!event.ctrlKey && !event.altKey && !event.metaKey && !event.shiftKey && this.keybindingService.mightProducePrintableCharacter(event) && label) { | ||
| keyboardMarker.textContent += ' ' + label; | ||
| if (!this.configurationService.getValue<boolean>('screencastMode.onlyControlKeys')) { |
There was a problem hiding this comment.
This could just be another condition in the line above.
There was a problem hiding this comment.
Assuming screencastMode.onlyControlKeys is true, if I change it back, an empty bar will show up across the screen when I type a non-modifier key. The unwritten else is to pass, leaving keyboardMarker.style.display = 'none' as it's set above
There was a problem hiding this comment.
I see what you mean. There's a bug here when modifiers for caps or foreign letters are pressed. I noticed it reading the comments for the mightProducePrintableCharacter implementation. I propose an alternative.
const isShortcutKey = event.ctrlKey || event.metaKey;
const isModifierKey = isShortcutKey || event.altKey || event.shiftKey;
const isPlainCharacterKey =
!isModifierKey &&
this.keybindingService.mightProducePrintableCharacter(event) &&
label;
if (isPlainCharacterKey) {
keyboardMarker.textContent += " ";
}
keyboardMarker.textContent += label;
if (isShortcutKey || !this.configurationService.getValue<boolean>( "screencastMode.showOnlyShortcutKeys")
) {
keyboardMarker.style.display = "block";
}
There was a problem hiding this comment.
@joaomoreno This looks close CC @mjbvz. Could we get an additional reviewer once @mkenigs implements changes?
| } | ||
| } else { | ||
| keyboardMarker.textContent = label; | ||
| keyboardMarker.style.display = 'block'; |
|
@texastoland wish I could have gotten to this sooner but anyways, I changed the setting to whether something is a keyboard shortcut, not just a modifier key. Seems to me like that's what's going to be desired most of the time. The feature as a whole is still kinda buggy but I think that's a separate issue. Let me know what you think |
|
Bonus feature: makes it super easy to add support for chords |
|
@mkenigs You seem to do a lot more here than expected:
Usually we prefer to have everything in separate PRs, but let's take it from here. The intended feature for this PR works fine. But not chord handling. Chords work in the following key sequence:
But they don't work in the following:
|
|
@joaomoreno thanks for looking at this! Also happy to split into multiple PRs if that would be easier. Changed it so that screencast mode only ever displays keys that have dispatch parts. That fixes the issue with chords, and it also allows slightly simpler logic regarding keys like shift. Chords still won't display properly if the screencast mode timeout is reached before that of chords - should those be changed to match? |
|
@joaomoreno Anything else I can do on this? |
|
Cleaned up a bit, removed the chord handling. Thanks! 🍻 |
5 participants
Fixes #66674
Creates settings for Screencast Mode and a setting to only display control characters, and changes Screencast Action accordingly.