Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6469

joyceerhl · 2021-07-01T01:17:18Z

Debug Cell problem was because executeObservable has side effects (updating execution count) that the notebook.cell.execute codepath doesn't.

Go to cell problem was because notebook cell metadata wasn't populated.

Cell markers were caused by interactive window editor taking focus when it shouldn't (required fix in core).

Collapse cell problem was because clicking on a toolbar button does not set focus to the interactive window, and the command handler relied on the activeNotebookEditor instead of using the interactive/toolbar context.

Will be porting this PR to release branch.

codecov-commenter · 2021-07-01T01:48:51Z

Codecov Report

Merging #6469 (15c0642) into main (9c762c6) will decrease coverage by 0%.
The diff coverage is 0%.

@@          Coverage Diff          @@
##            main   #6469   +/-   ##
=====================================
- Coverage     69%     69%   -1%     
=====================================
  Files        410     410           
  Lines      28248   28248           
  Branches    4184    4190    +6     
=====================================
- Hits       19624   19620    -4     
- Misses      6956    6962    +6     
+ Partials    1668    1666    -2

Impacted Files	Coverage Δ
...ence/interactive-window/nativeInteractiveWindow.ts	`6% <0%> (-1%)`	⬇️
...e-window/nativeInteractiveWindowCommandListener.ts	`9% <0%> (-1%)`	⬇️
src/client/common/process/pythonDaemonPool.ts	`75% <0%> (-5%)`	⬇️
...rc/client/datascience/jupyter/debuggerVariables.ts	`76% <0%> (-2%)`	⬇️
...active-common/intellisense/intellisenseProvider.ts	`74% <0%> (-1%)`	⬇️
src/client/common/utils/localize.ts	`95% <0%> (-1%)`	⬇️
...lient/datascience/jupyter/liveshare/serverCache.ts	`80% <0%> (ø)`
src/client/datascience/jupyter/jupyterNotebook.ts	`76% <0%> (+<1%)`	⬆️
src/client/common/process/pythonDaemonFactory.ts	`84% <0%> (+1%)`	⬆️
...ent/datascience/jupyter/pythonVariableRequester.ts	`74% <0%> (+1%)`	⬆️
... and 2 more

IanMatthewHuff · 2021-07-01T14:09:50Z

src/client/datascience/interactive-window/nativeInteractiveWindow.ts

-                autoReveal: true
-            });
-            return true;
+            return this.submitCodeImpl(code, file.fsPath, 0, false, notebookCell);


I was actually having to look this up myself. But this try catch block now seems funny. If you're directly returning the promise from submitCodeImpl I don't believe that the try catch block is relevant anymore. Looks to me like a case of this here: https://stackoverflow.com/questions/38708550/difference-between-return-await-promise-and-return-promise/42750371#42750371

IanMatthewHuff · 2021-07-01T14:18:53Z

src/client/datascience/interactive-window/nativeInteractiveWindowCommandListener.ts

