fix: create new query command opens soql builder #3091

dehru · 2021-03-25T22:21:09Z

What does this PR do?

With the last update of VSCode ( Version: 1.54.3 ), behavior around opening a new empty soql query command changed. Digging into this, i discovered we were probably riding on top of a bug that got fixed or a loop-hole that got closed. This PR fixes the issue.

Nitty Gritty Details

CustomEditors are associated with a filename pattern and not a languageId.

"customEditors": [
      {
        ...
        "selector": [
          {
            "filenamePattern": "*.soql"
          }
        ]
      }

Previously, when we created a new untitled file for a soql query, we did not specify a file name that matched our filenamePattern, but rather just specified a languageId like this:

const doc = await vscode.workspace.openTextDocument({
      language: 'soql',
      content: ''
    });

Yet, user's were still previously presented with a Soql Builder ui. Unfortunately, this stopped working in Version: 1.54.3.

This fix addresses that issue and honors the filenamePattern by naming the untitled file untitled.soql, as well as uses the special untitled scheme, which let's vscode know that the file has no title and is not in the file system ( until the user elects to save it ). There's a lot of discussion on this issue here.

Additionally, I discovered that the toggle feature requires a file on disk, and so the toggle feature does not present in the editor menu until the file is saved ( see the new when condition addition ). This also matches the existing behavior, so there is no functional change to the UX, as the toggle editor menu would not appear until the file was saved.

What issues does this PR fix or reference?

@W-8990594@

Functionality Before

create-new-bug.mov

Functionality After

create-new-fix.mov

codecov · 2021-03-25T22:21:15Z

Codecov Report

Merging #3091 (430f81a) into develop (e9d95b3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3091   +/-   ##
========================================
  Coverage    76.20%   76.21%           
========================================
  Files          276      276           
  Lines        10512    10513    +1     
  Branches      1235     1235           
========================================
+ Hits          8011     8012    +1     
  Misses        2157     2157           
  Partials       344      344

Impacted Files	Coverage Δ
...cedx-vscode-soql/src/commands/soqlBuilderToggle.ts	`100.00% <100.00%> (ø)`
...forcedx-vscode-soql/src/commands/soqlFileCreate.ts	`88.88% <100.00%> (+1.38%)`	⬆️

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 e9d95b3...430f81a. Read the comment docs.

dehru · 2021-03-25T22:25:27Z

packages/salesforcedx-vscode-soql/src/commands/soqlBuilderToggle.ts

@@ -20,9 +20,5 @@ export async function soqlBuilderToggle(doc: vscode.Uri): Promise<void> {
    ? BUILDER_VIEW_TYPE
    : EDITOR_VIEW_TYPE;

-  vscode.commands.executeCommand(
-    OPEN_WITH_COMMAND,
-    vscode.Uri.file(doc.fsPath),


this just produces a Uri, which is what doc already is. So I just use that as is since the openWith command takes a Uri.

dehru · 2021-03-25T22:27:23Z

packages/salesforcedx-vscode-soql/src/commands/soqlFileCreate.ts

+    const fileName = 'untitled.soql';
+    const newUri = vscode.Uri.file(fileName).with({
+      scheme: 'untitled',
+      path: fileName


ultimately, the Uri for an untitled file ends up looking like untitled:untitled.soql instead of when the file is on disc, which is a file: scheme.

dehru · 2021-03-25T22:28:54Z

packages/salesforcedx-vscode-soql/test/vscode-integration/commands/soqlFileCreate.test.ts

@@ -21,8 +21,6 @@ describe('soqlOpenNew should', () => {
  beforeEach(() => {
    sb = createSandbox();
    telemetryStub = sb.stub(telemetryService, 'sendCommandEvent') as SinonStub;
-    editorOpened = sb.stub();


this is no longer called, since we do not interact with the openTextDocument() api. Therefore, removed from test and associated expectations.

dehru · 2021-03-25T22:29:09Z

packages/salesforcedx-vscode-soql/test/vscode-integration/commands/soqlFileCreate.test.ts

    expect(executeCommandSpy.getCall(0).args[2]).contains(BUILDER_VIEW_TYPE);
+    expect(executeCommandSpy.getCall(0).args[1].scheme).contains('untitled');


verify the untitled scheme is used

dehru · 2021-03-25T22:31:24Z

packages/salesforcedx-vscode-soql/package.json

@@ -69,7 +69,9 @@
    "papaparse": "^5.3.0",
    "sinon": "7.5.0",
    "typescript": "3.8.3",
-    "vscode-languageclient": "6.1.3"
+    "vscode-languageclient": "6.1.3",
+    "webpack": "4.30.0",


without this, we can't actually run npm run compile from within the packages/salesforcedx-vscode-soql directory. which is a huge time saver.

This matches the version in the top level package.json. However, does anyone think i should just wildcard this? I think that would allow the version to stay in sync with the top level package.

dehru · 2021-03-25T22:48:48Z

packages/salesforcedx-vscode-soql/package.json

@@ -145,7 +147,7 @@
      ],
      "editor/title": [
        {
-          "when": "resourceLangId == soql",
+          "when": "resourceLangId == soql && resourceScheme == file",


Toggling from the text editor to the soql builder with an unsaved file causes this issue shown below. By checking the resourceScheme, we just don't show the toggle icon until the file is saved. Which prevents users from toggling back and forth and getting into this situation. Once saved, toggle works as expected.

create-new-toggle-bug.mov

dobladez · 2021-03-26T12:33:55Z

packages/salesforcedx-vscode-soql/src/commands/soqlFileCreate.ts

-    const doc = await vscode.workspace.openTextDocument({
-      language: 'soql',
-      content: ''
+    const fileName = 'untitled.soql';


If I run the "Create Query in SOQL..." command a second time before saving, nothing happens. I guess it is because it is always the same filename here.

If we are OK with this behavior, fine. If not, you might want to extract that hard-coded filename into a function to generate a unique file for the current workspace.

I messed with this a bit, and the behavior of untitled files with Custom Editors even with a numeric file sequence is not any better. Check out this movie. I'm a little dumbfounded...and ignorant still about much of the vscode OS along with the behavior of Custom Editors. If i use the openTextDocument() api, then the Save As doesn't behave as we expect.

create-query-numbering.mov

At this point, I say we go out with this behavior and see what people think. At least we are functioning again.

dobladez · 2021-03-26T12:35:09Z

Tricky stuff! Thanks for the explanations.

So, to make sure I understand: Would this not be a problem if the "customEditors" selector supported "languageId" (instead of file extension) ?

dobladez

👍 Just double-check the observation about the uniqueness of the generated filename.

jgellin-sf

Nice

dehru · 2021-03-26T14:37:20Z

Tricky stuff! Thanks for the explanations.

So, to make sure I understand: Would this not be a problem if the "customEditors" selector supported "languageId" (instead of file extension) ?

Yes. And it does kinda seem like a flaw in their design.

Co-authored-by: Jonny Hork <[email protected]>

Co-authored-by: Jonny Hork <[email protected]> Co-authored-by: Jonny Hork <[email protected]>

dehru requested a review from a team as a code owner March 25, 2021 22:21

dehru commented Mar 25, 2021

View reviewed changes

fix: create new query command opens soql builder

06769ae

dehru force-pushed the dehru/create-new-query-fix branch from 7e36727 to 06769ae Compare March 25, 2021 22:34

dehru commented Mar 25, 2021

View reviewed changes

dehru requested review from jgellin-sf, jonnyhork and dobladez March 25, 2021 22:54

dobladez reviewed Mar 26, 2021

View reviewed changes

dobladez approved these changes Mar 26, 2021

View reviewed changes

jgellin-sf approved these changes Mar 26, 2021

View reviewed changes

jonnyhork approved these changes Mar 26, 2021

View reviewed changes

jonnyhork and others added 2 commits March 26, 2021 15:33

Merge branch 'develop' into dehru/create-new-query-fix

d9af41a

Merge branch 'develop' into dehru/create-new-query-fix

430f81a

sfsholden approved these changes Mar 31, 2021

View reviewed changes

dehru merged commit 18e964f into develop Mar 31, 2021

dehru deleted the dehru/create-new-query-fix branch March 31, 2021 16:02

dehru added a commit that referenced this pull request Mar 31, 2021

fix: create new query command opens soql builder (#3091)

40e7720

Co-authored-by: Jonny Hork <[email protected]>

dehru mentioned this pull request Mar 31, 2021

fix: create new query command opens soql builder (#3091) #3107

Merged

xyc pushed a commit that referenced this pull request Mar 31, 2021

fix: create new query command opens soql builder (#3091) (#3107)

0715c0a

Co-authored-by: Jonny Hork <[email protected]> Co-authored-by: Jonny Hork <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: create new query command opens soql builder #3091

fix: create new query command opens soql builder #3091

dehru commented Mar 25, 2021 •

edited

Loading

codecov bot commented Mar 25, 2021 •

edited

Loading

dehru Mar 25, 2021

dehru Mar 25, 2021

dehru Mar 25, 2021

dehru Mar 25, 2021

dehru Mar 25, 2021 •

edited

Loading

dehru Mar 25, 2021

dobladez Mar 26, 2021

dehru Mar 26, 2021

dehru Mar 26, 2021

dobladez commented Mar 26, 2021

dobladez left a comment

jgellin-sf left a comment

dehru commented Mar 26, 2021

		expect(executeCommandSpy.getCall(0).args[2]).contains(BUILDER_VIEW_TYPE);
		expect(executeCommandSpy.getCall(0).args[1].scheme).contains('untitled');

fix: create new query command opens soql builder #3091

fix: create new query command opens soql builder #3091

Conversation

dehru commented Mar 25, 2021 • edited Loading

What does this PR do?

Nitty Gritty Details

What issues does this PR fix or reference?

Functionality Before

Functionality After

codecov bot commented Mar 25, 2021 • edited Loading

Codecov Report

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

dehru Mar 25, 2021 • 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

dobladez commented Mar 26, 2021

dobladez left a comment

Choose a reason for hiding this comment

jgellin-sf left a comment

Choose a reason for hiding this comment

dehru commented Mar 26, 2021

dehru commented Mar 25, 2021 •

edited

Loading

codecov bot commented Mar 25, 2021 •

edited

Loading

dehru Mar 25, 2021 •

edited

Loading