Support Notebook comments on Markup cells

[rehmsen]

I would like code review comments in notebooks to also work on non-code cells. Currently the code explicitly filters to only code cells, I am not sure why:

vscode/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts

Lines 157 to 164 in dee8eaf

	override didRenderCell(element: ICellViewModel): void {
	if (element.cellKind === CellKind.Code) {
	this.currentElement = element as CodeCellViewModel;
	this.initialize(element);
	this._bindListeners();
	}
	}

Is there a reason why comments on markup cells should be hidden, or would you accept a PR to add support for comments on Markup Cells?

@alexr00

[rebornix]

@rehmsen that was a pretty early exploration and we don't have support for the notebook commenting officially. With that said, I'm good with PRs for enabling it for Markup cell.

[VSCodeTriageBot]

This feature request is now a candidate for our backlog. The community has 60 days to upvote the issue. If it receives 20 upvotes we will move it to our backlog. If not, we will close it. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

[rehmsen]

I took a look at this today and wanted to propose a rough plan for how to implement this:

1. Add a new property commentOffset to the CodeCellLayoutInfo, so that cellComments does not need to know that the comments should go below the outputs - that computation would move into CodeCellViewModel.layoutChange. This is the first step to making cellComments.ts agnostic of the type of cell.
2. Extract shared interfaces CellLayoutInfo and CellLayoutChangeEvent from CodeCellLayoutInfo/MarkupCellLayoutInfo and CodeCellLayoutChangeEvent/MarkupCellLayoutChangeEvent. They already share a bunch of fields, and actually putting them into a parent interface means that cellComments.ts can read and write the relevant fields (commentHeight and commentOffset) without needing to distinguish between Code and Markup cells.
3. (Optional) share some of the logic of how these shared layout values are computed. That is not strictly necessary and in the case of offsets might not be possible because they depend on which UI elements are above them pushing them down. But those things that are the same could benefit from some deduping. That code could live in BaseCellViewModel.
4. Make MarkupCellViewModel compute commentOffset based on its view hierarchy.
5. Make cellComments use the commentHeight setter and the cellOffset getter from the ICellViewModel instead of casting to CodeCellViewModel.

@alexr00 @rebornix Please let me know if that sounds good to you.

[rzhao271]

@rehmsen could I get some verification steps for this feature request?
I haven't been able to find a way to add comments to a notebook.
The diff editor is also editable on the right side, which seems off to me.
 

[rehmsen]

@rebornix Can you help?

I also do not know how to add a code review comment for a notebook via the UI. We have a closed source extension which registers a comment provider that creates notebook comments, and I verified that those work on Markdown cells with the fix.

Open source I know that the Github Pull Requests extension provides Github comments, but those are always left on the raw file, not on notebook cells, see #214017 - I have suggested some ideas how one might fix that, but we have not reached agreement yet.

I there some other way to create a notebook comment @rebornix? I guess you had some way to verify this when you build the original comment support?

Regarding the diff editor, editing the right side is expected I think - the left side should not be editable. Is it? Is that caused by my change somehow? I tried to repro, but when I run ./scripts/code-web.sh, I cannot install any notebook extension (like jupyter) in the first place.

[rzhao271]

Marking this issue as verified considering the fix works for you.

Also, the left side of the diff editor is not modifiable. I'll check whether the diff editor is modifiable on the right side on Stable.

Edit: I have confirmed that the diff editor has not regressed.