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

[vscode] support CommentThread state property #12454

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

rschnekenbu
Copy link
Contributor

What it does

Add Comment#state optional property
This adds the state optional property from Comment to improve vscode api coverage. It also displays the state in the Comment Thread widget

Fixes #12231

Contributed on behalf of ST Microelectronics

How to test

Note: there are currently some issues with menus, since theia 1.28.0. This is due to #11290, integrated in 1.28.0. Prior to this change, the comment sample works on theia 1.27.0. So to test this change, I cherry-picked my commit on top of 1.27.0 tag.
This is the same case as for PR #12007.
The issue #12452 has been created to track those problems about menus.

  1. Install forked extension #comment-sample-0.0.1 vsix file - Source
  2. Create a comment using the side bar while comparing 2 test files and reply with some comment with toggle or reply.
    12231
  3. The resolved state should be displayed along the thread label.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added vscode issues related to VSCode compatibility comments issues related to comments api and functionality labels Apr 25, 2023
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The code looks good, I unfortunately cannot really test due to the menu issues you presented.
@rschnekenbu you might want to enable the ability for maintainers to perform edits so we can update the branch as needed so it satisfies the "out-of-date" check.

@rschnekenbu
Copy link
Contributor Author

@vince-fugnitto , thanks a lot for your review!

Still would it make sense to accept the PR? We could add a check on the issue #12452 for this API to be tested through the sample extension. The same should also be done for the already integrated Comment#timestamp property support.

And also, unfortunately, I can't switch this ability to perform edit on my own, despite being the creator of the PR. I don't have access to the configuration of the PR. We already tried with Thomas for previous PRs, as he wanted to be able to rebase my PRs on latest main.

solves eclipse-theia#12231

Contributed on behalf of STMicroelectronics

Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
@rschnekenbu
Copy link
Contributor Author

I rebased on top of master and I fixed the minor comment.

@vince-fugnitto
Copy link
Member

@rschnekenbu I think that’s fine, we can revisit and retest this pull-request once the menu is fixed, did I understand that you wanted to fix it?

@rschnekenbu
Copy link
Contributor Author

@vince-fugnitto, I won't have time to fix the menu issue before next release unfortunately. I already did some investigation, but the refactoring introduced on #11290 was too complex for me to understand the root cause of the issue.
As for the comment timestamp, I would be in favor of pushing the PR. Indeed, the feature is working as expected on 1.27 and the changes are isolated. Along with #12453 & #12456, that would allow next theia release to be API compatible with vscode 1.77.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Let's go ahead and keep #12452 in mind to properly review the changes once we fix the regression.

@rschnekenbu
Copy link
Contributor Author

Thanks for the review, @vince-fugnitto!
I don't have the ability to merge (no write access). Can you merge it please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comments issues related to comments api and functionality vscode issues related to VSCode compatibility
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[vscode] Support CommentThread state property
2 participants