search

codemirror / codemirror5

In-browser code editor (version 5, legacy)
http://codemirror.net/5/
MIT License
26.84k stars 4.97k forks source link

Toggle Comment: Uncomment: Allow uncomment of selection containing an inner comment #7091

Closed davidfstr closed 6 months ago

davidfstr commented 7 months ago

Explanation of changed lines inside the "comment" addon: Screenshot 2024-03-27 at 1 59 29 PM

marijnh commented 7 months ago

Given that these lines were there, they were probably not just randomly typed there, but had a reason. Git blame points at 66c68db0c4fcbc6bb6ce735c2def8f03c3f43478, which points at issue https://github.com/codemirror/codemirror5/issues/4641 , which seems like it would be regressed again by merging this.

Most languages don't even have a concept of nested comments, so I'm not sure it's going to be possible to define the thing you're trying to do here in a solid way.

davidfstr commented 7 months ago

Thanks for taking a look @marijnh. -- And sorry for the delay on my end. I was out of town all last week.

issue https://github.com/codemirror/codemirror5/issues/4641 , which seems like it would be regressed again by merging this.

Reading that issue, I think that the code change I made would not regress the example given in the issue. I'll write a regression test that covers the example from that issue to confirm, and add it to this PR.

Most languages don't even have a concept of nested comments, so I'm not sure it's going to be possible to define the thing you're trying to do here in a solid way.

I'm trying to ensure that the Toggle Comment action, when done twice, ends up at the original state most of the time. VS Code (but not Sublime Text) get back to the original state with the example given in this PR's test code.

davidfstr commented 7 months ago

@marijnh I've pushed to this PR a regression test for the issue that you mentioned (https://github.com/codemirror/codemirror5/issues/4641). This PR does not regress that issue.

Beyond that issue, were there any other behaviors you were concerned this PR could break?

marijnh commented 7 months ago

There's also https://github.com/codemirror/codemirror5/issues/1975 , which seems at odds with what you're trying to do here. I feel uncommenting probably shouldn't remove things that are not actually comment, syntactically speaking.

davidfstr commented 6 months ago

I've added a regression test for issue #1975 that you mentioned. It also passes.

I feel uncommenting probably shouldn't remove things that are not actually comment, syntactically speaking.

I suppose the difficulty is that if there were an intermediate comment inside the selected area, it wouldn't be possible to accurately determine whether the end of the selection was actually inside a comment or not. If it looked like the end of the selection was outside of the comment but it wasn't, then toggle comment wouldn't work (as the original regression test for this issue shows).

How would you like me to proceed?

I see there is general resistance to the implementation approach of preventing Toggle Comment from uncommenting an area of code that CodeMirror thinks (rightly or wrongly) to be outside a comment.

Hypothetically CodeMirror could be extended to recognize a comment nested inside an outer comment, so that the outer comment is fully (and correctly) identified as a comment. That may be a tricky extension to write however.

marijnh commented 6 months ago

Hypothetically CodeMirror could be extended to recognize a comment nested inside an outer comment, so that the outer comment is fully (and correctly) identified as a comment.

No, that's the thing—if the language doesn't support nested comments, nested comments shouldn't be highlighted as such, since that will just confuse the user into thinking their code is parsed differently than it actually is. Since this version of the library isn't actively developed, I don't really want to change a lot about how it works and risk regressing things for existing users.

davidfstr commented 6 months ago

Since this version of the library isn't actively developed, I don't really want to change a lot about how it works and risk regressing things for existing users.

Makes sense. I'll just keep the proposed changes in my company's (small) internal fork of CodeMirror 5 then.

I'll open a new PR containing just the new regression tests from this PR, so that at least those can be integrated.