Change how selection/caret location is determined after applying togg… by dibarbet · Pull Request #51411 · dotnet/roslyn

dibarbet · 2021-02-23T04:13:49Z

…le line comment

Two main changes in this PR:

When toggling a line comment with a selection, we leave the selection unchanged
When toggling a line comment with just a caret, the line is commented and the caret moves down to the next line at the same column position (or end of line)

Resolves #36349
Resolves #36351

…le line comment

sharwell

When toggling a line comment with just a caret, the line is commented and the caret moves down to the next line at the same column position (or end of line)

~~Oh please no. I use this the way it is today.~~

I use the Insert Comment and Remove Comment commands, not the Toggle Comment command

allisonchou

lgtm 🎉

allisonchou · 2021-02-25T19:25:02Z

+                    var caretColumn = caretLine.GetColumnFromLineOffset(caretOffset, editorOptions);
+
+                    var nextLine = selectedSpan.Snapshot.GetLineFromLineNumber(nextLineNumber);
+                    // Compute the correct offset location from the column in the previous line.


a super nit (you don't have to take this lol, I personally just think it looks a little cleaner with the newline):

Suggested change

// Compute the correct offset location from the column in the previous line.

// Compute the correct offset location from the column in the previous line.

CyrusNajmabadi · 2021-02-25T19:50:46Z

+            }
+
+            // In selection or any multi-caret scenarios, leave the selection unchanged.
+            return new CommentTrackingSpan(selectedSpan.Span.ToTextSpan());


can we invert the check so the simple case comes first :)

CyrusNajmabadi · 2021-02-25T20:13:40Z

How confident are we that this won't piss people off? :) the 'keep selection the same' part seems fine to me. The 'move to next line' seems odd. Is this parity with another editor?

dibarbet · 2021-02-25T20:35:20Z

@CyrusNajmabadi

How confident are we that this won't piss people off? :) the 'keep selection the same' part seems fine to me. The 'move to next line' seems odd. Is this parity with another editor?

It's a fair question. The original design review outcome is here - #36351 (comment)

We decided to take this specifically without an option. I don't recall the exact discussion since it's been a while, but the points in favor of this are

This extension provides toggle comment with similar behavior - https://marketplace.visualstudio.com/items?itemName=JustinClareburtMSFT.HotCommandsforVisualStudio . This is also similar to the behavior of intellij, though VSCode does not do this.
The command itself is fairly new (introduced midway through dev16) and the existing userbase is not incredibly large (we see ~4k unique daily users). I think we're less likely to significantly impact muscle memory for a lot of people.

As such I would rather introduce this change and revisit with an option if we do get feedback later on.

CyrusNajmabadi · 2021-02-25T20:45:02Z

Not sure how I feel about this:

though VSCode does not do this.

CyrusNajmabadi · 2021-02-25T20:45:47Z

I think we could separate this into two prs. Take the selection part immediately. Discuss the newline decision with vscode. I'd rather us be consistent on this.

dibarbet · 2021-02-25T21:03:56Z

@CyrusNajmabadi sure- I created #51493 for leaving the selection unchanged. I will rebase this PR on it once it merges. Could you take a look at that one?

justcla · 2021-02-25T23:49:33Z

Thanks for fixing the selection issue.

Regarding the move-line-down, an option would be cool, but if you had to make a choice, moving down a line is the more logical choice as moving to the next line is the more common next-action.
If anyone has ever used the feature WITH the move-down option (like with IntelliJ or HotCommands), they would agree it's definitely the preferred choice most of the time.

VSCode should probably consider updating to the move-line-down behaviour.

justcla · 2021-02-25T23:52:53Z

I think we could separate this into two prs. Take the selection part immediately. Discuss the newline decision with vscode. I'd rather us be consistent on this.

This was reviewed and decided upon back in 2019. Here is the relevant post.
#36351 (comment)

