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

Toggle comment doesn't toggle back off comments but keeps adding comm… #447

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

vrubezhny
Copy link
Contributor

…ents chars #445

Signed-off-by: Victor Rubezhny vrubezhny@redhat.com

@sebthom
Copy link
Member

sebthom commented Oct 5, 2022

@mickaelistria PR looks good to me. can you verify that it solves the issue for you?

@mickaelistria
Copy link
Contributor

It does fix the issue I currently face, but I noticed 2 pitfalls (that we may cover in other PRs if that's preferred):

  • The selection is messed up after the toggle comment action. To reproduce select 2 lines in an XML file, use Ctrl+Shift+C and selection moves only to a subset of the 2nd line.
  • Toggle is applied line by line, so if one line has comment and other hasn't and both are selected and then toggle comment is invoked, then 1 line is un-commented and the other is commented. I believe the usual expectation, as built by JDT is that the same operation (comment or uncomment according to eg 1st line) is applied to all selected lines.

@vrubezhny
Copy link
Contributor Author

  • Toggle is applied line by line, so if one line has comment and other hasn't and both are selected and then toggle comment is invoked, then 1 line is un-commented and the other is commented. I believe the usual expectation, as built by JDT is that the same operation (comment or uncomment according to eg 1st line) is applied to all selected lines.

.. and this is exactly the same algorithm was applied previously on Toggle Single-Line comment before I changed the behavior.
If in Java source we have selected several lines and some of them are already starting with a single-line comment a new single-line comment is added to such a line - comments are duplicating... which was a problem that we were solving in TM4E...

The only difference I see (from Java case) is that for xml-based text we don't have a single-line comment template - there is only a multi-line comments, while in Java we have both - single-line and multi-line comments.

So a solution I see is:

  • First row in text selection defines the operation to be performed - add or remove comments - for the rest of the selected rows
  • if a single-line comment exists in TM4E for a document type - perform the Toggle single-line comments action exactly like it's done in Java (including the single-line comment duplication)
  • if NO single-line comment exists in TM4E for a document type - then use multi-line comment template and NOT duplicate the comment for a raw that is already commented out.

WDYT?

…ents chars eclipse-tm4e#445

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>
@mickaelistria
Copy link
Contributor

This looks good to me. Any reason why it's still a draft? Anything left to do before we can merge?

@vrubezhny
Copy link
Contributor Author

This looks good to me. Any reason why it's still a draft? Anything left to do before we can merge?

The selection is still messed up if a line has a block comment inside the text or more than one block comment. So I'm working on make it to be similar to what we have for Java editor (regarding the block comments) including the selection change.

@mickaelistria
Copy link
Contributor

Couldn't we merge this increment, that I believe does cover very well the vast majority of cases, and track that other issue in a separate ticket/PR?

@vrubezhny vrubezhny marked this pull request as ready for review October 18, 2022 10:54
@vrubezhny
Copy link
Contributor Author

I think yes, this PR can be merged.
Other changes will follow in a new PR

@mickaelistria mickaelistria merged commit 2a91189 into eclipse-tm4e:master Oct 18, 2022
@mickaelistria
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggle comment doesn't toggle back off comments but keeps adding comments chars
3 participants