From ce7b68b48dbc8dc01ffa8f50008d511097b63441 Mon Sep 17 00:00:00 2001 From: Alex Ross Date: Fri, 4 Oct 2024 10:40:52 +0200 Subject: [PATCH] Typing in a comment, switching editors and switching back will not restore the cursor position in the comment box (#230466) Fixes #229214 --- src/vs/editor/common/languages.ts | 7 +++- src/vs/monaco.d.ts | 7 +++- .../contrib/comments/browser/commentNode.ts | 23 +++++++------ .../contrib/comments/browser/commentReply.ts | 23 +++++++------ .../comments/browser/commentThreadBody.ts | 6 ++-- .../comments/browser/commentThreadWidget.ts | 16 +++++----- .../browser/commentThreadZoneWidget.ts | 12 +++---- .../comments/browser/commentsController.ts | 32 +++++++++---------- 8 files changed, 72 insertions(+), 54 deletions(-) diff --git a/src/vs/editor/common/languages.ts b/src/vs/editor/common/languages.ts index 007c204ae98c..1691b1988e62 100644 --- a/src/vs/editor/common/languages.ts +++ b/src/vs/editor/common/languages.ts @@ -2001,11 +2001,16 @@ export interface Comment { } export interface PendingCommentThread { - body: string; range: IRange | undefined; uri: URI; uniqueOwner: string; isReply: boolean; + comment: PendingComment; +} + +export interface PendingComment { + body: string; + cursor: IPosition; } /** diff --git a/src/vs/monaco.d.ts b/src/vs/monaco.d.ts index c60964806d1c..91d82d16526c 100644 --- a/src/vs/monaco.d.ts +++ b/src/vs/monaco.d.ts @@ -7999,11 +7999,16 @@ declare namespace monaco.languages { } export interface PendingCommentThread { - body: string; range: IRange | undefined; uri: Uri; uniqueOwner: string; isReply: boolean; + comment: PendingComment; + } + + export interface PendingComment { + body: string; + cursor: IPosition; } export interface CodeLens { diff --git a/src/vs/workbench/contrib/comments/browser/commentNode.ts b/src/vs/workbench/contrib/comments/browser/commentNode.ts index 1f489cfa05b9..e014be03658a 100644 --- a/src/vs/workbench/contrib/comments/browser/commentNode.ts +++ b/src/vs/workbench/contrib/comments/browser/commentNode.ts @@ -14,7 +14,6 @@ import { MarkdownRenderer } from '../../../../editor/browser/widget/markdownRend import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js'; import { ICommentService } from './commentService.js'; import { LayoutableEditor, MIN_EDITOR_HEIGHT, SimpleCommentEditor, calculateEditorHeight } from './simpleCommentEditor.js'; -import { Selection } from '../../../../editor/common/core/selection.js'; import { Emitter, Event } from '../../../../base/common/event.js'; import { INotificationService } from '../../../../platform/notification/common/notification.js'; import { ToolBar } from '../../../../base/browser/ui/toolbar/toolbar.js'; @@ -50,6 +49,7 @@ import { IKeybindingService } from '../../../../platform/keybinding/common/keybi import { MarshalledCommentThread } from '../../../common/comments.js'; import { IHoverService } from '../../../../platform/hover/browser/hover.js'; import { IResolvedTextEditorModel, ITextModelService } from '../../../../editor/common/services/resolverService.js'; +import { Position } from '../../../../editor/common/core/position.js'; class CommentsActionRunner extends ActionRunner { protected override async runAction(action: IAction, context: any[]): Promise { @@ -103,7 +103,7 @@ export class CommentNode extends Disposable { private readonly parentEditor: LayoutableEditor, private commentThread: languages.CommentThread, public comment: languages.Comment, - private pendingEdit: string | undefined, + private pendingEdit: languages.PendingComment | undefined, private owner: string, private resource: URI, private parentThread: ICommentThreadWidget, @@ -503,7 +503,14 @@ export class CommentNode extends Disposable { this._commentEditorModel = modelRef; this._commentEditor.setModel(this._commentEditorModel.object.textEditorModel); - this._commentEditor.setValue(this.pendingEdit ?? this.commentBodyValue); + this._commentEditor.setValue(this.pendingEdit?.body ?? this.commentBodyValue); + if (this.pendingEdit) { + this._commentEditor.setPosition(this.pendingEdit.cursor); + } else { + const lastLine = this._commentEditorModel.object.textEditorModel.getLineCount(); + const lastColumn = this._commentEditorModel.object.textEditorModel.getLineLength(lastLine) + 1; + this._commentEditor.setPosition(new Position(lastLine, lastColumn)); + } this.pendingEdit = undefined; this._commentEditor.layout({ width: container.clientWidth - 14, height: this._editorHeight }); this._commentEditor.focus(); @@ -513,10 +520,6 @@ export class CommentNode extends Disposable { this._commentEditor!.focus(); }); - const lastLine = this._commentEditorModel.object.textEditorModel.getLineCount(); - const lastColumn = this._commentEditorModel.object.textEditorModel.getLineLength(lastLine) + 1; - this._commentEditor.setSelection(new Selection(lastLine, lastColumn, lastLine, lastColumn)); - const commentThread = this.commentThread; commentThread.input = { uri: this._commentEditor.getModel()!.uri, @@ -571,10 +574,10 @@ export class CommentNode extends Disposable { return false; } - getPendingEdit(): string | undefined { + getPendingEdit(): languages.PendingComment | undefined { const model = this._commentEditor?.getModel(); - if (model && model.getValueLength() > 0) { - return model.getValue(); + if (this._commentEditor && model && model.getValueLength() > 0) { + return { body: model.getValue(), cursor: this._commentEditor.getPosition()! }; } return undefined; } diff --git a/src/vs/workbench/contrib/comments/browser/commentReply.ts b/src/vs/workbench/contrib/comments/browser/commentReply.ts index 1b151755c752..981a3513d598 100644 --- a/src/vs/workbench/contrib/comments/browser/commentReply.ts +++ b/src/vs/workbench/contrib/comments/browser/commentReply.ts @@ -31,6 +31,7 @@ import { ICellRange } from '../../notebook/common/notebookRange.js'; import { LayoutableEditor, MIN_EDITOR_HEIGHT, SimpleCommentEditor, calculateEditorHeight } from './simpleCommentEditor.js'; import { IHoverService } from '../../../../platform/hover/browser/hover.js'; import { IContextMenuService } from '../../../../platform/contextview/browser/contextView.js'; +import { Position } from '../../../../editor/common/core/position.js'; let INMEM_MODEL_ID = 0; export const COMMENTEDITOR_DECORATION_KEY = 'commenteditordecoration'; @@ -57,7 +58,7 @@ export class CommentReply extends Disposable { private _contextKeyService: IContextKeyService, private _commentMenus: CommentMenus, private _commentOptions: languages.CommentOptions | undefined, - private _pendingComment: string | undefined, + private _pendingComment: languages.PendingComment | undefined, private _parentThread: ICommentThreadWidget, focus: boolean, private _actionRunDelegate: (() => void) | null, @@ -96,10 +97,13 @@ export class CommentReply extends Disposable { } const model = await this.textModelService.createModelReference(resource); - model.object.textEditorModel.setValue(this._pendingComment || ''); + model.object.textEditorModel.setValue(this._pendingComment?.body || ''); this._register(model); this.commentEditor.setModel(model.object.textEditorModel); + if (this._pendingComment) { + this.commentEditor.setPosition(this._pendingComment.cursor); + } this.calculateEditorHeight(); this._register(model.object.textEditorModel.onDidChangeContent(() => { @@ -157,20 +161,21 @@ export class CommentReply extends Disposable { } } - public getPendingComment(): string | undefined { + public getPendingComment(): languages.PendingComment | undefined { const model = this.commentEditor.getModel(); if (model && model.getValueLength() > 0) { // checking length is cheap - return model.getValue(); + return { body: model.getValue(), cursor: this.commentEditor.getPosition() ?? new Position(1, 1) }; } return undefined; } - public setPendingComment(comment: string) { - this._pendingComment = comment; + public setPendingComment(pending: languages.PendingComment) { + this._pendingComment = pending; this.expandReplyArea(); - this.commentEditor.setValue(comment); + this.commentEditor.setValue(pending.body); + this.commentEditor.setPosition(pending.cursor); } public layout(widthInPixel: number) { @@ -254,7 +259,7 @@ export class CommentReply extends Disposable { commentEditor.setValue(input.value); if (input.value === '') { - this._pendingComment = ''; + this._pendingComment = { body: '', cursor: new Position(1, 1) }; commentForm.classList.remove('expand'); commentEditor.getDomNode()!.style.outline = ''; this._error.textContent = ''; @@ -339,7 +344,7 @@ export class CommentReply extends Disposable { domNode.style.outline = ''; } this.commentEditor.setValue(''); - this._pendingComment = ''; + this._pendingComment = { body: '', cursor: new Position(1, 1) }; this.form.classList.remove('expand'); this._error.textContent = ''; this._error.classList.add('hidden'); diff --git a/src/vs/workbench/contrib/comments/browser/commentThreadBody.ts b/src/vs/workbench/contrib/comments/browser/commentThreadBody.ts index 2cf1ddeb6635..1ee553714bf2 100644 --- a/src/vs/workbench/contrib/comments/browser/commentThreadBody.ts +++ b/src/vs/workbench/contrib/comments/browser/commentThreadBody.ts @@ -49,7 +49,7 @@ export class CommentThreadBody extends D readonly container: HTMLElement, private _options: IMarkdownRendererOptions, private _commentThread: languages.CommentThread, - private _pendingEdits: { [key: number]: string } | undefined, + private _pendingEdits: { [key: number]: languages.PendingComment } | undefined, private _scopedInstatiationService: IInstantiationService, private _parentCommentThreadWidget: ICommentThreadWidget, @ICommentService private commentService: ICommentService, @@ -142,8 +142,8 @@ export class CommentThreadBody extends D }); } - getPendingEdits(): { [key: number]: string } { - const pendingEdits: { [key: number]: string } = {}; + getPendingEdits(): { [key: number]: languages.PendingComment } { + const pendingEdits: { [key: number]: languages.PendingComment } = {}; this._commentElements.forEach(element => { if (element.isEditing) { const pendingEdit = element.getPendingEdit(); diff --git a/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts b/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts index 5b6ed30b6498..ebad6e3f24bd 100644 --- a/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts +++ b/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts @@ -68,8 +68,8 @@ export class CommentThreadWidget extends private _contextKeyService: IContextKeyService, private _scopedInstantiationService: IInstantiationService, private _commentThread: languages.CommentThread, - private _pendingComment: string | undefined, - private _pendingEdits: { [key: number]: string } | undefined, + private _pendingComment: languages.PendingComment | undefined, + private _pendingEdits: { [key: number]: languages.PendingComment } | undefined, private _markdownOptions: IMarkdownRendererOptions, private _commentOptions: languages.CommentOptions | undefined, private _containerDelegate: { @@ -333,11 +333,11 @@ export class CommentThreadWidget extends return this._body.getCommentCoords(commentUniqueId); } - getPendingEdits(): { [key: number]: string } { + getPendingEdits(): { [key: number]: languages.PendingComment } { return this._body.getPendingEdits(); } - getPendingComment(): string | undefined { + getPendingComment(): languages.PendingComment | undefined { if (this._commentReply) { return this._commentReply.getPendingComment(); } @@ -345,9 +345,9 @@ export class CommentThreadWidget extends return undefined; } - setPendingComment(comment: string) { - this._pendingComment = comment; - this._commentReply?.setPendingComment(comment); + setPendingComment(pending: languages.PendingComment) { + this._pendingComment = pending; + this._commentReply?.setPendingComment(pending); } getDimensions() { @@ -378,7 +378,7 @@ export class CommentThreadWidget extends const activeComment = this._body.activeComment; if (activeComment) { return activeComment.submitComment(); - } else if ((this._commentReply?.getPendingComment()?.length ?? 0) > 0) { + } else if ((this._commentReply?.getPendingComment()?.body.length ?? 0) > 0) { return this._commentReply?.submitComment(); } } diff --git a/src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts b/src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts index e6ff2d08d696..4ef7bbc6025c 100644 --- a/src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts +++ b/src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts @@ -128,8 +128,8 @@ export class ReviewZoneWidget extends ZoneWidget implements ICommentThreadWidget editor: ICodeEditor, private _uniqueOwner: string, private _commentThread: languages.CommentThread, - private _pendingComment: string | undefined, - private _pendingEdits: { [key: number]: string } | undefined, + private _pendingComment: languages.PendingComment | undefined, + private _pendingEdits: { [key: number]: languages.PendingComment } | undefined, @IInstantiationService instantiationService: IInstantiationService, @IThemeService private themeService: IThemeService, @ICommentService private commentService: ICommentService, @@ -242,17 +242,17 @@ export class ReviewZoneWidget extends ZoneWidget implements ICommentThreadWidget } } - public getPendingComments(): { newComment: string | undefined; edits: { [key: number]: string } } { + public getPendingComments(): { newComment: languages.PendingComment | undefined; edits: { [key: number]: languages.PendingComment } } { return { newComment: this._commentThreadWidget.getPendingComment(), edits: this._commentThreadWidget.getPendingEdits() }; } - public setPendingComment(comment: string) { - this._pendingComment = comment; + public setPendingComment(pending: languages.PendingComment) { + this._pendingComment = pending; this.expand(); - this._commentThreadWidget.setPendingComment(comment); + this._commentThreadWidget.setPendingComment(pending); } protected _fillContainer(container: HTMLElement): void { diff --git a/src/vs/workbench/contrib/comments/browser/commentsController.ts b/src/vs/workbench/contrib/comments/browser/commentsController.ts index 60de02295a0b..c4cb1896a538 100644 --- a/src/vs/workbench/contrib/comments/browser/commentsController.ts +++ b/src/vs/workbench/contrib/comments/browser/commentsController.ts @@ -464,8 +464,8 @@ export class CommentController implements IEditorContribution { private _emptyThreadsToAddQueue: [Range | undefined, IEditorMouseEvent | undefined][] = []; private _computeCommentingRangePromise!: CancelablePromise | null; private _computeCommentingRangeScheduler!: Delayer> | null; - private _pendingNewCommentCache: { [key: string]: { [key: string]: string } }; - private _pendingEditsCache: { [key: string]: { [key: string]: { [key: number]: string } } }; // uniqueOwner -> threadId -> uniqueIdInThread -> pending comment + private _pendingNewCommentCache: { [key: string]: { [key: string]: languages.PendingComment } }; + private _pendingEditsCache: { [key: string]: { [key: string]: { [key: number]: languages.PendingComment } } }; // uniqueOwner -> threadId -> uniqueIdInThread -> pending comment private _inProcessContinueOnComments: Map = new Map(); private _editorDisposables: IDisposable[] = []; private _activeCursorHasCommentingRange: IContextKey; @@ -578,12 +578,12 @@ export class CommentController implements IEditorContribution { } } - if (pendingNewComment !== lastCommentBody) { + if (pendingNewComment.body !== lastCommentBody) { pendingComments.push({ uniqueOwner: zone.uniqueOwner, uri: zone.editor.getModel()!.uri, range: zone.commentThread.range, - body: pendingNewComment, + comment: pendingNewComment, isReply: (zone.commentThread.comments !== undefined) && (zone.commentThread.comments.length > 0) }); } @@ -896,7 +896,7 @@ export class CommentController implements IEditorContribution { }); let continueOnCommentText: string | undefined; if ((continueOnCommentIndex !== undefined) && continueOnCommentIndex >= 0) { - continueOnCommentText = this._inProcessContinueOnComments.get(uniqueOwner)?.splice(continueOnCommentIndex, 1)[0].body; + continueOnCommentText = this._inProcessContinueOnComments.get(uniqueOwner)?.splice(continueOnCommentIndex, 1)[0].comment.body; } const pendingCommentText = (this._pendingNewCommentCache[uniqueOwner] && this._pendingNewCommentCache[uniqueOwner][thread.threadId]) @@ -999,18 +999,18 @@ export class CommentController implements IEditorContribution { const matchedZones = this._commentWidgets.filter(zoneWidget => zoneWidget.uniqueOwner === thread.uniqueOwner && Range.lift(zoneWidget.commentThread.range)?.equalsRange(thread.range)); if (thread.isReply && matchedZones.length) { this.commentService.removeContinueOnComment({ uniqueOwner: thread.uniqueOwner, uri: editorURI, range: thread.range, isReply: true }); - matchedZones[0].setPendingComment(thread.body); + matchedZones[0].setPendingComment(thread.comment); } else if (matchedZones.length) { this.commentService.removeContinueOnComment({ uniqueOwner: thread.uniqueOwner, uri: editorURI, range: thread.range, isReply: false }); const existingPendingComment = matchedZones[0].getPendingComments().newComment; // We need to try to reconcile the existing pending comment with the incoming pending comment - let pendingComment: string; - if (!existingPendingComment || thread.body.includes(existingPendingComment)) { - pendingComment = thread.body; - } else if (existingPendingComment.includes(thread.body)) { + let pendingComment: languages.PendingComment; + if (!existingPendingComment || thread.comment.body.includes(existingPendingComment.body)) { + pendingComment = thread.comment; + } else if (existingPendingComment.body.includes(thread.comment.body)) { pendingComment = existingPendingComment; } else { - pendingComment = `${existingPendingComment}\n${thread.body}`; + pendingComment = { body: `${existingPendingComment}\n${thread.comment.body}`, cursor: thread.comment.cursor }; } matchedZones[0].setPendingComment(pendingComment); } else if (!thread.isReply) { @@ -1062,7 +1062,7 @@ export class CommentController implements IEditorContribution { return undefined; } - private async displayCommentThread(uniqueOwner: string, thread: languages.CommentThread, shouldReveal: boolean, pendingComment: string | undefined, pendingEdits: { [key: number]: string } | undefined): Promise { + private async displayCommentThread(uniqueOwner: string, thread: languages.CommentThread, shouldReveal: boolean, pendingComment: languages.PendingComment | undefined, pendingEdits: { [key: number]: languages.PendingComment } | undefined): Promise { const editor = this.editor?.getModel(); if (!editor) { return; @@ -1075,7 +1075,7 @@ export class CommentController implements IEditorContribution { if (thread.range && !pendingComment) { continueOnCommentReply = this.commentService.removeContinueOnComment({ uniqueOwner, uri: editor.uri, range: thread.range, isReply: true }); } - const zoneWidget = this.instantiationService.createInstance(ReviewZoneWidget, this.editor, uniqueOwner, thread, pendingComment ?? continueOnCommentReply?.body, pendingEdits); + const zoneWidget = this.instantiationService.createInstance(ReviewZoneWidget, this.editor, uniqueOwner, thread, pendingComment ?? continueOnCommentReply?.comment, pendingEdits); await zoneWidget.display(thread.range, shouldReveal); this._commentWidgets.push(zoneWidget); this.openCommentsView(thread); @@ -1361,12 +1361,12 @@ export class CommentController implements IEditorContribution { const providerEditsCacheStore = this._pendingEditsCache[info.uniqueOwner]; info.threads = info.threads.filter(thread => !thread.isDisposed); for (const thread of info.threads) { - let pendingComment: string | undefined = undefined; + let pendingComment: languages.PendingComment | undefined = undefined; if (providerCacheStore) { pendingComment = providerCacheStore[thread.threadId]; } - let pendingEdits: { [key: number]: string } | undefined = undefined; + let pendingEdits: { [key: number]: languages.PendingComment } | undefined = undefined; if (providerEditsCacheStore) { pendingEdits = providerEditsCacheStore[thread.threadId]; } @@ -1412,7 +1412,7 @@ export class CommentController implements IEditorContribution { lastCommentBody = lastComment.body.value; } } - if (pendingNewComment && (pendingNewComment !== lastCommentBody)) { + if (pendingNewComment && (pendingNewComment.body !== lastCommentBody)) { if (!providerNewCommentCacheStore) { this._pendingNewCommentCache[zone.uniqueOwner] = {}; }