Add support for palette/context menu commands to live share by rchiodo · Pull Request #4574 · microsoft/vscode-python

rchiodo · 2019-02-27T18:53:31Z

For #4520, #4197

Revamp how commands are passed around. Now they are mirror'd at the history level instead of the command level as something like 'Run Current Cell' is context sensitive (so you can't just send it to the host).

Also fixed colors and the issue with the interactive window not coming up (because of file-matcher hanging). Did this here because it was making it a pain to debug this.

Still more work to do:

Write functional test for liveshare
Try on actual remote boxes
Write systemtest for the colors

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Has a news entry file (remember to thank yourself!)
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated
Test plan is updated as appropriate
package-lock.json has been regenerated by running npm install (if dependencies have changed)

codecov · 2019-02-27T19:07:41Z

Codecov Report

Merging #4574 into master will increase coverage by 20%.
The diff coverage is 49%.

@@           Coverage Diff            @@
##           master   #4574     +/-   ##
========================================
+ Coverage      58%     78%    +20%     
========================================
  Files         367     445     +78     
  Lines       15790   21080   +5290     
  Branches     2462    3424    +962     
========================================
+ Hits         9130   16272   +7142     
+ Misses       6047    4802   -1245     
+ Partials      613       6    -607

Flag	Coverage Δ
#Linux	`65% <38%> (?)`
#Windows	`65% <40%> (?)`
#macOS	`65% <38%> (?)`

rchiodo · 2019-02-27T19:08:16Z

        }
    }

