iframe-proxy

sherryyshi · 2018-04-09T17:17:16Z

Create an API for registering configurations in the currently active language client.

sherryyshi · 2018-04-09T17:24:33Z

WardenGnaw

Thank you for your contribution. I will wait on a full review until you mention it is ready.

I have just a few comments that can be addressed.

You can also ignore the
continuous-integration/appveyor/pr failure. That was a leftover artifact from an investigation on testing on Windows.

WardenGnaw · 2018-04-10T17:19:39Z

 	"compilerOptions": {
 		"module": "commonjs",
 		"target": "es6",
+		"declaration": false,


The default value of declaration is false. Was there confusion that this needed to be included?

Nope, I was experimenting and checked this in by accident :)

WardenGnaw · 2018-04-10T17:21:04Z

@@ -38,7 +38,26 @@
    "Linters"


The file is rewritten when the extension activates. Please only check in changes you have made to the package.json, and not the extension's rewritten content.

sherryyshi · 2018-04-10T20:29:35Z

@WardenGnaw thanks! I think this is ready to be reviewed. If you have questions regarding the background/purpose of this PR, feel free to contact me. #Closed

WardenGnaw · 2018-04-10T20:34:14Z

@sherryyshi This seems to be mostly configurations from the Language Service. I will have @sean-mcmanus and @bobbrow to take a look at this PR. They may have questions about the purpose of these changes.

Travis-CI is complaining about a couple errors. This it to make sure that the changes follow style guidelines, the project compiles, and it properly activates the extension for debugging. You can find more about the errors in the details link next to
continuous-integration/travis-ci/pr.

sean-mcmanus · 2018-04-10T21:43:49Z

Bob said he was going to code review this. #Closed

bobbrow · 2018-04-11T17:58:10Z

+ * ------------------------------------------------------------------------------------------ */
+'use strict';
+
+export interface ICppTools {


I [](start = 17, length = 1)

nit: we don't use Hungarian naming conventions in TypeScript. Please remove the 'I' from the interface name. #Closed

bobbrow · 2018-04-11T19:34:53Z

@sherryyshi, I sent you an email about this. Let's discuss over email or we can set up another meeting if necessary. #Closed

sherryyshi · 2018-04-16T20:34:48Z

    }