-        const interactiveWindow = this.interactiveWindowProvider.activeWindow;
+    private expandAllCells(uri?: Uri) {
+        const interactiveWindow = uri
+            ? this.interactiveWindowProvider.windows.find((window) => window.notebookUri!.toString() === uri.toString())


From the code it looks like notebookUri is always defined, but the "!" feels a tiny bit less typesafe here since it is optional on the interface. Would ? accomplish the same thing since undefined won't equal uri.toString()?

Ohh, looks like you have ? below for exportAs (was just reading on more). Can we use that in these locations as well.

rchiodo · 2021-07-01T16:02:37Z

src/client/datascience/interactive-window/nativeInteractiveWindowCommandListener.ts

@@ -454,7 +466,7 @@ export class NativeInteractiveWindowCommandListener {

    private export(uri?: Uri) {
        const interactiveWindow = uri
-            ? this.interactiveWindowProvider.windows.find((window) => window.notebookUri!.toString() === uri.toString())
+            ? this.interactiveWindowProvider.windows.find((window) => window.notebookUri?.toString() === uri.toString())


Maybe this 'find matching interactive window' could be on the intearctiveWindowProvider? And then everyone of these functions would use that code instead?

Unfortunately we're still using the IInteractiveWindowProvider interface which is shared for webview, so I'd have to put that method on the old provider too (and it doesn't make sense there). I tracked this item in a list of todos for webview code cleanup #6488 feel free to add more there as well :)

…and/collapse all from toolbar when focus is not on notebook editor (#6469) * fix #6423 * Debug cell fixes * update execution count. * Expand/collapse cell should not rely on `activeNotebookEditor` * Format * Refactor add/debug code codepaths * Prettier Co-authored-by: rebornix <penn.lv@gmail.com>

…and/collapse all from toolbar when focus is not on notebook editor (#6469) (#6493) * fix #6423 * Debug cell fixes * update execution count. * Expand/collapse cell should not rely on `activeNotebookEditor` * Format * Refactor add/debug code codepaths * Prettier Co-authored-by: rebornix <penn.lv@gmail.com> Co-authored-by: rebornix <penn.lv@gmail.com>

* prepare release (#6403) * Port jupyter kernel fix to release branch (#6486) * Fix some problems running kernels with 'jupyter' or no raw kernel (#6464) * Fix for 6409 * Jupyter server specific case for kernel data not being used * Update changelog * Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor (#6469) (#6493) * fix #6423 * Debug cell fixes * update execution count. * Expand/collapse cell should not rely on `activeNotebookEditor` * Format * Refactor add/debug code codepaths * Prettier Co-authored-by: rebornix <penn.lv@gmail.com> Co-authored-by: rebornix <penn.lv@gmail.com> * enble the debugging setting (#6492) * Port #6503 and #6504 to release (#6507) * Don't pass registrationData twice when registering notebook serializer (#6503) * If in Python daily insiders and VS Code Insiders opt into native interactive window (#6504) * If in Python daily insiders and VS Code Insiders opt into native interactive window * Oops * Remove unused import * Changelog * Format * debugging work (#6505) (#6558) * don't fetch breakpoints on activate * implement IDisposable on the debugging manager * pass on session id's * delete temp files * lint * add try catch * store sessionId * pass jupyter session id * Remove start page in favor of a message (#6576) * prepare release (#6590) * prepare * use same version in package.jpsn and changelog * leave version at 2021.8.0 * publish release * update package-lock Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> Co-authored-by: Joyce Er <joyceerhuiling@gmail.com> Co-authored-by: rebornix <penn.lv@gmail.com> Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>

rebornix and others added 3 commits June 30, 2021 14:30

fix #6423

fade08c

Debug cell fixes

2bcf7eb

update execution count.

cbc46cb

joyceerhl requested a review from a team as a code owner July 1, 2021 01:17

joyceerhl changed the title ~~Fix interactive window debug cell, codelens tracking~~ Fix interactive window debug cell, codelens tracking, go to cell Jul 1, 2021

joyceerhl added 2 commits June 30, 2021 19:00

Expand/collapse cell should not rely on activeNotebookEditor

f2c60f4

Format

78a27a3

joyceerhl changed the title ~~Fix interactive window debug cell, codelens tracking, go to cell~~ Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor Jul 1, 2021

IanMatthewHuff reviewed Jul 1, 2021

View reviewed changes

Refactor add/debug code codepaths

39692b3

rchiodo reviewed Jul 1, 2021

View reviewed changes

rchiodo approved these changes Jul 1, 2021

View reviewed changes

DavidKutu approved these changes Jul 1, 2021

View reviewed changes

Prettier

15c0642

IanMatthewHuff approved these changes Jul 1, 2021

View reviewed changes

joyceerhl merged commit 617fccc into main Jul 1, 2021

joyceerhl deleted the dev/rebornix/iw-regressions branch July 1, 2021 18:11

joyceerhl mentioned this pull request Jul 1, 2021

Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6493

Merged

rebornix mentioned this pull request Jul 2, 2021

Interactive window debugging of cells doesn't work #6426

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6469

Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6469

joyceerhl commented Jul 1, 2021 •

edited

Loading

codecov-commenter commented Jul 1, 2021 •

edited

Loading

IanMatthewHuff Jul 1, 2021

IanMatthewHuff Jul 1, 2021

IanMatthewHuff Jul 1, 2021

rchiodo Jul 1, 2021

joyceerhl Jul 1, 2021

Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6469

Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6469

Conversation

joyceerhl commented Jul 1, 2021 • edited Loading

codecov-commenter commented Jul 1, 2021 • edited Loading

Codecov Report

IanMatthewHuff Jul 1, 2021

Choose a reason for hiding this comment

IanMatthewHuff Jul 1, 2021

Choose a reason for hiding this comment

IanMatthewHuff Jul 1, 2021

Choose a reason for hiding this comment

rchiodo Jul 1, 2021

Choose a reason for hiding this comment

joyceerhl Jul 1, 2021

Choose a reason for hiding this comment

joyceerhl commented Jul 1, 2021 •

edited

Loading

codecov-commenter commented Jul 1, 2021 •

edited

Loading