Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comments Provider API #53598

Closed
RMacfarlane opened this issue Jul 5, 2018 · 16 comments
Closed

Comments Provider API #53598

RMacfarlane opened this issue Jul 5, 2018 · 16 comments
Assignees
Labels
api api-proposal feature-request Request for new features or functionality

Comments

@RMacfarlane
Copy link
Contributor

Introduces the concept of comment threads and comments, and adds two providers an extension can register. One for on-demand comments when an editor is opened, rendered in the editor, and one to display all comments in the workspace and allow navigation.

interface CommentInfo {
		threads: CommentThread[];
		commentingRanges?: Range[];
	}

	export enum CommentThreadCollapsibleState {
		/**
		 * Determines an item is collapsed
		 */
		Collapsed = 0,
		/**
		 * Determines an item is expanded
		 */
		Expanded = 1
	}

	interface CommentThread {
		threadId: string;
		resource: Uri;
		range: Range;
		comments: Comment[];
		collapsibleState?: CommentThreadCollapsibleState;
	}

	interface Comment {
		commentId: string;
		body: MarkdownString;
		userName: string;
		gravatar: string;
		command?: Command;
	}

	export interface CommentThreadChangedEvent {
		/**
		 * Added comment threads.
		 */
		readonly added: CommentThread[];

		/**
		 * Removed comment threads.
		 */
		readonly removed: CommentThread[];

		/**
		 * Changed comment threads.
		 */
		readonly changed: CommentThread[];
	}

	interface DocumentCommentProvider {
		provideDocumentComments(document: TextDocument, token: CancellationToken): Promise<CommentInfo>;
		createNewCommentThread?(document: TextDocument, range: Range, text: string, token: CancellationToken): Promise<CommentThread>;
		replyToCommentThread?(document: TextDocument, range: Range, commentThread: CommentThread, text: string, token: CancellationToken): Promise<CommentThread>;
		onDidChangeCommentThreads?: Event<CommentThreadChangedEvent>;
	}

	interface WorkspaceCommentProvider {
		provideWorkspaceComments(token: CancellationToken): Promise<CommentThread[]>;
		createNewCommentThread?(document: TextDocument, range: Range, text: string, token: CancellationToken): Promise<CommentThread>;
		replyToCommentThread?(document: TextDocument, range: Range, commentThread: CommentThread, text: string, token: CancellationToken): Promise<CommentThread>;

		onDidChangeCommentThreads?: Event<CommentThreadChangedEvent>;
	}

@mgood
Copy link

mgood commented Sep 6, 2018

This is exciting. I had started prototyping a code review extension using "info" diagnostic messages to display comments, but this will be much better.

A couple things I'd find useful:

  • "draft" state (or similar) for replies to support comments that have been saved, but not yet published
  • marking threads as "done" or "completed"

We use Phabricator which supports these features, but Github code reviews has something very similar so that could be a good candidate to look at for features.

Or maybe instead of adding specific support this could be implemented by including a way to include annotations for the thread "status", as well as a menu of actions that can be performed on the thread.

On a related note, I was initially going to use the Language Server Protocol to provide the Diagnostics for my extension. Once this has stabilized I would also find it interesting if the Language Server was extended to provide a commenting API like this as well to standardize support across other editors.

@mgood
Copy link

mgood commented Sep 6, 2018

Another wishlist item would be for the "Reply" to use a rich editor instead of the basic text field shown in the screenshot from the release notes. To support that the comment provider API should have a way to indicate the syntax language for the reply. This would enable some nicer support for the markup language supported by the comment extension. In addition to basics like general Markdown syntax I'd like to have the ability to provide other language features like completions for things like @username.

@RMacfarlane
Copy link
Contributor Author

@mgood These are great suggestions, and things I'd also like to see in the API.

There's definitely a need for having some kind of "status" associated with a thread, VSTS also has a concept of marking threads as "resolved", "won't fix", etc, and GitHub has also recently introduced a way of resolving conversations.

As you said, "draft" state is also common between various code review tools. Username completions are, too. There's also a similar concept of either liking or reacting to other's comments.

The next step I'm going to start work on is providing a way to edit and delete comments: #58078

@nicksnyder
Copy link
Contributor

nicksnyder commented Sep 15, 2018

