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

10002: Add canReply to CommentThread #11062

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

CamilleLetavernier
Copy link
Contributor

@CamilleLetavernier CamilleLetavernier commented Apr 22, 2022

What it does

  • Add canReply to the CommentThread API
  • Show or Hide the Comment Reply widget based on the CommentThread.canReply value

How to test

  • Use the test extension: https://github.com/CamilleLetavernier/vscode-comment-api-example/releases

  • Select 2 files and use "Compare with each other" (Or open any DiffEditor)

  • Use the gutter to create a new comment thread

  • Use the "Toggle canReply" action to disable replies

  • Notes:

    • It seems that not all menus options are supported. The "Toggle canReply" command is also enabled for the CommentThread, but isn't displayed; so it's currently not possible to turn replies back on.
    • The PR is based on an older branch, because it seems that the upgrade for Monaco to VSCode v. 1.65.2 broke CommentThreads (See detailed comment below) Fixed in CommentThread support no longer works #11064
    • Because it is based on an older branch, I haven't push the Changelog (It would cause too much conflict at the moment) Fixed

Review checklist

Reminder for reviewers

Fixes #10002

Contributed on behalf of STMicroelectronics

Signed-off-by: Camille Letavernier [email protected]

@CamilleLetavernier
Copy link
Contributor Author

Toggling CommentThread.canReply:

comment-threads

@CamilleLetavernier
Copy link
Contributor Author

CamilleLetavernier commented Apr 22, 2022

The PR is based on an older branch, because it seems that the upgrade for Monaco to VSCode v. 1.65.2 broke CommentThreads (See detailed comment below)

When rebasing the PR on master, I get this exception when trying to open the CommentThread:

root ERROR TypeError: Cannot read properties of undefined (reading 'offsetX')
    at CommentsContribution.onEditorMouseDown (http://localhost:3000/packages_plugin-ext_lib_hosted_browser_hosted-plugin_js.js:1708:36)
    at http://localhost:3000/packages_plugin-ext_lib_hosted_browser_hosted-plugin_js.js:1655:63
    at http://localhost:3000/bundle.js:133954:69
    at CallbackList.invoke (http://localhost:3000/bundle.js:133960:26)
    at Emitter.fire (http://localhost:3000/bundle.js:134075:29)
    at http://localhost:3000/packages_monaco_lib_browser_monaco-workspace_js.js:207:37
    at Listener.invoke (http://localhost:3000/vendors-node_modules_theia_monaco-editor-core_esm_vs_editor_editor_main_js.js:28435:23)
    at Emitter.fire (http://localhost:3000/vendors-node_modules_theia_monaco-editor-core_esm_vs_editor_editor_main_js.js:28579:30)
    at ViewUserInputEvents.viewUserInputEvents.onMouseDown (http://localhost:3000/vendors-node_modules_theia_monaco-editor-core_esm_vs_editor_editor_main_js.js:66678:68)
    at ViewUserInputEvents.emitMouseDown (http://localhost:3000/vendors-node_modules_theia_monaco-editor-core_esm_vs_editor_editor_main_js.js:58155:18)

It seems that the MouseEvent target details is no longer correctly set by the editor, which breaks the CommentContribution

Edit: this is now fixed 👍

@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Apr 22, 2022
- Add canReply to the CommentThread API
- Show or Hide the Comment Reply widget based on the
CommentThread.canReply value

fixes eclipse-theia#10002

Contributed on behalf of STMicroelectronics

Signed-off-by: Camille Letavernier <[email protected]>
@CamilleLetavernier
Copy link
Contributor Author

Issue #11064 was created and fixed. I've rebased the PR on the current master and updated the Changelog, so this is now ready for review.

As far as I can tell, the 2 failed checks are unrelated to these changes (I see similar failures in other pull requests)

@colin-grant-work
Copy link
Contributor

@CamilleLetavernier, I pushed a fix for the error related to the Mocha tests - if you rebase, those failures should go away (or reveal something else that needs fixing...)

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The changes work as described. There are a number of UI differences between VSCode's comment threads and ours:

Initial State:
image

After first comment (note Toggle canReply button in upper right):
image

Theia:
Initial state:
image

After first comment:
image

In addition, VSCode's input uses the editor's monospace font; ours uses the UI font.

Do you have interest in reconciling those differences.

@JonasHelming
Copy link
Contributor

The changes work as described. There are a number of UI differences between VSCode's comment threads and ours:

Initial State: image

After first comment (note Toggle canReply button in upper right): image

Theia: Initial state: image

After first comment: image

In addition, VSCode's input uses the editor's monospace font; ours uses the UI font.

Do you have interest in reconciling those differences.

I create a new issue, thank your for highlighting this!

@JonasHelming JonasHelming merged commit adb4f7c into eclipse-theia:master Apr 28, 2022
@JonasHelming JonasHelming added this to the 1.25.0 milestone Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add canReply to CommentThread
4 participants