"We took this one for design review on Monday. We agree that this functionality would be good, and we would accept a PR that would move the caret down to the next line when the toggle line comment command is invoked on a caret location."

"We do not want to add a new options for this."

justcla · 2021-02-25T23:57:34Z

Two main changes in this PR:

When toggling a line comment with a selection, we leave the selection unchanged

To be clear, this should apply to all lines included in the selection (except where caret posn = 0).

So if it's a MULTI-LINE selection and the user applies ToogleLineComment, every line should have line comment applied or removed, but the selection will be unchanged - spanning multiple lines.
To avoid ambiguity about "toggling a line comment", I would state it like this:

When applying ToggleLineComment to any non-zero length selection, we leave the selection unchanged.

Edit:
I notice that the tests do account for this scenario. Good work.

justcla · 2021-02-26T00:32:12Z

In the case where the selection starts at the first char on a line, the selection changes to include the comment markers at the start. This is not expected behaviour and is inconsistent with the idea of not changing the selection.

As an example, if the start of the selection was one more char to the right, it would not include the comment makers.

The problem stems from adding comment markers mid-line. If comment markers always go at the beginning of the line, which they might as well, then it's easy to achieve consistency with comment behavior. Will consider raising new issue to apply comment markers to the start of each line.

dibarbet · 2021-02-26T00:54:20Z

I personally like the move down a line behavior now that I've tried as well. I'm also not sure that unifying behavior here is 100% necessary - I don't know if users moving from VSCode have a strong expectation that VS functions exactly the same. In so many other ways they already differ.

I would lean towards just doing what we decided previously in design review, though I can bring it up again if others are strongly against this.

justcla · 2021-02-26T01:07:59Z

Agreed. VSCode already has a very different set of features and shortcuts.

And definitely go with default move-line-down behaviour in the case of zero-length selection. :-D
It changes the way you code. It's awesome.

…nt_improvements

Change how selection/caret location is determined after applying togg…

51b2239

…le line comment

dibarbet added the Area-IDE label Feb 23, 2021

dibarbet requested a review from a team as a code owner February 23, 2021 04:13

dibarbet force-pushed the toggle_line_comment_improvements branch from 594f9b3 to 51b2239 Compare February 23, 2021 04:13

sharwell previously requested changes Feb 23, 2021

View reviewed changes

allisonchou approved these changes Feb 25, 2021

View reviewed changes

CyrusNajmabadi reviewed Feb 25, 2021

View reviewed changes

Comment thread src/EditorFeatures/Core/Implementation/CommentSelection/ToggleLineCommentCommandHandler.cs Outdated

dibarbet mentioned this pull request Feb 25, 2021

Leave selection unchanged when toggling line comment #51493

Merged

dibarbet added 2 commits February 26, 2021 15:26

Merge remote-tracking branch 'upstream/master' into toggle_line_comme…

30f517f

…nt_improvements

formatting

19acf5b

Base automatically changed from master to main March 3, 2021 23:53

	// Compute the correct offset location from the column in the previous line.

	// Compute the correct offset location from the column in the previous line.

Conversation

dibarbet commented Feb 23, 2021

Uh oh!

sharwell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonchou left a comment

Choose a reason for hiding this comment

Uh oh!

allisonchou Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi commented Feb 25, 2021

Uh oh!

dibarbet commented Feb 25, 2021

Uh oh!

CyrusNajmabadi commented Feb 25, 2021

Uh oh!

CyrusNajmabadi commented Feb 25, 2021

Uh oh!

dibarbet commented Feb 25, 2021

Uh oh!

justcla commented Feb 25, 2021

Uh oh!

justcla commented Feb 25, 2021

Uh oh!

justcla commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justcla commented Feb 26, 2021

Uh oh!

dibarbet commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justcla commented Feb 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sharwell left a comment •

edited

Loading

justcla commented Feb 25, 2021 •

edited

Loading

dibarbet commented Feb 26, 2021 •

edited

Loading