I am experimenting with the proposed API and am confused how to get createNewCommentThread to get called (I dug through the code and didn't see a way).

Ideally, I would like my extension to be able to expose a command that opens the UI to create a new comment in a code file.

Got it. Needed to return commentingRanges!

@nicksnyder
Copy link
Contributor

nicksnyder commented Sep 15, 2018

Why does the Comment interface have a field called gravatar when it is just an image url?

@RMacfarlane
Copy link
Contributor Author

@nicksnyder it should be renamed to something like userIcon

@RMacfarlane RMacfarlane added the feature-request Request for new features or functionality label Sep 18, 2018
@pprice
Copy link
Contributor

pprice commented Sep 22, 2018

I've started looking into the APIs for use with phabricator.

I'd like to echo the need for:

  • Comment states (done / not done); given the breadth of code-review workflows, maybe this could be an extension specified "list" to pick from? In phabriactor this is actually boolean state, but a list would be more flexible.
  • Draft comments - phabricator's workflow is to publish comments as a single action. This could also be done with a state api, but a state that is not under controlled.
  • Comment formats - Support different comment input formats, API expects markdown today, phabricator uses remarkup.

Some nice to haves:

  • Comment completions - Ability to provide customization to the comment input "completions" (user mentions, macros, phabricator style object links)
  • Comment previews - Ability to render comment markup inline when editing (just like on github)

That said, I think I can make a fair bit of headway with what is here today.

@rebornix
Copy link
Member

@pprice thanks Phil, your suggestions are really helpful and they are on our radar.

Comment completion is probably an easy one as we use a specific scheme for the comment editor inside the editor, so an extension can register a suggestion provider for it. We didn't finalize the scheme yet but that's the way we'll go.

Speaking of comment format, it's a bit complex but we can provide similar APIs as above, a specific preview provider or something similar, it can help with both the final rendering and comment preview.

We need to think more about how to introduce the comment state concept into the API, like you said it's like an enum but may vary in different comment systems. Drafts of comments (or pending reviews in some systems) may involve some UI change and @RMacfarlane has some insights into it already #53598 (comment) .

@otakustay
Copy link

I'm wondering how we can utilize the onDidChangeCommentThreads event to modify comment threads on a certain document, the CommentThreadChangedEvent interface does not contain a document property, the only added, changed and removed properties are insufficient to tell which document should respond to changes.

@rebornix
Copy link
Member

rebornix commented Sep 23, 2018

@otakustay every CommentThread has a resource which links to a certain document, so to update the comment threads in a single document, we only need to trigger CommentThreadChangedEvent for threads with that resource uri.

@nicksnyder
Copy link
Contributor

nicksnyder commented Oct 4, 2018

I know of at least two extensions that are published that use the new API. What is required to be able to use the new API in the marketplace?

I noticed that both of these have the "preview" flag enabled. Is that it? What does the "preview" flag imply?

edit: I tried uploading a test extension with "preview": true, "enabledProposedApi": true and it didn't seem to work the same as when developing locally (i.e. provideDocumentComments is never called).

@eamodio
Copy link
Contributor

eamodio commented Oct 4, 2018

@nicksnyder the preview flag just marks an extension as "preview" in the marketplace (it gets a little preview badge). Other than being whitelisted by vscode and the marketplace there isn't a way to use the proposed apis.

Also CodeStream doesn't currently use the comments API.

@nicksnyder
Copy link
Contributor

Also CodeStream doesn't currently use the comments API.

Oops, not sure why I thought this.

whitelisted by vscode and the marketplace

To whoever owns this whitelist (I couldn't find one in this public repo), we would love to be able to help dogfood the new extension API with Sourcegraph Discussions inside of our team and provide feedback. Discussions would be hidden behind a feature flag (off by default) and we would not be inconvenienced at all if you made breaking changes to the API.

You can checkout the implementation here: sourcegraph/sourcegraph-vscode-DEPRECATED#19

@eamodio
Copy link
Contributor

eamodio commented Oct 4, 2018

I've tried to make similar arguments to be whitelisted, but it's just not going to happen. As far as I know they don't want to be tied to proposed apis for anything in the wild that they don't control. Which is understandable. I do wish that there was a better option for users to use a vsix with proposed apis vs running with command line flags for each extension

@rebornix
Copy link
Member

rebornix commented Oct 5, 2018

Thanks @eamodio for the detailed explaination of the proposed api ---> #60024

@nicksnyder to allow dogfooding inside your team, probably a bash alias can work seamlessly (for example I use ci for code-insiders so you can do the same thing for --enable-proposed-api).

@rebornix
Copy link
Member

rebornix commented Feb 7, 2019

Close this one in favor of #68020.

@rebornix rebornix closed this as completed Feb 7, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-proposal feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

7 participants