Skip to content

notebook comment support#145090

Merged
rebornix merged 29 commits intomainfrom
rebornix/notebook-commenting
Mar 17, 2022
Merged

notebook comment support#145090
rebornix merged 29 commits intomainfrom
rebornix/notebook-commenting

Conversation

@rebornix
Copy link
Member

@rebornix rebornix commented Mar 15, 2022

This PR fixes #144850. The changes included:

  • Decoupling the CommentThreadWidget from ZoneWidget and make it reuseable for Notebook.
  • CommentThread to CommentThread<T> where T is either IRange or ICellRange.
  • CellComments cell part in notebook

Originally I thought we should at least introduce a new API to allow contributing comments to notebook cells but at the end I took a shortcut: extensions contribute comments to notebook cells and we render comments in notebook cells instead of inside the cell editor.

@rebornix
Copy link
Member Author

@alexr00 FYI, to support comments in notebook editor, I decoupled the comment widget from the zoneWidget API and make it reuseable. The functionalities of the comment widget in monaco should not be changed.

@rebornix rebornix merged commit f890d96 into main Mar 17, 2022
@rebornix rebornix deleted the rebornix/notebook-commenting branch March 17, 2022 19:59
@alexr00 alexr00 restored the rebornix/notebook-commenting branch March 18, 2022 09:21
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

I took a quick look, but it's a large change so I couldn't be super thorough.


private readonly _threads: Map<number, MainThreadCommentThread> = new Map<number, MainThreadCommentThread>();
public activeCommentThread?: MainThreadCommentThread;
private readonly _threads: Map<number, MainThreadCommentThread<IRange | ICellRange>> = new Map<number, MainThreadCommentThread<IRange | ICellRange>>();
Copy link
Member

Choose a reason for hiding this comment

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

Should MainThreadCommentController also be templated so that IRange | ICellRange can be a T?

if (matchedZones.length) {
let matchedZone = matchedZones[0];
matchedZone.update(thread);
if (thread.isDocumentCommentThread()) {
Copy link
Member

Choose a reason for hiding this comment

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

We now need to have these checks in various places. Would it make more sense to have a separate notebook comments service?

Copy link
Member

Choose a reason for hiding this comment

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

or, at the very least, separate onDidUpdateCommentThreads events for documents and notebooks?

Copy link
Member Author

Choose a reason for hiding this comment

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

💯 for filtering through onDidUpdateCommentThreads.

@alexr00 alexr00 deleted the rebornix/notebook-commenting branch March 18, 2022 09:33
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Explore cell level commenting in notebooks

3 participants