-    private async waitForObservable(subscriber: Subscriber<ICell[]>, code: string, file: string, line: number, id: string) : Promise<void> {


All of this code moved into the responseQueue class. #ByDesign

rchiodo · 2019-02-27T19:09:11Z

-            if (!uri && result) {
-                const connectionInfo = result.getConnectionInfo();
-                if (connectionInfo) {
-                    const portMatch = RegExpValues.ExtractPortRegex.exec(connectionInfo.baseUrl);


portMatch [](start = 26, length = 9)

Port forwarding is now done by the server object instead. This allows the port forwarding to be removed when the server is disposed (which happens when doing an export to a file) #ByDesign

rchiodo · 2019-02-27T19:10:00Z

-        }
-
-        return result;
+    public getServer(options?: INotebookServerOptions) : Promise<INotebookServer | undefined> {


getServer [](start = 11, length = 9)

This API is now how the 'cached' server is requested. Variables uses this now. #ByDesign

rchiodo · 2019-02-27T19:12:10Z

    constructor(
        @inject(IWorkspaceService) private workspaceService: IWorkspaceService,
-        @inject(ICurrentProcess) private currentProcess: ICurrentProcess,
+        @inject(IThemeFinder) private themeFinder: IThemeFinder,


This new class finds the theme json files #ByDesign

rchiodo · 2019-02-27T19:13:12Z

    ConnectRemoteJupyter = 'DATASCIENCE.CONNECTREMOTEJUPYTER',
-    ConnectFailedJupyter = 'DATASCIENCE.CONNECTFAILEDJUPYTER'
+    ConnectFailedJupyter = 'DATASCIENCE.CONNECTFAILEDJUPYTER',
+    RemoteAddCode = 'DATASCIENCE.LIVESHARE.ADDCODE'


LIVESHARE [](start = 33, length = 9)

This is the single telemetry point we'll get for liveshare situations. It happens every time somebody tries to run a cell #Resolved

Oh actually I didn't enable it. Whoops.

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

rchiodo · 2019-02-27T19:18:43Z

    export const ArgsSplitterRegEx = /([^\s,]+)/g;
 }

-export namespace HistoryMessages {


I moved all of these into historyTypes #ByDesign

rchiodo · 2019-02-27T19:19:37Z

    }

    private registerCommands(): void {
-        let disposable = this.commandBroker.registerCommand(Commands.RunAllCells, this.runAllCells, this);


commandBroker [](start = 30, length = 13)

I got rid of the commandBroker as mirroring commands doesn't really make sense. #ByDesign

This also means the 'id' is not propagated from the commands anymore. It is propagated through the 'remoteAddCode' callback. That's how both sides end up with the same 'id' in the request they make.

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

rchiodo · 2019-02-27T19:22:13Z


+    // tslint:disable-next-line:no-any
+    private dispatchMessage<M extends IHistoryMapping, T extends keyof M>(message: T, payload: any, handler: (args : M[T]) => void) {
+        const args = payload as M[T];


This function enforces type safety on messages passed around between history, the interactive window, and other history's in a live share scenario #ByDesign

rchiodo · 2019-02-27T19:23:04Z

+        }
+    }
+
+    private showInformationMessage(message: string, question?: string) : Thenable<string | undefined> {


This function exists merely to satisfy hygiene. It didn't like passing an undefined string to the applicationShell.showInformationMessage #ByDesign

rchiodo · 2019-02-27T19:28:03Z

    max-height: 100%;
 }

+.cell-button-image svg{


Forgot about this fix. This fixes the tooltip on our buttons that had a title specified. #ByDesign

IanMatthewHuff · 2019-02-27T21:15:41Z


-    public async runSelectionOrLine(id: string): Promise<void> {
+    // tslint:disable-next-line:no-any
+    public async runSelectionOrLine(... args: any[]): Promise<void> {


... args: any[] [](start = 36, length = 15)

Remove if not needed. #Resolved

Will do

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

IanMatthewHuff · 2019-02-27T21:44:50Z

+        let extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions');
+        if (!(await fs.pathExists(extensionsPath))) {
+            // Might be on mac or linux. try a different path
+            currentPath = path.resolve(currentPath, '../../../..');


currentPath = path.resolve(currentPath, '../../../..'); [](start = 12, length = 55)

This looks odd. The default themes on mac / linux are saved out of the exe directory? Is this relying on users doing a default install of some type to move back the right number of levels? #ByDesign

This is what I found on a mac/linux machine when I originally wrote this code (this algorithm was what we used before). Right now it's actually a backup as the comment above is technically wrong. On my machine all of the colors were found through the extensions api.

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

It's relying on the fact that the exe for vscode is 4 levels above its root install location.

In reply to: 260958949 [](ancestors = 260958949,260958142)

I think I'll just update the comment.

In reply to: 260959732 [](ancestors = 260959732,260958949,260958142)

IanMatthewHuff · 2019-02-27T21:49:46Z

+                const fpath = path.join(rootPath, f);
+                const data = await this.findMatchingThemeFromJson(fpath, themeName);
+                if (data) {
+                    foundData.push(data);


If this is slow, this might be better as findMatchingTheme and just return back the first hit. Looks like you just take the first hit in all cases anyways. If it's not slow then probably fine to leave as is. #Resolved

This code never runs right now, so it's not slow :)

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

But you're right. I should return a single one.

In reply to: 260960097 [](ancestors = 260960097,260959779)

IanMatthewHuff · 2019-02-27T21:55:23Z

        }
    }

-    private async waitForObservable(subscriber: Subscriber<ICell[]>, code: string, file: string, line: number, id: string) : Promise<void> {


ync waitForObservable(subscribe [](start = 14, length = 31)

Thanks, this makes it much cleaner here with the new class. At least from my perspective. #Resolved

IanMatthewHuff

rchiodo added 11 commits February 25, 2019 09:27

Change liveshare to mirror addcode and not commands

e7558dd

Fixup after rebasing

05407bc

Merge from master

36fcef2

Fix colors to work without using file-matcher

e03fb9f

More color fixes

5852af7

Working on syncing servers when multiple

678c76c

Export working on guest with shutdown

ed88072

Move port forwarding into the server so can dispose on server shutdown

1383bb1

Fix hygiene errors

d5bc551

Fix history functional tests

8b0dc2f

Add news entry

0f84820

rchiodo self-assigned this Feb 27, 2019

rchiodo requested review from DonJayamanne and IanMatthewHuff February 27, 2019 18:53

Fix jupyter variables unit test

f66bc37

rchiodo commented Feb 27, 2019

View reviewed changes

Add live share telemetry

97f896f

rchiodo commented Feb 27, 2019

View reviewed changes

IanMatthewHuff reviewed Feb 27, 2019

View reviewed changes

Review comment

b2f5c51

IanMatthewHuff reviewed Feb 27, 2019

View reviewed changes

Make theme search faster and update comment

f401112

IanMatthewHuff reviewed Feb 27, 2019

View reviewed changes

IanMatthewHuff approved these changes Feb 27, 2019

View reviewed changes

rchiodo merged commit de62536 into master Feb 27, 2019

rchiodo deleted the rchiodo/live_share_3 branch March 8, 2019 00:30

lock Bot locked as resolved and limited conversation to collaborators Jul 30, 2019

Conversation

rchiodo commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rchiodo Feb 27, 2019 • 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!

rchiodo Feb 27, 2019 • 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!

rchiodo Feb 27, 2019 • 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!

rchiodo Feb 27, 2019 • 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!

rchiodo Feb 27, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

rchiodo Feb 27, 2019 • 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!

rchiodo Feb 27, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

rchiodo Feb 27, 2019 • 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!

rchiodo Feb 27, 2019 • 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!

rchiodo Feb 27, 2019 • 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!

IanMatthewHuff Feb 27, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Feb 27, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Feb 27, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

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!

IanMatthewHuff Feb 27, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Feb 27, 2019 •

edited

Loading

codecov Bot commented Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

rchiodo Feb 27, 2019 •

edited

Loading

IanMatthewHuff Feb 27, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Feb 27, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Feb 27, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Feb 27, 2019 •

edited by rchiodo

Loading