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

Interrupt kernel button on interactive window toolbar should be disabled when kernel is not busy #7488

Merged
merged 2 commits into from
Sep 13, 2021

Conversation

joyceerhl
Copy link
Contributor

Fix #7269

@joyceerhl joyceerhl requested a review from a team as a code owner September 13, 2021 02:03
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2021

Codecov Report

Merging #7488 (66becaa) into main (d96d21a) will increase coverage by 0%.
The diff coverage is 89%.

@@          Coverage Diff          @@
##            main   #7488   +/-   ##
=====================================
  Coverage     64%     64%           
=====================================
  Files        361     362    +1     
  Lines      22506   22536   +30     
  Branches    3422    3429    +7     
=====================================
+ Hits       14589   14638   +49     
+ Misses      6694    6679   -15     
+ Partials    1223    1219    -4     
Impacted Files Coverage Δ
...lient/datascience/variablesView/notebookWatcher.ts 69% <60%> (+1%) ⬆️
...client/datascience/commands/activeEditorContext.ts 90% <90%> (+1%) ⬆️
src/client/datascience/constants.ts 99% <100%> (+<1%) ⬆️
...c/client/datascience/interactive-window/helpers.ts 100% <100%> (ø)
...lient/datascience/jupyter/kernels/cellExecution.ts 78% <0%> (+<1%) ⬆️
...atascience/interactive-window/interactiveWindow.ts 52% <0%> (+<1%) ⬆️
...datascience/editor-integration/codelensprovider.ts 69% <0%> (+1%) ⬆️
...tascience/jupyter/kernels/kernelCommandListener.ts 58% <0%> (+2%) ⬆️
... and 1 more

@@ -100,24 +115,28 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
private onDidChangeActiveInteractiveWindow(e?: IInteractiveWindow) {
this.interactiveContext.set(!!e).ignoreErrors();
this.updateMergedContexts();
this.updateContextOfActiveInteractiveWindowKernel();
}
Copy link
Contributor

@DonJayamanne DonJayamanne Sep 13, 2021

Choose a reason for hiding this comment

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

You can use vscode built in context variable notebookCellExecuting
That'll be easier (less code) and more responsive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we adopt that for the notebook editor as well then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried it out, that context key only works when you set focus to the notebook cell that is executing. I don't think that will work for our use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Try this notebookHasRunningCell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't work well too, e.g. if a notebook and IW are open side by side, running a cell in a notebook will cause the interrupt button in the IW toolbar to light up, and vice versa. Also in the IW case, since the IW may not actually be active (focus may be in the Python file), that would cause issues as well. The current implementation still suffers from this limitation #7487 for SxS IWs, but at least allows us to ensure that the IW toolbar is enabled correctly based on the active #%% file or active IW, and to ensure that IWs' and notebooks' run state doesn't affect the toolbar button enablement for each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

yuck, agreed.

@joyceerhl joyceerhl merged commit 84926f6 into main Sep 13, 2021
@joyceerhl joyceerhl deleted the dev/joyceerhl/iw-kernel-context branch September 13, 2021 18:14
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.

Interrupt button is enabled even when there's no code running in interactive window
4 participants