Fix 766, Uploading a document by closing it no longer tries to update the closed editor #818

tec-goblin · 2018-09-13T17:49:16Z

Explanation: doc.isClosed (as its doc says) returns true if there are no visible editors for that document. You can get multiple editors open for a document for example by using Shift Left on the contextual menu of the first editor's tab.
I checked there are no regressions. I tested also by having other editors open, and also having twice the same editor open.
While I managed to reproduce the issue easily with the current version of the plugin in the marketplace, when trying the same in the emulator with the master, it doesn't even try to save the file. UploadToCloud is not called. That's weird and definitely not caused by this change.

Desired behaviour: when closing a cosmos document with pending changes, 1 get prompted to save it, then (depending on settings) 2 get prompted to upload it. Then 3 if there are still open editors for that document (it is possible to have 2 editors open for the same document), they are updated. If not, we don't update them.
Behaviour of the current version of the plugin in the marketplace: 1,2 as above, then 3 if there no other editors open, an error is shown. If there are any other editors shown, the active one has its contents replaced with the document you just uploaded to the cloud.
Behaviour of the master (at least in the extensions host): 1 never happens, so steps 2 and 3 don't happen either. This pull request only impacts step 3.

PrashanthCorp · 2018-09-13T18:21:24Z

src/CosmosEditorManager.ts

