Skip to content

Commit

Permalink
Various continue on comments fixes (microsoft#195260)
Browse files Browse the repository at this point in the history
* Various continue on comments fixes
Part of microsoft#194288

* Fix continue-on reply being restored before root comment
Fix duplicate continue-on comments

* Include monaco.d.ts changes
  • Loading branch information
alexr00 authored and Alex0007 committed Oct 26, 2023
1 parent 446be0a commit b98a0a1
Show file tree
Hide file tree
Showing 10 changed files with 93 additions and 37 deletions.
1 change: 1 addition & 0 deletions src/vs/editor/common/languages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1792,6 +1792,7 @@ export interface PendingCommentThread {
range: IRange;
uri: URI;
owner: string;
isReply: boolean;
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/vs/monaco.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7628,6 +7628,7 @@ declare namespace monaco.languages {
range: IRange;
uri: Uri;
owner: string;
isReply: boolean;
}

export interface CodeLens {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/api/browser/mainThreadComments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,8 @@ export class MainThreadCommentController implements ICommentController {
return ret;
}

createCommentThreadTemplate(resource: UriComponents, range: IRange | undefined): void {
this._proxy.$createCommentThreadTemplate(this.handle, resource, range);
createCommentThreadTemplate(resource: UriComponents, range: IRange | undefined): Promise<void> {
return this._proxy.$createCommentThreadTemplate(this.handle, resource, range);
}

async updateCommentThreadTemplate(threadHandle: number, range: IRange) {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/api/common/extHost.protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2332,7 +2332,7 @@ export interface ExtHostProgressShape {
}

export interface ExtHostCommentsShape {
$createCommentThreadTemplate(commentControllerHandle: number, uriComponents: UriComponents, range: IRange | undefined): void;
$createCommentThreadTemplate(commentControllerHandle: number, uriComponents: UriComponents, range: IRange | undefined): Promise<void>;
$updateCommentThreadTemplate(commentControllerHandle: number, threadHandle: number, range: IRange): Promise<void>;
$deleteCommentThread(commentControllerHandle: number, commentThreadHandle: number): void;
$provideCommentingRanges(commentControllerHandle: number, uriComponents: UriComponents, token: CancellationToken): Promise<{ ranges: IRange[]; fileComments: boolean } | undefined>;
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/api/common/extHostComments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export function createExtHostComments(mainContext: IMainContext, commands: ExtHo
return commentController.value;
}

$createCommentThreadTemplate(commentControllerHandle: number, uriComponents: UriComponents, range: IRange | undefined): void {
async $createCommentThreadTemplate(commentControllerHandle: number, uriComponents: UriComponents, range: IRange | undefined): Promise<void> {
const commentController = this._commentControllers.get(commentControllerHandle);

if (!commentController) {
Expand Down
8 changes: 7 additions & 1 deletion src/vs/workbench/contrib/comments/browser/commentReply.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export class CommentReply<T extends IRange | ICellRange> extends Disposable {
// Only add the additional step of clicking a reply button to expand the textarea when there are existing comments
if (hasExistingComments) {
this.createReplyButton(this.commentEditor, this.form);
} else if (this._commentThread.comments && this._commentThread.comments.length === 0) {
} else if ((this._commentThread.comments && this._commentThread.comments.length === 0) || this._pendingComment) {
this.expandReplyArea();
}
this._error = dom.append(this.form, dom.$('.validation-error.hidden'));
Expand Down Expand Up @@ -152,6 +152,12 @@ export class CommentReply<T extends IRange | ICellRange> extends Disposable {
return undefined;
}

public setPendingComment(comment: string) {
this._pendingComment = comment;
this.expandReplyArea();
this.commentEditor.setValue(comment);
}

public layout(widthInPixel: number) {
this.commentEditor.layout({ height: this._editorHeight, width: widthInPixel - 54 /* margin 20px * 10 + scrollbar 14px*/ });
}
Expand Down
36 changes: 19 additions & 17 deletions src/vs/workbench/contrib/comments/browser/commentService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export interface ICommentController {
};
options?: CommentOptions;
contextValue?: string;
createCommentThreadTemplate(resource: UriComponents, range: IRange | undefined): void;
createCommentThreadTemplate(resource: UriComponents, range: IRange | undefined): Promise<void>;
updateCommentThreadTemplate(threadHandle: number, range: IRange): Promise<void>;
deleteCommentThreadMain(commentThreadId: string): void;
toggleReaction(uri: URI, thread: CommentThread, comment: Comment, reaction: CommentReaction, token: CancellationToken): Promise<void>;
Expand Down Expand Up @@ -90,7 +90,7 @@ export interface ICommentService {
registerCommentController(owner: string, commentControl: ICommentController): void;
unregisterCommentController(owner?: string): void;
getCommentController(owner: string): ICommentController | undefined;
createCommentThreadTemplate(owner: string, resource: URI, range: Range | undefined): void;
createCommentThreadTemplate(owner: string, resource: URI, range: Range | undefined): Promise<void>;
updateCommentThreadTemplate(owner: string, threadHandle: number, range: Range): Promise<void>;
getCommentMenus(owner: string): CommentMenus;
updateComments(ownerId: string, event: CommentThreadChangedEvent<IRange>): void;
Expand All @@ -105,7 +105,7 @@ export interface ICommentService {
setCurrentCommentThread(commentThread: CommentThread<IRange | ICellRange> | undefined): void;
enableCommenting(enable: boolean): void;
registerContinueOnCommentProvider(provider: IContinueOnCommentProvider): IDisposable;
removeContinueOnComment(pendingComment: { range: IRange; uri: URI; owner: string }): PendingCommentThread | undefined;
removeContinueOnComment(pendingComment: { range: IRange; uri: URI; owner: string; isReply?: boolean }): PendingCommentThread | undefined;
}

const CONTINUE_ON_COMMENTS = 'comments.continueOnComments';
Expand Down Expand Up @@ -174,7 +174,8 @@ export class CommentService extends Disposable implements ICommentService {
this._workspaceHasCommenting = CommentContextKeys.WorkspaceHasCommenting.bindTo(contextKeyService);
const storageListener = this._register(new DisposableStore());

storageListener.add(this.storageService.onDidChangeValue(StorageScope.WORKSPACE, CONTINUE_ON_COMMENTS, storageListener)((v) => {
const storageEvent = Event.debounce(this.storageService.onDidChangeValue(StorageScope.WORKSPACE, CONTINUE_ON_COMMENTS, storageListener), (last, event) => last?.external ? last : event, 500);
storageListener.add(storageEvent(v => {
if (!this.configurationService.getValue<ICommentsConfiguration | undefined>(COMMENTS_SECTION)?.experimentalContinueOn) {
return;
}
Expand All @@ -186,7 +187,7 @@ export class CommentService extends Disposable implements ICommentService {
return;
}
this.logService.debug(`Comments: URIs of continue on comments from storage ${commentsToRestore.map(thread => thread.uri.toString()).join(', ')}.`);
const changedOwners = this._addContinueOnComments(commentsToRestore);
const changedOwners = this._addContinueOnComments(commentsToRestore, this._continueOnComments);
for (const owner of changedOwners) {
const evt: ICommentThreadChangedEvent = {
owner,
Expand All @@ -202,11 +203,12 @@ export class CommentService extends Disposable implements ICommentService {
if (!this.configurationService.getValue<ICommentsConfiguration | undefined>(COMMENTS_SECTION)?.experimentalContinueOn) {
return;
}
const map: Map<string, PendingCommentThread[]> = new Map();
for (const provider of this._continueOnCommentProviders) {
const pendingComments = provider.provideContinueOnComments();
this._addContinueOnComments(pendingComments);
this._addContinueOnComments(pendingComments, map);
}
this._saveContinueOnComments();
this._saveContinueOnComments(map);
}));
}

Expand Down Expand Up @@ -298,14 +300,14 @@ export class CommentService extends Disposable implements ICommentService {
return this._commentControls.get(owner);
}

createCommentThreadTemplate(owner: string, resource: URI, range: Range | undefined): void {
async createCommentThreadTemplate(owner: string, resource: URI, range: Range | undefined): Promise<void> {
const commentController = this._commentControls.get(owner);

if (!commentController) {
return;
}

commentController.createCommentThreadTemplate(resource, range);
return commentController.createCommentThreadTemplate(resource, range);
}

async updateCommentThreadTemplate(owner: string, threadHandle: number, range: Range) {
Expand Down Expand Up @@ -415,34 +417,34 @@ export class CommentService extends Disposable implements ICommentService {
};
}

private _saveContinueOnComments() {
private _saveContinueOnComments(map: Map<string, PendingCommentThread[]>) {
const commentsToSave: PendingCommentThread[] = [];
for (const pendingComments of this._continueOnComments.values()) {
for (const pendingComments of map.values()) {
commentsToSave.push(...pendingComments);
}
this.logService.debug(`Comments: URIs of continue on comments to add to storage ${commentsToSave.map(thread => thread.uri.toString()).join(', ')}.`);
this.storageService.store(CONTINUE_ON_COMMENTS, commentsToSave, StorageScope.WORKSPACE, StorageTarget.USER);
}

removeContinueOnComment(pendingComment: { range: IRange; uri: URI; owner: string }): PendingCommentThread | undefined {
removeContinueOnComment(pendingComment: { range: IRange; uri: URI; owner: string; isReply?: boolean }): PendingCommentThread | undefined {
const pendingComments = this._continueOnComments.get(pendingComment.owner);
if (pendingComments) {
const commentIndex = pendingComments.findIndex(comment => comment.uri.toString() === pendingComment.uri.toString() && Range.equalsRange(comment.range, pendingComment.range));
const commentIndex = pendingComments.findIndex(comment => comment.uri.toString() === pendingComment.uri.toString() && Range.equalsRange(comment.range, pendingComment.range) && (pendingComment.isReply === undefined || comment.isReply === pendingComment.isReply));
if (commentIndex > -1) {
return pendingComments.splice(commentIndex, 1)[0];
}
}
return undefined;
}

private _addContinueOnComments(pendingComments: PendingCommentThread[]): Set<string> {
private _addContinueOnComments(pendingComments: PendingCommentThread[], map: Map<string, PendingCommentThread[]>): Set<string> {
const changedOwners = new Set<string>();
for (const pendingComment of pendingComments) {
if (!this._continueOnComments.has(pendingComment.owner)) {
this._continueOnComments.set(pendingComment.owner, [pendingComment]);
if (!map.has(pendingComment.owner)) {
map.set(pendingComment.owner, [pendingComment]);
changedOwners.add(pendingComment.owner);
} else {
const commentsForOwner = this._continueOnComments.get(pendingComment.owner)!;
const commentsForOwner = map.get(pendingComment.owner)!;
if (commentsForOwner.every(comment => (comment.uri.toString() !== pendingComment.uri.toString()) || !Range.equalsRange(comment.range, pendingComment.range))) {
commentsForOwner.push(pendingComment);
changedOwners.add(pendingComment.owner);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,11 @@ export class CommentThreadWidget<T extends IRange | ICellRange = IRange> extends
return undefined;
}

setPendingComment(comment: string) {
this._pendingComment = comment;
this._commentReply?.setPendingComment(comment);
}

getDimensions() {
return this._body.getDimensions();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,9 @@ export class ReviewZoneWidget extends ZoneWidget implements ICommentThreadWidget
this._commentOptions = controller.options;
}

this._initialCollapsibleState = _commentThread.initialCollapsibleState;
this._isExpanded = _commentThread.initialCollapsibleState === languages.CommentThreadCollapsibleState.Expanded;
this._initialCollapsibleState = _pendingComment ? languages.CommentThreadCollapsibleState.Expanded : _commentThread.initialCollapsibleState;
_commentThread.initialCollapsibleState = this._initialCollapsibleState;
this._isExpanded = this._initialCollapsibleState === languages.CommentThreadCollapsibleState.Expanded;
this._commentThreadDisposables = [];
this.create();

Expand Down Expand Up @@ -215,6 +216,12 @@ export class ReviewZoneWidget extends ZoneWidget implements ICommentThreadWidget
};
}

public setPendingComment(comment: string) {
this._pendingComment = comment;
this.expand();
this._commentThreadWidget.setPendingComment(comment);
}

protected _fillContainer(container: HTMLElement): void {
this.setCssClass('review-widget');
this._commentThreadWidget = this._scopedInstantiationService.createInstance(
Expand Down
60 changes: 47 additions & 13 deletions src/vs/workbench/contrib/comments/browser/commentsController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import { AccessibilityVerbositySettingId } from 'vs/workbench/contrib/accessibil
import { AccessibilityCommandId } from 'vs/workbench/contrib/accessibility/common/accessibilityCommands';
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { IAccessibilityService } from 'vs/platform/accessibility/common/accessibility';
import { URI } from 'vs/base/common/uri';

export const ID = 'editor.contrib.review';

Expand Down Expand Up @@ -369,6 +370,7 @@ export class CommentController implements IEditorContribution {
private _computeCommentingRangeScheduler!: Delayer<Array<ICommentInfo | null>> | null;
private _pendingNewCommentCache: { [key: string]: { [key: string]: string } };
private _pendingEditsCache: { [key: string]: { [key: string]: { [key: number]: string } } }; // owner -> threadId -> uniqueIdInThread -> pending comment
private _inProcessContinueOnComments: Map<string, languages.PendingCommentThread[]> = new Map();
private _editorDisposables: IDisposable[] = [];
private _activeCursorHasCommentingRange: IContextKey<boolean>;
private _activeEditorHasCommentingRange: IContextKey<boolean>;
Expand Down Expand Up @@ -481,7 +483,8 @@ export class CommentController implements IEditorContribution {
owner: zone.owner,
uri: zone.editor.getModel()!.uri,
range: zone.commentThread.range,
body: pendingNewComment
body: pendingNewComment,
isReply: (zone.commentThread.comments !== undefined) && (zone.commentThread.comments.length > 0)
});
}
}
Expand Down Expand Up @@ -826,7 +829,7 @@ export class CommentController implements IEditorContribution {
this.openCommentsView(thread);
}
});
added.forEach(thread => {
for (const thread of added) {
const matchedZones = this._commentWidgets.filter(zoneWidget => zoneWidget.owner === e.owner && zoneWidget.commentThread.threadId === thread.threadId);
if (matchedZones.length) {
return;
Expand All @@ -839,23 +842,48 @@ export class CommentController implements IEditorContribution {
return;
}

const continueOnCommentText = (thread.range ? this.commentService.removeContinueOnComment({ owner: e.owner, uri: editorURI, range: thread.range })?.body : undefined);
const continueOnCommentIndex = this._inProcessContinueOnComments.get(e.owner)?.findIndex(pending => Range.lift(pending.range).equalsRange(thread.range));
let continueOnCommentText: string | undefined;
if ((continueOnCommentIndex !== undefined) && continueOnCommentIndex >= 0) {
continueOnCommentText = this._inProcessContinueOnComments.get(e.owner)?.splice(continueOnCommentIndex, 1)[0].body;
}

const pendingCommentText = (this._pendingNewCommentCache[e.owner] && this._pendingNewCommentCache[e.owner][thread.threadId!])
?? continueOnCommentText;
const pendingEdits = this._pendingEditsCache[e.owner] && this._pendingEditsCache[e.owner][thread.threadId!];
this.displayCommentThread(e.owner, thread, pendingCommentText, pendingEdits);
this._commentInfos.filter(info => info.owner === e.owner)[0].threads.push(thread);
this.tryUpdateReservedSpace();
});
pending.forEach(thread => {
this.commentService.createCommentThreadTemplate(thread.owner, thread.uri, Range.lift(thread.range));
});
}

for (const thread of pending) {
await this.resumePendingComment(editorURI, thread);
}
this._commentThreadRangeDecorator.update(this.editor, commentInfo);
}));

this.beginComputeAndHandleEditorChange();
}

private async resumePendingComment(editorURI: URI, thread: languages.PendingCommentThread) {
const matchedZones = this._commentWidgets.filter(zoneWidget => zoneWidget.owner === thread.owner && Range.lift(zoneWidget.commentThread.range)?.equalsRange(thread.range));
if (thread.isReply && matchedZones.length) {
this.commentService.removeContinueOnComment({ owner: thread.owner, uri: editorURI, range: thread.range, isReply: true });
const matchedZone = matchedZones[0];
matchedZone.setPendingComment(thread.body);
} else if (!thread.isReply) {
const threadStillAvailable = this.commentService.removeContinueOnComment({ owner: thread.owner, uri: editorURI, range: thread.range, isReply: false });
if (!threadStillAvailable) {
return;
}
if (!this._inProcessContinueOnComments.has(thread.owner)) {
this._inProcessContinueOnComments.set(thread.owner, []);
}
this._inProcessContinueOnComments.get(thread.owner)?.push(thread);
await this.commentService.createCommentThreadTemplate(thread.owner, thread.uri, Range.lift(thread.range));
}
}

private beginComputeAndHandleEditorChange(): void {
this.beginCompute().then(() => {
if (!this._hasRespondedToEditorChange) {
Expand Down Expand Up @@ -893,13 +921,19 @@ export class CommentController implements IEditorContribution {
}

private displayCommentThread(owner: string, thread: languages.CommentThread, pendingComment: string | undefined, pendingEdits: { [key: number]: string } | undefined): void {
if (!this.editor?.getModel()) {
const editor = this.editor?.getModel();
if (!editor) {
return;
}
if (this.isEditorInlineOriginal(this.editor)) {
if (!this.editor || this.isEditorInlineOriginal(this.editor)) {
return;
}
const zoneWidget = this.instantiationService.createInstance(ReviewZoneWidget, this.editor, owner, thread, pendingComment, pendingEdits);

let continueOnCommentReply: languages.PendingCommentThread | undefined;
if (thread.range && !pendingComment) {
continueOnCommentReply = this.commentService.removeContinueOnComment({ owner, uri: editor.uri, range: thread.range, isReply: true });
}
const zoneWidget = this.instantiationService.createInstance(ReviewZoneWidget, this.editor, owner, thread, pendingComment ?? continueOnCommentReply?.body, pendingEdits);
zoneWidget.display(thread.range);
this._commentWidgets.push(zoneWidget);
this.openCommentsView(thread);
Expand Down Expand Up @@ -1162,9 +1196,9 @@ export class CommentController implements IEditorContribution {

this.displayCommentThread(info.owner, thread, pendingComment, pendingEdits);
});
info.pendingCommentThreads?.forEach(thread => {
this.commentService.createCommentThreadTemplate(thread.owner, thread.uri, Range.lift(thread.range));
});
for (const thread of info.pendingCommentThreads ?? []) {
this.resumePendingComment(this.editor!.getModel()!.uri, thread);
}
});

this._commentingRangeDecorator.update(this.editor, this._commentInfos);
Expand Down

0 comments on commit b98a0a1

Please sign in to comment.