[vscode] introduction of the CommentingRange type #15015

rschnekenbu · 2025-02-24T10:40:24Z

What it does

Introduce the support for Commenting Ranges and capability to create comments on file rather than on a specific range.

also it fixes the creation of comments, with arguments passed to the command being the one expected by argument processor

fix #14941

It is interesting to note that commenting range is supported, but the API do not let the extensions create Thread with not range (see https://github.com/microsoft/vscode/blob/c5549bc00e5f2e17773e9cffa694996fff318772/src/vscode-dts/vscode.d.ts#L17413). So this is hard to test and debug, even on vscode itself... For now, theia code support the ranges to be undefined, but it is not possible to test at runtime. Weird!

How to test

Steps to test:

Install this extension to test the feature on comparing markdown or typescript files (commenting ruler is available on Theia on diff comparator):

src:comment-sample-0.0.4-src.zip
zip vsix: vscode-comment-api-example-0.0.4.zip
Using this extension, you get 2 comment providers:
the markdown (*.md files) comment controller provide 10 lines range every 20 lines and also allow file comments,
the typescript (*.ts files) allow commenting on every line.

Open the comparison between 2 files (for example, 2 typescript files or 2 markdown files using compare with each other menu
You can see in the diff editor the comment gutter (grey bar) on the side of the text editors, you can create a comment while clicking on the gutter. Notice that the md files have a gutter that is starting every 20 lines for a length of 10 lines. ts files have a gutter for the whole file length.

The UI seems currently broken on comments, I commented on a followup issue for comments action and UI: #12452. (css are bad, actions do no support the 'when' condition, etc.). But this is not relevant for this API implementation

Follow-ups

Improvements on Comment support:

Improvements on buttons and UI: #12452

Breaking changes

This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

contributed on behalf of STMicroelectronics

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

also fix creation of comments, with arguments passed to the command being the one expected by argument processor fix eclipse-theia#14941 Contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>

tsmaeder · 2025-02-25T08:05:49Z

@rschnekenbu not sure how to test this: is the commenting UI available in Theia before this PR? It's not clear to me how I can invoke the commenting functionality from the video 🤷 Side note: is this an animated gif? If so, that is very inconvenient for Firefox users, because I don't get the video controls: I have to watch it from the start each time I want to go back.

rschnekenbu · 2025-02-25T08:15:41Z

@tsmaeder , the UI is existing (and was not working) prior to this PR. To test it, you have to compare 2 different files (in my case, a markdown file and a copy of it) using the compare with each other menu when the 2 are selected. In this case, the comment gutter (a grey bar) is shown in the Diff Editor.
The area covered by the gutter is the one given by the comment providers in the extension. In the case of MD files, 10 lines can be commented, 10 not, and the same again. For typescript, any line can be commented.
For the gif, that is the most portable solution over the OS and the browsers but indeed with no controls.

tsmaeder · 2025-02-25T08:18:41Z

I'm not seeing the "comment gutter" 🤷 And if I see it, how do I invoke the comment UI?

rschnekenbu · 2025-02-25T08:21:29Z

Do you have any error in the log when the app is supposed to activate?
When you see it and your mouse is on top of the gutter, a [+] grey icon is showing on top of the gutter.

tsmaeder · 2025-02-25T08:32:13Z

I see

Error: AbstractContextKeyService has been disposed
    at E.contextMatchesRules (contextKeyService.ts:319:10)
    at o.createActionGroups (menuService.ts:285:33)
    at C.getActions (menuService.ts:453:25)
    at h._getValue (gutterFeature.ts:39:97)
    at b.handleEvent [as value] (utils.ts:129:25)
    at t.DebounceEmitter._deliver (event.ts:1243:13)
    at t.DebounceEmitter.fire (event.ts:1274:9)
    at t.DebounceEmitter.resume (event.ts:1423:12)
    at event.ts:1462:10

in the browser log.

tsmaeder · 2025-02-25T08:33:10Z

Now I did a "Compare with each other" again. No error in the log this time, but still not gutter. I'm using the 1.59 preview build on Windows for testing this, btw.

rschnekenbu · 2025-02-25T08:43:52Z

I will test again on my side, but I never see such an issue. I do not get why the contextKeyService shall be disposed.

rschnekenbu · 2025-02-25T10:52:44Z

@tsmaeder I added some steps to clarify the testing.

tsmaeder · 2025-02-25T10:59:15Z

What I was missing is that you need to install the extension in order to make the UI show up 🤷

tsmaeder · 2025-02-25T14:49:32Z

@rschnekenbu I get the "+" signs in the gutter as well on the left side of the left editor in the diff for removed sections. However, when I click on the plus, nothing happens (no error log either).

tsmaeder · 2025-02-25T08:54:16Z

packages/plugin-ext/src/main/browser/comments/comment-thread-widget.tsx

@@ -651,8 +653,10 @@ export class CommentActions extends React.Component<CommentActions.Props> {
                    commands={commands}
                    node={node}
                    onClick={() => {
+                        console.log('CommentActions.onClick: ' + commentThread);


Development leftover?

tsmaeder · 2025-02-25T14:58:51Z

packages/plugin-ext/src/plugin/comments.ts

@@ -221,7 +245,8 @@ export class ExtHostCommentThread implements theia.CommentThread, theia.Disposab
    readonly onDidUpdateCommentThread = this._onDidUpdateCommentThread.event;

    set range(range: theia.Range) {
-        if (!range.isEqual(this._range)) {
+        if ( ((range === undefined) !== (this._range === undefined))


The signature of the range parameter does not allow it to be undefined. It probably should, considering we're checking for it.

This is now possible to change to an undefined range. Evolution should come later with the ability to create comments without range from the API.

tsmaeder · 2025-02-25T15:32:22Z

packages/plugin-ext/src/main/browser/comments/comment-thread-widget.tsx

@@ -256,8 +256,8 @@ export class CommentThreadWidget extends BaseWidget {
        const frameThickness = Math.round(lineHeight / 9) * 2;
        const body = this.zoneWidget.containerNode.getElementsByClassName('body')[0];

-        const computedLinesNumber = Math.ceil((headHeight + body.clientHeight + arrowHeight + frameThickness + 8 /** margin bottom to avoid margin collapse */) / lineHeight);
-        this.zoneWidget.show({ afterLineNumber: this._commentThread.range.startLineNumber, heightInLines: computedLinesNumber });
+        const computedLinesNumber = Math.ceil((headHeight + body?.clientHeight + arrowHeight + frameThickness + 8 /** margin bottom to avoid margin collapse */) / lineHeight);


Probably needs body?.clientHeight ?? 0, no?

changed in next commit

tsmaeder · 2025-02-25T15:35:00Z

packages/plugin-ext/src/plugin/comments.ts

-            if (ranges) {
-                return ranges.map(x => fromRange(x));
+            const commentingRanges:
+                | theia.Range[]


Can we not let type inference determine the type here?

tsmaeder · 2025-02-25T15:36:36Z

packages/plugin-ext/src/plugin/comments.ts

+                    fileComments: commentingRanges.enableFileComments
+                };
+            } else {
+                return commentingRanges ?? undefined;


We already now that !commentingRanges here, why not just return undefined?

indeed, being null or undefined, we don't care as we want undefined only. Removed in next commit.

rschnekenbu · 2025-02-25T17:07:44Z

@tsmaeder could you finally get the comment dialog to open ?

tsmaeder · 2025-02-26T13:00:41Z

@rschnekenbu I get the "+" signs in the gutter as well on the left side of the left editor in the diff for removed sections. However, when I click on the plus, nothing happens (no error log either).

@rschnekenbu ping?

rschnekenbu · 2025-02-26T15:38:35Z

@tsmaeder, indeed, the left editor can't have comments. That does not make sense for the set of changes I have introduced, probably some other defect existing prior to this change. But in any case, I will have a look and check why left editor does not support commenting.

tsmaeder · 2025-02-26T15:56:50Z

If it's not a regression, I don't have a problem merging this one, but I can't really test that because the plugin does not work without the PR.

rschnekenbu · 2025-02-26T16:11:43Z

The onMouseDownEvent in comments-contribution.ts is not send when clicking on the left gutter. This seems an issue with the current implementation of the comments contribution and not from the API evolution.

* [vscode] introduction of the CommentingRange type also fix creation of comments, with arguments passed to the command being the one expected by argument processor fix eclipse-theia#14941 Contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>

rschnekenbu added the vscode issues related to VSCode compatibility label Feb 24, 2025

rschnekenbu requested a review from tsmaeder February 24, 2025 11:03

tsmaeder reviewed Feb 25, 2025

View reviewed changes

address review comments

4837b8a

tsmaeder approved these changes Feb 26, 2025

View reviewed changes

tsmaeder merged commit 03751ab into eclipse-theia:master Feb 26, 2025
10 of 11 checks passed

github-actions bot added this to the 1.59.0 milestone Feb 26, 2025

[vscode] introduction of the CommentingRange type #15015

[vscode] introduction of the CommentingRange type #15015

Uh oh!

Conversation

rschnekenbu commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it does

How to test

Follow-ups

Breaking changes

Attribution

Review checklist

Reminder for reviewers

Uh oh!

tsmaeder commented Feb 25, 2025

Uh oh!

rschnekenbu commented Feb 25, 2025

Uh oh!

tsmaeder commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschnekenbu commented Feb 25, 2025

Uh oh!

tsmaeder commented Feb 25, 2025

Uh oh!

tsmaeder commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschnekenbu commented Feb 25, 2025

Uh oh!

rschnekenbu commented Feb 25, 2025

Uh oh!

tsmaeder commented Feb 25, 2025

Uh oh!

tsmaeder commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rschnekenbu commented Feb 25, 2025

Uh oh!

tsmaeder commented Feb 26, 2025

Uh oh!

rschnekenbu commented Feb 26, 2025

Uh oh!

tsmaeder commented Feb 26, 2025

Uh oh!

rschnekenbu commented Feb 26, 2025

Uh oh!

Uh oh!

Uh oh!

rschnekenbu commented Feb 24, 2025 •

edited

Loading

tsmaeder commented Feb 25, 2025 •

edited

Loading

tsmaeder commented Feb 25, 2025 •

edited

Loading