@@ -95,7 +95,11 @@ export class CosmosEditorManager {
        const timestamp = (new Date()).toLocaleTimeString();
        ext.outputChannel.appendLine(`${timestamp}: Updated entity "${editor.label}"`);
        ext.outputChannel.show();
-        await this.updateEditor(updatedContent, vscode.window.activeTextEditor, editor);
+        if (doc.isClosed !== true) {
+            const firstRelatedEditor = vscode.window.visibleTextEditors.filter((ed) => ed.document === doc)[0];


Thanks for helping out with this issue! We appreciate you taking the time to figure out the root cause and the interesting side effect - the extension picking up another activeEditor when this doc is closed - that was nice catch!

I just have a couple of clarifications:

The [0] suggests you're looking at the first matching document only. With that in mind, the comment two lines below could be a little confusing.
The interesting case could be where you split the editor (Ctrl + '\' in windows; icon below). Here there would be two views of the same document. From my testing, the views are in sync, so editing one view updates the other. I'm wondering if this implementation would work fine in that case? If you haven't tried it, the steps below could help:
open a document in a cosmos-document.json -> split the editor twice -> You should get 3 columns -> then repro the original bug.

Just curious, is there a case (for example a race condition) where doc.isClosed is false, but doc is not present in visibleTextEditors? The if condition and the filter seem to be checking for the same thing. :)

Icon appears in top right corner, highlighted in red here -

Thank you for the encouragement! Those two points surprised me too when implementing (I started with just a simple if doc.isClosed and then from its documentation I realised the issue of multiple editors for the same document).

I am looking for the first matching editor. When updating an editor through the API, VS Code updates the underlying document, thus all visible editors. I tested that with multiple open editors. It's also implied by the documentation of vscode.TextEditorEdit

I don't think it could happen, taking into account the single-threaded model of javascript. That said, the condition is what allows me to safely do [0] in the next line. An alternative (but slightly slower) code would be:

const relatedEditors = vscode.window.visibleTextEditors.filter((ed) => ed.document === doc); if (relatedEditors.length > 0) { //update relatedEditors[0]

@PrashanthCorp I suppose I'm not supposed to resolve the conversation myself :).

Just curious, is there a case (for example a race condition) where doc.isClosed is false, but doc is not present in visibleTextEditors? The if condition and the filter seem to be checking for the same thing. :)

It's not a race condition, but this is possible. For example, follow these steps:

Edit the file, but don't save

Open another file (so that it covers the other tab)

Select "File -> Save All"

In this case, isClosed is false because the editor is still open, but it's not in the list of visibleTextEditors because it's behind the other tab.

Please just do if (firstRelatedEditor) { before you call updateEditor

I misunderstood that visibleTextEditors would be all open text editors (it's not clear from the doc)
But in that case, if (firstRelatedEditor) won't do what we want: we want to update the hidden (open) editor with what we got from the server. I don't see a way to get "all open editors" . Anyway, just doing a if (firstRelatedEditor) is already an improvement. I will update the pull request immediately

Yes I agree this is an improvement and is worth getting in - even though it's not perfect

ejizba · 2018-09-27T17:06:38Z

src/CosmosEditorManager.ts

@@ -95,7 +95,11 @@ export class CosmosEditorManager {
        const timestamp = (new Date()).toLocaleTimeString();
        ext.outputChannel.appendLine(`${timestamp}: Updated entity "${editor.label}"`);
        ext.outputChannel.show();
-        await this.updateEditor(updatedContent, vscode.window.activeTextEditor, editor);
+        if (doc.isClosed !== true) {
+            const firstRelatedEditor = vscode.window.visibleTextEditors.filter((ed) => ed.document === doc)[0];


nit: Instead of .filter()[0], you can use .find() which automatically returns the first object found.

Thank you, I didn't know that. I will send a fix.

ejizba · 2018-09-27T17:12:57Z

src/CosmosEditorManager.ts

@@ -95,7 +95,11 @@ export class CosmosEditorManager {
        const timestamp = (new Date()).toLocaleTimeString();
        ext.outputChannel.appendLine(`${timestamp}: Updated entity "${editor.label}"`);
        ext.outputChannel.show();
-        await this.updateEditor(updatedContent, vscode.window.activeTextEditor, editor);
+        if (doc.isClosed !== true) {
+            const firstRelatedEditor = vscode.window.visibleTextEditors.filter((ed) => ed.document === doc)[0];


Just curious, is there a case (for example a race condition) where doc.isClosed is false, but doc is not present in visibleTextEditors? The if condition and the filter seem to be checking for the same thing. :)

It's not a race condition, but this is possible. For example, follow these steps:

Edit the file, but don't save

Open another file (so that it covers the other tab)

Select "File -> Save All"

In this case, isClosed is false because the editor is still open, but it's not in the list of visibleTextEditors because it's behind the other tab.

Please just do if (firstRelatedEditor) { before you call updateEditor

ejizba · 2018-09-27T17:14:23Z

src/docdb/tree/DocDBDatabaseTreeItemBase.ts

@@ -68,7 +68,7 @@ export abstract class DocDBDatabaseTreeItemBase extends DocDBTreeItemBase<Collec
    // Create a DB collection
    public async createChild(_node: IAzureNode, showCreatingNode: (label: string) => void): Promise<IAzureTreeItem> {
        const collectionName = await vscode.window.showInputBox({
-            placeHolder: `Enter a name for your ${this.childTypeLabel}`,
+            placeHolder: `Enter an Id for your ${this.childTypeLabel}`,


Also - please keep this change separate

Oups, newbie in GitHub mistake - for the next changes I did one branch for each pull request. Sorry

…ng the commit clean

tec-goblin added 5 commits September 9, 2018 17:26

fix microsoft#808

003b265

fix microsoft#792

5e72ecc

Merge branch 'master' of https://github.com/tec-goblin/vscode-cosmosdb

fb54ab4

fix microsoft#766

295d7de

Merge branch 'master' of git://github.com/Microsoft/vscode-cosmosdb

653a0b9

PrashanthCorp reviewed Sep 13, 2018

View reviewed changes

fix microsoft#813

d8ad8a3

ejizba reviewed Sep 27, 2018

View reviewed changes

fixed feedback on find, taking into account hidden editors, and keepi…

6c29bcd

…ng the commit clean

ejizba approved these changes Sep 27, 2018

View reviewed changes

ejizba merged commit a6e8aa4 into microsoft:master Sep 27, 2018

ejizba mentioned this pull request Sep 27, 2018

Selecting 'Save' pops up an error after closing the document in editor #766

Closed

github-actions bot locked and limited conversation to collaborators Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 766, Uploading a document by closing it no longer tries to update the closed editor #818

Fix 766, Uploading a document by closing it no longer tries to update the closed editor #818

tec-goblin commented Sep 13, 2018

PrashanthCorp Sep 13, 2018 •

edited

Loading

tec-goblin Sep 14, 2018

tec-goblin Sep 17, 2018

ejizba Sep 27, 2018

tec-goblin Sep 27, 2018

ejizba Sep 27, 2018

ejizba Sep 27, 2018

tec-goblin Sep 27, 2018

ejizba Sep 27, 2018

ejizba Sep 27, 2018

tec-goblin Sep 27, 2018

Fix 766, Uploading a document by closing it no longer tries to update the closed editor #818

Fix 766, Uploading a document by closing it no longer tries to update the closed editor #818

Conversation

tec-goblin commented Sep 13, 2018

PrashanthCorp Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrashanthCorp Sep 13, 2018 •

edited

Loading