+    public sendCustomConfiguration(document: vscode.TextDocument, config: SourceFileConfiguration): void {
+        // TODO: Implement this


I would need some help on this.

Take a look at the notifications defined at the top of this file under the "Notifications to the server" comment. You'll want to follow that pattern to create the JSON we'll want to send to the language server. It would probably be a vscode.Uri paired with a SourceFileConfiguration.

ChangeFolderSettingsNotification is probably the closest in function to what you will be doing. Then do

this.notifyWhenReady(() => this.languageClient.sendNotification(CustomConfigurationNotification, params));

You should move this method down under the "notifications to the language server" comment below. There are plenty of examples to emulate down there as well. #Closed

bobbrow · 2018-04-16T21:30:52Z

@@ -1,3 +1,3 @@
-{
-    "intelliSenseEngine_default_percentage": 100
+{


revert changes to this file (did you remove the last newline?) #Closed

bobbrow · 2018-04-16T21:31:22Z

+    defines: string[];        // This must also include the compiler default defines (_MSC_VER, __cplusplus, etc)
+    intelliSenseMode: string; // Probably just ‘msvc-x64’ for Windows
+    forcedInclude?: string[]; // Any files that need to be included before the source file is parsed
+   standard: string;         // This is the C or C++ standard (language server will determine if the file is C or C++ based on this)


fix indentation (off by one) #Closed

bobbrow · 2018-04-16T21:31:44Z

+
+export interface SourceFileConfiguration {
+    includePaths: string[];    // This must also include the system include path (compiler defaults)
+    defines: string[];        // This must also include the compiler default defines (_MSC_VER, __cplusplus, etc)


remove _MSC_VER (Windows-specific) #Closed

bobbrow · 2018-04-16T21:32:01Z

+export interface SourceFileConfiguration {
+    includePaths: string[];    // This must also include the system include path (compiler defaults)
+    defines: string[];        // This must also include the compiler default defines (_MSC_VER, __cplusplus, etc)
+    intelliSenseMode: string; // Probably just ‘msvc-x64’ for Windows


Remove Windows-specific comment #Closed

bobbrow · 2018-04-16T21:32:37Z

+ * ------------------------------------------------------------------------------------------ */
+'use strict';
+
+import { CppToolsApi, CustomConfigurationProvider } from "./api";


nit: we always use single quote ' for imports #Closed

bobbrow · 2018-04-16T21:52:57Z

+
+                // If custom config provider exists, ask for the custom configuration and send a notification to the server
+                // before the textDocument/didOpen message so that the right information is used when the file is opened.
+                me.CustomConfigurationProvider.provideConfiguration(document.uri, (config: SourceFileConfiguration) => {


Make sure me.CustomConfigurationProvider is valid first. #Closed

bobbrow · 2018-04-16T21:55:06Z

+                // If custom config provider exists, ask for the custom configuration and send a notification to the server
+                // before the textDocument/didOpen message so that the right information is used when the file is opened.
+                me.CustomConfigurationProvider.provideConfiguration(document.uri, (config: SourceFileConfiguration) => {
+                    me.sendCustomConfiguration(document, config);  // We have to add this method to client.ts


remove this comment (you already added the method) #Closed

bobbrow · 2018-04-16T21:56:29Z

+
+export interface CustomConfigurationProvider {
+    name: string;
+    provideConfiguration(uri: vscode.Uri, callback: (config: SourceFileConfiguration) => any): void;


Don't make this a callback interface. We need the answer immediately, so this method should just return it. #Closed

Hmm... Our server needs to do this asynchronously because it's communication with an out-of-proc process vis json rpc... #Closed

Making this async may result in vscode messages being sent out of order which will result in undefined behavior and confuse the language server. If you can't synchronize here because you think that your out of proc server is going to take too long to service the request, then we'll need to rethink this. Regardless of what we do here, sendMessage needs to be called before this method returns so you might need to block anyway unless we decide to let the language server open the file with the wrong config until your extension comes back with the correct one.

We probably need some kind of timeout and/or a try/catch block in here to ensure the safety of the provider call (in case a bad extension hangs or crashes) #Closed

bobbrow · 2018-04-16T21:58:10Z

+                // before the textDocument/didOpen message so that the right information is used when the file is opened.
+                me.CustomConfigurationProvider.provideConfiguration(document.uri, (config: SourceFileConfiguration) => {
+                    me.sendCustomConfiguration(document, config);  // We have to add this method to client.ts
+                    sendMessage(document);


This needs to come outside the callback. This message must always be sent. #Closed

bobbrow · 2018-04-16T22:03:12Z


    reportMacCrashes();

+    eventEmitter.emit(activationCompleteEventId);


Having this event may be unnecessary if we just keep track of the providers in this file and send them down to the Client when they are available. I haven't thought this all the way through yet. I might be able to be more prescriptive once you get the UI part in for selecting providers. #Closed

…ging callback implementation for provideConfiguration

bobbrow · 2018-04-17T17:43:35Z

    Name: string = "(empty)";
    TrackedDocuments = new Set<vscode.TextDocument>();
-    CustomConfigurationProvider: CustomConfigurationProvider;
+    CustomConcigurationProviders: CustomConfigurationProvider[] = [];


typo #Closed

bobbrow · 2018-04-17T21:36:54Z

+            uri: document.uri.toString(),
+            configuration: config
+        };
+        this.notifyWhenReady(() => this.languageClient.sendNotification(ChangeCompileCommandsNotification, params));


ChangeCompileCommandsNotification [](start = 72, length = 33)

ChangeCompileCommandsNotification -> CustomConfigurationNotification #Closed

… with a timeout

…ith batched version(provideConfigurations).

saqadri · 2018-05-01T23:46:00Z

+import * as vscode from 'vscode';
+
+export interface CppToolsApi {
+    registerCustomConfigurationProvider(provider: CustomConfigurationProvider): void;


registerCustomConfigurationProvider [](start = 4, length = 35)

Not blocking for this PR, but can you add comments in here (for each file/property/interface), especially for the API functions? Because this is the public contract for this extension, it should be well-documented

saqadri · 2018-05-01T23:51:42Z

+     * wait until the all pendingTasks are complete (e.g. language client is ready for use)
+     * before attempting to send messages
     *************************************************************************************/
+    public runBlockingTask(task: Thenable<any>): Thenable<any> {


any [](start = 42, length = 3)

Should this function be generic (i.e. accept type T), so this can be Thenable?

Same for requestWhenReady below

In reply to: 185363898 [](ancestors = 185363898)

saqadri · 2018-05-02T00:01:18Z

+import { SourceFileConfiguration, CustomConfigurationProvider } from '../api';
+import { provideCustomConfiguration } from './extension';
+
+function runThenableWithTimeout(promise: Thenable<any>, ms: number): Thenable<any> {


runThenableWithTimeout [](start = 9, length = 22)

btw general comment: might be nice to have a utils.ts for the extension with common functions such as this being co-located.

Not blocking for this PR

saqadri

sherryyshi · 2018-05-02T17:01:47Z

+    "onCommand:C_Cpp.ResumeParsing",
+    "onCommand:C_Cpp.ShowParsingCommands",
+    "onCommand:C_Cpp.TakeSurvey",
+    "onDebug"


I keep checking this file in by accident... :(

sherryyshi · 2018-05-02T17:04:44Z

+import { SourceFileConfiguration, CustomConfigurationProvider } from '../api';
+import { provideCustomConfiguration } from './extension';
+
+function runThenableWithTimeout(promise: Thenable<any>, ms: number): Thenable<any> {


This should be removed. There is another copy of this in extension.ts which is what is actually being used.

sherryyshi · 2018-05-02T17:05:44Z

+
+    // Returns a race between our timeout and the passed in promise
+    return client.runBlockingTask(Promise.race([thenable(), timeout]).then((result: any) => {
+        clearTimeout(timer);


This line is unnecessary since the timer is cleared in the timeout promise. Will remove in the next push.

bobbrow · 2018-05-02T20:43:59Z

 const ChangeCompileCommandsNotification: NotificationType<FileChangedParams, void> = new NotificationType<FileChangedParams, void>('cpptools/didChangeCompileCommands');
 const ChangeSelectedSettingNotification: NotificationType<FolderSelectedSettingParams, void> = new NotificationType<FolderSelectedSettingParams, void>('cpptools/didChangeSelectedSetting');
 const IntervalTimerNotification: NotificationType<void, void> = new NotificationType<void, void>('cpptools/onIntervalTimer');
+const CustomConfigurationNotification: NotificationType<SourceFileConfigurationItem[], void> = new NotificationType<SourceFileConfigurationItem[], void>('cpptools/sendCustomConfiguration');


Can you keep this as an interface instead of an array of items? It will make our life easier on the language server side. #Closed

bobbrow · 2018-05-02T20:55:11Z

+}
+
+export function onDidCustomConfigurationChange(customConfigurationProvider: CustomConfigurationProvider): void {
+    // TODO: How to handle multiple workspaces? Only works with the current workspace.


multiple workspaces are handled by ClientCollection. You probably just need to forward this message to ClientCollection and have it iterate on its clients. #Closed

bobbrow · 2018-05-02T21:01:26Z

+    }
+
+    let documents: vscode.Uri[] = [];
+    for (let i: number = 0; i < vscode.workspace.textDocuments.length; ++i) {


You're not going to want to do it this way. This doesn't have any notion of which workspace (read: Client) owns which files. Each Client tracks the files it owns, so you want to have each client request updates for its tracked documents and then send the updated configs to its language server. #Closed

bobbrow · 2018-05-02T21:03:26Z

+
+    // TODO: Can we rely on clients.ActiveClient to be the client for the current workspace?
+    runBlockingThenableWithTimeout(() => customConfigurationProvider.provideConfigurations(documents, tokenSource.token), 1000, clients.ActiveClient, tokenSource)
+    .then((configs: SourceFileConfigurationItem[]) => {


i think you want to .then the runBlockingThenableWithTimeout call rather than the provideConfigurations call so that the language server notification doesn't count toward the timeout. #Closed

Hmm... I think that's what it's doing now

In reply to: 185637994 [](ancestors = 185637994)

Ah! I got confused parsing the long line in my head. Can you reformat?

runBlockingThenableWithTimeout(() => { return customConfigurationProvider.provideConfigurations(documents, tokenSource.token); }, 1000, clients.ActiveClient, tokenSource) ``` #Closed

bobbrow · 2018-05-02T21:43:13Z

+        timer = setTimeout(() => {
+            clearTimeout(timer);
+            if (tokenSource) {
+                tokenSource.cancel();


I see you are marking the operation as cancelled, but I don't see any code that actually does anything about it. Are there future changes coming for this? #Closed

The token is passed to our extension, and then passed to our "configuration server", which will react to it. Is there something to be done here as well?

Not sure how I missed that. I see it now. #Closed

…ions logic to client.ts

ThadHouse · 2018-05-17T00:00:27Z

Looking through this, I've noticed that the system headers and system defines need to be included in the provided item. However, the code that grabbed those when passed in a compilerPath seems to be in the low level language service rather then accessable in typescript. Is there any way to get that added to the API? There are some uses for this where the build system doesn't inject the system included directly, but instead just depends on the compiler to have them.

bobbrow · 2018-05-17T03:23:27Z

@ThadHouse, that's a reasonable request.

bobbrow · 2018-05-17T03:52:51Z

+    /**
+     * This must also include the system include path (compiler defaults)
+     */
+    includePaths: string[];


this should be renamed includePath to match c_cpp_properties.json

…eedback

sherryyshi · 2018-05-17T20:39:26Z

+     */
+    forcedInclude?: string[];
+
+    /**


@ThadHouse @bobbrow does this address the request?

Does the low level language service cache values for the same compiler path?

This looks great, but I would worry about performance if it checked the compiler for every file, even with the same path.

@ThadHouse, yes the language service will cache values to avoid invoking the compiler repeatedly. I am working on this change right now.

One more question. The normal docs say skip the headers and the compilerPath is msvc mode is set. Is that still the case with this API?

Which docs? Clang for Windows also requires msvc mode, so you should specify the compilerPath in all cases where you want the extension to add system includes and defines for you. We don't specifically query cl.exe in msvc mode yet since the mechanism isn't readily available (we read the registry instead), but we will in the future.

https://github.com/Microsoft/vscode-cpptools/blob/master/Documentation/LanguageServer/FAQ.md

Specifically this one, where if you want to use MSVC it says don't provide a compilerPath, and only add a compilerPath if using clang for windows.

sherryyshi added 2 commits April 4, 2018 17:33

WIP

3dace8c

RegisterConfigurations working.

9937753

WardenGnaw reviewed Apr 10, 2018

View reviewed changes

sherryyshi added 4 commits April 10, 2018 10:41

PR comments

e405f52

Moving exported interfaces into interfaces.ts

4863b7d

Reverting unnecessary exports

d2338b7

Merge branch 'master' into user/shersh/prototype

c29be07

WardenGnaw requested review from bobbrow and sean-mcmanus April 10, 2018 20:34

Fixing Travis CI errors.

b9f7b50

bobbrow reviewed Apr 11, 2018

View reviewed changes

sherryyshi added 2 commits April 16, 2018 10:33

Switch to CustomConfigurationProvider implementation

1e157b5

Fixing typo

2b05150

sherryyshi commented Apr 16, 2018

View reviewed changes

bobbrow requested changes Apr 16, 2018

View reviewed changes

sherryyshi added 3 commits April 17, 2018 10:16

Adding handling for multiple config providers (excluding UI) and chan…

5b15979

…ging callback implementation for provideConfiguration

Reverting cpptools.json

68038a7

Cleaning up

648be36

bobbrow reviewed Apr 17, 2018

View reviewed changes

sherryyshi added 2 commits April 17, 2018 11:58

Implement sendCustomConfiguration

3ce76d7

Move CustomConfiguration and related logic out of client.ts

e08f6bf

bobbrow reviewed Apr 17, 2018

View reviewed changes

sherryyshi added 2 commits April 17, 2018 16:10

Fix typo

bb89652

Implement canProvideConfiguration interface; Run provideConfiguration…

7f95710

… with a timeout

sherryyshi force-pushed the user/shersh/prototype branch from 2a8775c to 440c304 Compare April 19, 2018 01:15

Adding didCustomConfigurationChange; replacing provideConfiguration w…

dbec7db

…ith batched version(provideConfigurations).

saqadri reviewed May 1, 2018

View reviewed changes

saqadri reviewed May 2, 2018

View reviewed changes

saqadri approved these changes May 2, 2018

View reviewed changes

sherryyshi commented May 2, 2018

View reviewed changes

sherryyshi added 4 commits May 2, 2018 10:19

Remove duplicate function

018c9e8

Adding cancellation token

3db75d7

Resetting package.json and cpptools.json

fa00007

Adding documentation to api.ts

7894aea

bobbrow requested changes May 2, 2018

View reviewed changes

sherryyshi added 3 commits May 2, 2018 17:27

Moving runBlockingThenableWithTimeout and onDidChangeCustomConfigurat…

cbfd324

…ions logic to client.ts

Adding CustomConfigurationParams

4ceb6ba

Reformatting

c88ced7

bobbrow mentioned this pull request May 8, 2018

Single c_cpp_properties.json for an entire workspace #1229

Closed

sherryyshi added 3 commits May 11, 2018 16:21

Bug fix

5784c68

Add initial request for custom configurations

ebb1d7e

Don't send custom config request for an empty list of documents

3bb0e3c

Merge branch 'master' into user/shersh/prototype

2be570d

bobbrow reviewed May 17, 2018

View reviewed changes

sherryyshi added 2 commits May 17, 2018 10:02

Adding compilerPath to SourceFileConfiguration; addressing other CR f…

7825cf2

…eedback

Bug fix

5968a12

sherryyshi commented May 17, 2018

View reviewed changes

minor API changes

2147408

bobbrow approved these changes May 18, 2018

View reviewed changes

fix tslint issue

79e078e

bobbrow merged commit 01efb1a into microsoft:master May 18, 2018

github-actions Bot locked and limited conversation to collaborators Oct 14, 2020


		reportMacCrashes();

		eventEmitter.emit(activationCompleteEventId);

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

sherryyshi commented Apr 9, 2018

Uh oh!

sherryyshi commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WardenGnaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherryyshi commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WardenGnaw commented Apr 10, 2018

Uh oh!

sean-mcmanus commented Apr 10, 2018 • edited by bobbrow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbrow Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherryyshi Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrow Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

sherryyshi commented Apr 9, 2018 •

edited

Loading

sherryyshi commented Apr 10, 2018 •

edited

Loading

sean-mcmanus commented Apr 10, 2018 •

edited by bobbrow

Loading

bobbrow Apr 11, 2018 •

edited

Loading

bobbrow commented Apr 11, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

sherryyshi Apr 17, 2018 •

edited

Loading

bobbrow Apr 17, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 16, 2018 •

edited

Loading

bobbrow Apr 17, 2018 •

edited

Loading

bobbrow Apr 17, 2018 •

edited

Loading

bobbrow May 2, 2018 •

edited

Loading

bobbrow May 2, 2018 •

edited

Loading

bobbrow May 2, 2018 •

edited

Loading

bobbrow May 2, 2018 •

edited

Loading

bobbrow May 3, 2018 •

edited

Loading