Skip to content

Fixed the focus on the interactive window #9813

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 5 commits into from
Feb 4, 2020

Conversation

DavidKutu
Copy link

For #9693

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

@DavidKutu DavidKutu closed this Jan 29, 2020
@DavidKutu DavidKutu reopened this Jan 29, 2020
@DavidKutu DavidKutu closed this Jan 29, 2020
@DavidKutu DavidKutu reopened this Jan 29, 2020
@codecov-io
Copy link

codecov-io commented Jan 29, 2020

Codecov Report

Merging #9813 into master will increase coverage by 0.34%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9813      +/-   ##
==========================================
+ Coverage   60.88%   61.22%   +0.34%     
==========================================
  Files         556      563       +7     
  Lines       30000    30058      +58     
  Branches     4548     4545       -3     
==========================================
+ Hits        18265    18403     +138     
+ Misses      10708    10627      -81     
- Partials     1027     1028       +1
Impacted Files Coverage Δ
src/client/activation/serviceRegistry.ts 84.14% <0%> (-15.86%) ⬇️
...tion/languageServer/languageServerFolderService.ts 85.71% <0%> (-14.29%) ⬇️
src/client/common/utils/platform.ts 76.47% <0%> (ø) ⬆️
...activation/languageServer/languageClientFactory.ts 91.66% <0%> (-4.17%) ⬇️
...rc/client/activation/node/languageClientFactory.ts 40% <0%> (-3.75%) ⬇️
.../client/application/diagnostics/serviceRegistry.ts 96.42% <0%> (-3.58%) ⬇️
src/client/debugger/extension/adapter/logging.ts 97.14% <0%> (-2.86%) ⬇️
...c/client/linters/errorHandlers/baseErrorHandler.ts 75% <0%> (-2.78%) ⬇️
src/client/linters/pydocstyle.ts 88.88% <0%> (+0.25%) ⬆️
src/client/datascience/debugLocationTracker.ts 78.12% <0%> (ø) ⬆️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a9a7b7...b4cb3b3. Read the comment docs.

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

As this is a fix for a regression, I'd like to have a functional test to ensure we do not regress again.

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.

I second what Don said. A functional test would be great so we don't break this again.

rchiodo
rchiodo previously approved these changes Jan 31, 2020
@DavidKutu DavidKutu closed this Feb 3, 2020
@DavidKutu DavidKutu reopened this Feb 3, 2020
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 3, 2020

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.0% 0.0% Duplication

@rchiodo rchiodo dismissed stale reviews from themself February 4, 2020 00:08

revoking review

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:

domDiv.focus();

// send the ctrl + 1/2 message, this should put focus back on the input box
const message = createMessageEvent({ type: InteractiveWindowMessages.Activate, payload: undefined });

Choose a reason for hiding this comment

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

I don't see any tests for ctrl+1 keys

Choose a reason for hiding this comment

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

It mentions ctrl+1 key, but we're not sending such key strokes!

Choose a reason for hiding this comment

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

Oops, ignore that.

domDiv.focus();

// send the ctrl + 1/2 message, this should put focus back on the input box
const message = createMessageEvent({ type: InteractiveWindowMessages.Activate, payload: undefined });

Choose a reason for hiding this comment

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

Oops, ignore that.

@DavidKutu DavidKutu merged commit 2c83354 into master Feb 4, 2020
@DavidKutu DavidKutu deleted the davidkutu/InteractiveWindowFocus branch February 4, 2020 17:30
@lock lock bot locked as resolved and limited conversation to collaborators Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants