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.

Sign up for GitHub

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

Jump to bottom

Various continue on comments fixes #195260

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Various continue on comments fixes #195260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44b653a
Various continue on comments fixes
alexr00 Oct 10, 2023
d49bc02
Fix continue-on reply being restored before root comment
alexr00 Oct 10, 2023
4a2fc4e
Include monaco.d.ts changes
alexr00 Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/vs/editor/common/languages.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,8 +451,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2326,7 +2326,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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts
View file
Open in desktop
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
11 changes: 9 additions & 2 deletions src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentionally not awaited?

Copy link
Member Author

@alexr00 alexr00 Oct 11, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this call, it doesn't matter either way and the enclosing method is not already async.

}
});

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