Skip to content

Fixes to telemetry Keyboard and mouse nb telemetry #11220

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

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

DonJayamanne
Copy link

For #10809

  • Changed to use enums (strongly typed) without any mapping of values.
  • We can now see (find references etc) where telemetry is used/sent (instead of string search).
  • Removed redundant telemetry (e.g. keyboard to move cells up/down, this can only be done via mouse)
  • Added missing telemetry (due to mapping we had missed some telemetry, now that its strongly typed we shouldn't have this problem)

@@ -305,70 +304,40 @@ export enum Telemetry {
}

export enum NativeKeyboardCommandTelemetry {
AddToEnd = 'DATASCIENCE.NATIVE.KEYBOARD.ADD_TO_END',
Copy link
Author

Choose a reason for hiding this comment

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

Some of these cannot be done using keyboard, only possible via mouse clicks (hmm, highlights accessibility issues)

Copy link
Member

Choose a reason for hiding this comment

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

Our accessibility is rough, but pretty soon a big chunk of that will be on the VS Code notebook editor.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

}
break;
case 'l':
if (!this.isFocused() && this.isSelected()) {
e.stopPropagation();
e.preventDefault();
this.props.toggleLineNumbers(cellId);
this.props.sendCommand(NativeCommandType.ToggleLineNumbers, 'keyboard');
this.props.sendCommand(NativeKeyboardCommandTelemetry.ToggleLineNumbers);
Copy link
Author

Choose a reason for hiding this comment

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

The telemetry tells us whether its keyboard/mouse, hence we don't need the command argument either.

Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

:shipit:

@DonJayamanne DonJayamanne added the no-changelog No news entry required label Apr 16, 2020
@DonJayamanne DonJayamanne merged commit a049747 into microsoft:master Apr 16, 2020
@DonJayamanne DonJayamanne deleted the issue10809Telemetry branch April 16, 2020 22:29
@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants