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 setting to only display control characters #69296

Merged
merged 8 commits into from Aug 7, 2019

Conversation

@mkenigs
Copy link
Contributor

commented Feb 24, 2019

Fixes #66674
Creates settings for Screencast Mode and a setting to only display control characters, and changes Screencast Action accordingly.

@msftclas

This comment has been minimized.

Copy link

commented Feb 24, 2019

CLA assistant check
All CLA requirements met.

@joaomoreno joaomoreno added this to the Backlog milestone Feb 25, 2019

@mkenigs mkenigs force-pushed the mkenigs:fix-66674 branch to 1cec8dd Feb 25, 2019

@jbreuer95

This comment has been minimized.

Copy link

commented Feb 26, 2019

+1

@texastoland

This comment has been minimized.

Copy link

commented Mar 7, 2019

Excited to try this! Does it handle multi-key shortcuts like Markdown preview too?

@mkenigs

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

It works for multi-key shortcuts, though screencast mode as a whole doesn't handle chords yet.

@texastoland
Copy link

left a comment

CC @mkenigs 👀

'properties': {
'screencastMode.onlyControlKeys': {
'type': 'boolean',
'description': nls.localize('screencastMode.onlyControlKeys', "Only show control keys in Screencast Mode."),

This comment has been minimized.

Copy link
@texastoland

This comment has been minimized.

Copy link
@mkenigs

mkenigs Jun 12, 2019

Author Contributor

Agreed, pushed a commit! Honestly would probably be better to tweak this so it was just all keyboard shortcuts - thoughts?

@@ -193,13 +196,15 @@ export class ToggleScreencastModeAction extends Action {
const label = keybinding.getLabel();

if (!event.ctrlKey && !event.altKey && !event.metaKey && !event.shiftKey && this.keybindingService.mightProducePrintableCharacter(event) && label) {
keyboardMarker.textContent += ' ' + label;
if (!this.configurationService.getValue<boolean>('screencastMode.onlyControlKeys')) {

This comment has been minimized.

Copy link
@texastoland

texastoland Jun 12, 2019

This could just be another condition in the line above.

This comment has been minimized.

Copy link
@mkenigs

mkenigs Jun 12, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@texastoland

texastoland Jun 12, 2019

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";
}

This comment has been minimized.

Copy link
@texastoland

texastoland Jun 12, 2019

@joaomoreno This looks close CC @mjbvz. Could we get an additional reviewer once @mkenigs implements changes?

} else {
keyboardMarker.textContent = label;
keyboardMarker.style.display = 'block';

This comment has been minimized.

Copy link
@texastoland

texastoland Jun 12, 2019

And move this back below.

mkenigs added 5 commits Jun 12, 2019
@mkenigs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@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

@mkenigs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Bonus feature: makes it super easy to add support for chords

@mkenigs mkenigs force-pushed the mkenigs:fix-66674 branch to bb98a8a Jul 11, 2019

@joaomoreno

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@mkenigs You seem to do a lot more here than expected:

  • control characters
  • chords
  • setting
  • setting category

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:

  • press ⌘
  • press K, release K
  • press C, release C
  • release ⌘

But they don't work in the following:

  • press ⌘
  • press K, release K
  • release ⌘
  • press ⌘
  • press C, release C
  • release ⌘
@mkenigs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

@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?

@mkenigs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@joaomoreno Anything else I can do on this?

@joaomoreno

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Cleaned up a bit, removed the chord handling. Thanks! 🍻

@joaomoreno joaomoreno modified the milestones: Backlog, August 2019 Aug 7, 2019

@joaomoreno joaomoreno merged commit 61c463b into microsoft:master Aug 7, 2019

5 checks passed

VS Code Build #20190713.27 succeeded
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
VS Code (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
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.