Support mentionable users into liveshare 'suggested' contacts#1694
Support mentionable users into liveshare 'suggested' contacts#1694RMacfarlane merged 4 commits intomicrosoft:masterfrom
Conversation
…ers into liveshare pane
| /** | ||
| * A contact service provider for liveshare that would suggest contacts based on the pull request manager | ||
| */ | ||
| export class PullRequestProvider implements ContactServiceProvider { |
There was a problem hiding this comment.
Let's rename this class to GitHubContactServiceProvider and also rename the file, I think that better reflects what this is.
| @@ -0,0 +1,121 @@ | |||
| import * as vscode from 'vscode'; | |||
There was a problem hiding this comment.
Can you run the Format Document command on this file, as well as Convert Indentation to Tabs?
| constructor(private readonly pullRequestManager: PullRequestManager) { | ||
| pullRequestManager.onDidChangeMentionableUsers(e => { | ||
| this.notifySuggestedAccounts(e); | ||
| }) |
There was a problem hiding this comment.
tslint warning here, missing semicolon
|
|
||
| public async requestAsync( | ||
| type: string, | ||
| parameters: Object, |
There was a problem hiding this comment.
parameters and cancellationToken are unused, either remove them or write them as _
| supportsContactPresenceRequest: false, | ||
| supportsPublishPresence: false | ||
| } | ||
| } |
| // if we get initialized and | ||
| const allMentionableUsers = this.pullRequestManager.getAllMentionableUsers(); | ||
| if (allMentionableUsers) { | ||
| setTimeout(() => this.notifySuggestedAccounts(allMentionableUsers), 10); |
There was a problem hiding this comment.
why is this timeout needed?
There was a problem hiding this comment.
I did have some concerns that we are sending a notify without the 'initialize' request to be even returned. But i check again liveshare and it is prepared to receive notifications during the initialize so i remove the timeout
| } | ||
| } | ||
|
|
||
| // if we get initialized and |
|
|
||
| interface NotifyContactServiceEventArgs { | ||
| type: string; | ||
| body?: any | undefined; |
There was a problem hiding this comment.
can body truly be any type? should it be Contact?
There was a problem hiding this comment.
Yes, the interface is intented to notify multiple type of bodies controlled by the 'type' property. I did not want to define the whole list of notifications available. This definitions should have come from oru public npm package (that is auto generated). The problem is that the package was triggering all sort of errors and warnings so i decided to handcraft what we really need
| public onNotified: vscode.Event<NotifyContactServiceEventArgs> = this.onNotifiedEmitter.event; | ||
|
|
||
| constructor(private readonly pullRequestManager: PullRequestManager) { | ||
| pullRequestManager.onDidChangeMentionableUsers(e => { |
There was a problem hiding this comment.
I think it would be better to use assignableUsers instead of mentionableUsers for this.
assignableUsers: A list of users that can be assigned to issues in this repository. Essentially all the users that you are able to assign the issue to, meaning all your collaborators.
mentionableUsers: A list of Users that can be mentioned in the context of the repository. All the users that might not be able to collaborate however you can mention or @ on the issue.
Perhaps VSCode is unique in this problem, but our mentionable users list is huge. Assignable users is much more focused.
There was a problem hiding this comment.
I check with Jonathan Carter and we both agree to follow your suggestion, thanks for the advice
src/extension.ts
Outdated
| context.subscriptions.push(registerLiveShareGitProvider(apiImpl)); | ||
| const liveshareGitProvider = registerLiveShareGitProvider(apiImpl); | ||
| context.subscriptions.push(liveshareGitProvider); | ||
| const liveshareApi = liveshareGitProvider.initialize(); |
There was a problem hiding this comment.
the await (actually a .then()) is done later on the init method. I will rename the name to 'liveshareApiPromise'
RMacfarlane
left a comment
There was a problem hiding this comment.
This looks good! I only have extremely minor comments, I'll merge this once those are fixed.
| }; | ||
|
|
||
| // if we get initialized and users are available on the pr manager | ||
| const allMentionableUsers = this.pullRequestManager.getAllAssignableUsers(); |
There was a problem hiding this comment.
nit: allAssignableUsers
src/github/pullRequestManager.ts
Outdated
| }); | ||
| } | ||
|
|
||
| getAllMentionableUsers(): IAccount[] | undefined { |
There was a problem hiding this comment.
would you mind removing this and the corresponding events since it is unused?
| const prManager = new PullRequestManager(repository, telemetry, git, credentialStore); | ||
| context.subscriptions.push(prManager); | ||
|
|
||
| liveshareApiPromise.then((api) => { |
There was a problem hiding this comment.
prefer await syntax over .then
There was a problem hiding this comment.
i intentionally changed the 'await' for the 'then', the reason behind is that awaiting the liveshare api means that the liveshare api return would have to wait for the liveshare extension to fully activate. Waiting for the activation would block (awaited fo course) the pr github extension without any good benefit. If you really think that 'await' would be enough i can change the code.
There was a problem hiding this comment.
Got it, yeah let's not block
|
Thank you, everything looks good! I've merged it in |
Liveshare as a pane to display 'suggested' contacts to allow them an easy click to invite option. The model build around that allow external providers to extend the mechanism on how to suggest contacts and aggregate on multiple providers.
This PR will have the vscode-pr extension to regsiter a provider where it can monitor the calculated mentionable users and push them into liveshare which then will enable presence for them and allow them invite.
The changes involve using the liveshare API to register the new provider and also a way to hookup when mentionable users are finished, then it will convert back into liveshare 'contact' models.
Please note that the IAccount was modified to add the 'email' optional property in order to have liveshare to be able to match the correct liveshare 'live' user (with presence support)