Fix save on close #13567

rchiodo · 2020-08-21T21:10:20Z

For #11711

Root cause seems to be the change for the new FS that removed this line here:

    private async getStoredContentsFromFile(file: Uri, key: string): Promise<string | undefined> {
        const filePath = this.getHashedFileName(key); <-- This line was removed and the passed in file was used instead.
        try {

However our logic around saving was working before that by sheer luck as far as I can tell. It would always read from the backup file (in order to load the model).

For the old web view, it's much simpler to use the model already in use.

Added a couple of tests to make sure this doesn't regress again.

Assuming I can get all of the tests to pass, somebody else may have to submit this as I'll be out next week.

rchiodo · 2020-08-21T21:11:12Z

src/client/datascience/notebookStorage/nativeEditorProvider.ts

-        // Every time we load a new untitled file, up the counter past the max value for this counter
-        this.untitledCounter = getNextUntitledCounter(file, this.untitledCounter);
+        // Get the model that may match this file
+        let model = [...this.models.values()].find((m) => this.fs.arePathsSame(m.file, file));


This was necessary for custom editor to find the correct model.

rchiodo · 2020-08-21T21:11:36Z

src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts

-                async (resource: Uri, targetResource: Uri) => {
-                    const customDocument = this.customDocuments.get(resource.fsPath);
+                async (model: INotebookModel, targetResource: Uri) => {
+                    await this.storage.saveAs(model, targetResource);


This makes sure the correct model is used for saving.

rchiodo · 2020-08-21T21:11:56Z

src/client/datascience/notebookStorage/nativeEditorStorage.ts

    }

    private async getStoredContentsFromFile(file: Uri, key: string): Promise<string | undefined> {
+        const filePath = this.getHashedFileName(key);


This line was the removed one. I believe it's the root cause of the problem.

codecov-commenter · 2020-08-21T21:19:57Z

Codecov Report

Merging #13567 into master will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #13567      +/-   ##
==========================================
- Coverage   59.74%   59.71%   -0.03%     
==========================================
  Files         670      670              
  Lines       37427    37434       +7     
  Branches     5371     5373       +2     
==========================================
- Hits        22359    22353       -6     
- Misses      13905    13914       +9     
- Partials     1163     1167       +4

Impacted Files	Coverage Δ
...atascience/notebookStorage/nativeEditorProvider.ts	`17.26% <0.00%> (-0.39%)`	⬇️
...datascience/notebookStorage/nativeEditorStorage.ts	`66.98% <88.88%> (+0.15%)`	⬆️
src/client/common/utils/platform.ts	`64.70% <0.00%> (-11.77%)`	⬇️
src/client/datascience/crossProcessLock.ts	`79.41% <0.00%> (-11.77%)`	⬇️
src/client/linters/pydocstyle.ts	`86.66% <0.00%> (-2.23%)`	⬇️
src/client/datascience/debugLocationTracker.ts	`76.56% <0.00%> (-1.57%)`	⬇️
src/client/common/process/proc.ts	`14.49% <0.00%> (-0.73%)`	⬇️

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 1c44def...1f89914. Read the comment docs.

sonarqubecloud · 2020-08-21T21:57:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

IanMatthewHuff · 2020-08-21T23:07:58Z

src/datascience-ui/interactive-common/redux/store.ts

-                redoCount: afterState.main.redoStack.length,
-                selectedCell: currentSelection.selectedCellId
-            });
+        if (!action.type || action.type !== CommonActionType.UNMOUNT) {


What was this change for?

It was necessary to add the check for the mounted web view being used by dispose. We were sending messages after the webview had been destroyed.

* Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests

* Show the server display string that the user is going to connect to after selecting a compute instance and reloading the window. (#13600) * Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing (#13554) * Update vscode-tas-client * Add experiment group enum * Add method to retrieve experiment values * Implementation + tests * News file * Update wording of the news entry * Add telemetry * More tests * No opting-in and out of this one * Don't fetch value if opted out, add tests * Address comments * Fix save on close (#13567) * Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests * Update changelog and version for point release. * Change date. * Format using latest black (#13625) Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com>

* Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests

* Show the server display string that the user is going to connect to after selecting a compute instance and reloading the window. (#13600) * Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing (#13554) * Update vscode-tas-client * Add experiment group enum * Add method to retrieve experiment values * Implementation + tests * News file * Update wording of the news entry * Add telemetry * More tests * No opting-in and out of this one * Don't fetch value if opted out, add tests * Address comments * Fix save on close (#13567) * Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests * Update changelog and version for point release. * Skip workspace virtual env service tests * Add link to issue for skipping Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com>

* Port gather survey and icon getting disabled while gathering (#13296) * Add gather status bar and update survey link (#13225) * add status bar * add localization * Added news file * add a 2 second timeout * updated the gather survey link * Change the gather icon while gathering * moved the gathering redux function to interactive common added a constant * move css to common.css * removed the status bar message * changed noop import * update changelog * Disable test (#13346) (#13350) Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> * Add cell navigation commands as well as a default set of keyboard bindings for most new interactive window cell commands (#13334) (#13348) * Add Goto Prev/Next Cell * Add goto previous cell * Remove unneeded changes * Add default keybindings for cell commands * Goto should be Go to Co-authored-by: Jim Griesmer <jimg@live.com> Co-authored-by: Jim Griesmer <jimg@live.com> * Autoscroll when extending cell select (#13353) (#13354) Code change can only affect new keyboard shortcut operations * Update Release branch for August Release (#13401) * update version number and npm install to update package-lock.json * update changelog version and date * update news for changelog.md * update generated TPN Co-authored-by: Ian Huff <ianhuff@ravikun-dev2.redmond.corp.microsoft.com> * Update version and change log for bug fix release (#13537) * Update LSP to latest to resolve problems with LS settings (#13511) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Update LSP to fix issue with settings in MPLS v1 * Add telemetry for `switch to Pylance` acceptance (#13526) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Initial * Add test * PR feedback * Rename property * Rename constant * Define property * Moar rename * casing * Update version and changelogs * Show status of Jupyter server (Connected/Disconnected) when the user hovers over the status icon (#13544) * Update change log with more cherry picks Co-authored-by: Mikhail Arkhipov <mikhaila@microsoft.com> Co-authored-by: Shantnu Suman <shsuman@microsoft.com> * Update changelog and version for point release. (#13610) * Show the server display string that the user is going to connect to after selecting a compute instance and reloading the window. (#13600) * Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing (#13554) * Update vscode-tas-client * Add experiment group enum * Add method to retrieve experiment values * Implementation + tests * News file * Update wording of the news entry * Add telemetry * More tests * No opting-in and out of this one * Don't fetch value if opted out, add tests * Address comments * Fix save on close (#13567) * Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests * Update changelog and version for point release. * Change date. * Format using latest black (#13625) Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> * Skip flaky virtual env tests (#13650) * Show the server display string that the user is going to connect to after selecting a compute instance and reloading the window. (#13600) * Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing (#13554) * Update vscode-tas-client * Add experiment group enum * Add method to retrieve experiment values * Implementation + tests * News file * Update wording of the news entry * Add telemetry * More tests * No opting-in and out of this one * Don't fetch value if opted out, add tests * Address comments * Fix save on close (#13567) * Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests * Update changelog and version for point release. * Skip workspace virtual env service tests * Add link to issue for skipping Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> * Cherry picks, change log, and version update for point release. (#13666) * Add install telemetry (#13653) * Add extension install telemetry. * Add tests. * Fix typo * Fix tests. * Update comment to reflect what the installed actually does. * Update change log and version for point release. * Cherry pick history bug for Azure ML CI && Make Jupyter server clickable (#13712) * Fix: Getting an error on selecting an existing CI which has a display name from the historical CI's in the quick picks (#13642) * Make Jupyter Server name clickale to select Jupyter server (#13657) * Update Changelog and remove news files * Update package version. (#13748) * Update and pin LSP libraries for new APIs (#13725) (#13756) Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com> * PORT showStartPage setting fix (#13706) (#13758) * fix showStartPage setting on configSettings.ts (#13706) * fix showStartPage setting on configSettings.ts * add news file * Fix unit tests * oops * update changelog * Fix merge issues * Clean up news Co-authored-by: David Kutugata <dakutuga@microsoft.com> Co-authored-by: Ian Huff <ianhu@microsoft.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> Co-authored-by: Jim Griesmer <jimgries@microsoft.com> Co-authored-by: Jim Griesmer <jimg@live.com> Co-authored-by: Ian Huff <ianhuff@ravikun-dev2.redmond.corp.microsoft.com> Co-authored-by: Mikhail Arkhipov <mikhaila@microsoft.com> Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

* Port gather survey and icon getting disabled while gathering (#13296) * Add gather status bar and update survey link (#13225) * add status bar * add localization * Added news file * add a 2 second timeout * updated the gather survey link * Change the gather icon while gathering * moved the gathering redux function to interactive common added a constant * move css to common.css * removed the status bar message * changed noop import * update changelog * Disable test (#13346) (#13350) Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> * Add cell navigation commands as well as a default set of keyboard bindings for most new interactive window cell commands (#13334) (#13348) * Add Goto Prev/Next Cell * Add goto previous cell * Remove unneeded changes * Add default keybindings for cell commands * Goto should be Go to Co-authored-by: Jim Griesmer <jimg@live.com> Co-authored-by: Jim Griesmer <jimg@live.com> * Autoscroll when extending cell select (#13353) (#13354) Code change can only affect new keyboard shortcut operations * Update Release branch for August Release (#13401) * update version number and npm install to update package-lock.json * update changelog version and date * update news for changelog.md * update generated TPN Co-authored-by: Ian Huff <ianhuff@ravikun-dev2.redmond.corp.microsoft.com> * Update version and change log for bug fix release (#13537) * Update LSP to latest to resolve problems with LS settings (#13511) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Update LSP to fix issue with settings in MPLS v1 * Add telemetry for `switch to Pylance` acceptance (#13526) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Initial * Add test * PR feedback * Rename property * Rename constant * Define property * Moar rename * casing * Update version and changelogs * Show status of Jupyter server (Connected/Disconnected) when the user hovers over the status icon (#13544) * Update change log with more cherry picks Co-authored-by: Mikhail Arkhipov <mikhaila@microsoft.com> Co-authored-by: Shantnu Suman <shsuman@microsoft.com> * Update changelog and version for point release. (#13610) * Show the server display string that the user is going to connect to after selecting a compute instance and reloading the window. (#13600) * Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing (#13554) * Update vscode-tas-client * Add experiment group enum * Add method to retrieve experiment values * Implementation + tests * News file * Update wording of the news entry * Add telemetry * More tests * No opting-in and out of this one * Don't fetch value if opted out, add tests * Address comments * Fix save on close (#13567) * Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests * Update changelog and version for point release. * Change date. * Format using latest black (#13625) Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> * Skip flaky virtual env tests (#13650) * Show the server display string that the user is going to connect to after selecting a compute instance and reloading the window. (#13600) * Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing (#13554) * Update vscode-tas-client * Add experiment group enum * Add method to retrieve experiment values * Implementation + tests * News file * Update wording of the news entry * Add telemetry * More tests * No opting-in and out of this one * Don't fetch value if opted out, add tests * Address comments * Fix save on close (#13567) * Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests * Update changelog and version for point release. * Skip workspace virtual env service tests * Add link to issue for skipping Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> * Cherry picks, change log, and version update for point release. (#13666) * Add install telemetry (#13653) * Add extension install telemetry. * Add tests. * Fix typo * Fix tests. * Update comment to reflect what the installed actually does. * Update change log and version for point release. * Cherry pick history bug for Azure ML CI && Make Jupyter server clickable (#13712) * Fix: Getting an error on selecting an existing CI which has a display name from the historical CI's in the quick picks (#13642) * Make Jupyter Server name clickale to select Jupyter server (#13657) * Update Changelog and remove news files * Update package version. (#13748) * Update and pin LSP libraries for new APIs (#13725) (#13756) Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com> * PORT showStartPage setting fix (#13706) (#13758) * fix showStartPage setting on configSettings.ts (#13706) * fix showStartPage setting on configSettings.ts * add news file * Fix unit tests * oops * update changelog * Fixes for point release (#13841) * Use 'main' branch for experiments. * Update changelog and versions * Update CHANGELOG.md * Replace 'master' with 'main' in readme (#13845) Co-authored-by: David Kutugata <dakutuga@microsoft.com> Co-authored-by: Ian Huff <ianhu@microsoft.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> Co-authored-by: Jim Griesmer <jimgries@microsoft.com> Co-authored-by: Jim Griesmer <jimg@live.com> Co-authored-by: Ian Huff <ianhuff@ravikun-dev2.redmond.corp.microsoft.com> Co-authored-by: Mikhail Arkhipov <mikhaila@microsoft.com> Co-authored-by: Shantnu Suman <shsuman@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

rchiodo added 4 commits August 21, 2020 09:35

Pass model through command instead of URI in order to use directly

5dda9bd

Add a test to verify we don't regress

3c86fbf

Finish fixing tests

1fe7f41

Add news entry

187bc39

rchiodo requested review from DavidKutu, DonJayamanne, IanMatthewHuff, greazer and joyceerhl August 21, 2020 21:10

rchiodo self-assigned this Aug 21, 2020

rchiodo commented Aug 21, 2020

View reviewed changes

DavidKutu approved these changes Aug 21, 2020

View reviewed changes

Fix unit tests

1f89914

IanMatthewHuff reviewed Aug 21, 2020

View reviewed changes

IanMatthewHuff approved these changes Aug 21, 2020

View reviewed changes

rchiodo merged commit c3af16d into master Aug 21, 2020

rchiodo deleted the rchiodo/fix_save_on_close branch August 21, 2020 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix save on close #13567

Fix save on close #13567

Uh oh!

rchiodo commented Aug 21, 2020

Uh oh!

rchiodo Aug 21, 2020

Uh oh!

rchiodo Aug 21, 2020

Uh oh!

rchiodo Aug 21, 2020

Uh oh!

codecov-commenter commented Aug 21, 2020 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Aug 21, 2020

Uh oh!

IanMatthewHuff Aug 21, 2020

Uh oh!

rchiodo Aug 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix save on close #13567

Fix save on close #13567

Uh oh!

Conversation

rchiodo commented Aug 21, 2020

Uh oh!

rchiodo Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

rchiodo Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

rchiodo Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Aug 21, 2020

Uh oh!

IanMatthewHuff Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

rchiodo Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Aug 21, 2020 •

edited

Loading