Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Toggle comment cursor placement #17519

Closed
wants to merge 4 commits into from
Closed

Toggle comment cursor placement #17519

wants to merge 4 commits into from

Conversation

Aerijo
Copy link
Contributor

@Aerijo Aerijo commented Jun 16, 2018

Description of the Change

When toggleLineComments is triggered, it will now correct the selection range to be within the comment delimiters.

It will activate when

  • correctSelection is true (it is hardcoded true right now when toggling comments),
  • The end comment delim is truthy,
  • The line is being commented

If these conditions pass, it will then get the selection range and see if the head or tail needs to be moved. The range is adjusted, with autoscroll disabled to be consistent with exisiting behaviour (and it would be really annoying otherwise).

  • Note: because the delim is added by modifying the entire line, there is no need to move the cursor if it is not at an end. The bug only happens because the cursor being at an end counts as outside the modification range, and so is pushed along when the change is applied.

Alternate Designs

I originally had it only work for empty selections, but decided this way is better.

It does seem quite long compared to what it's supposed to do though. I don't know how much of a problem that is.

This is applied to every selection in the editor when line comments are toggled. It's possible people only want it when there is just one selection / cursor. An interesting bug(?) is that when there's an even number of cursors on a line, it gets toggled twice, resulting in no change (this bug isn't related to this PR though).

Why Should This Be In Core?

It's a core mechanic

Benefits

Consistent with other text editors, and makes sense.

Possible Drawbacks

There's an interesting ...(this.getBufferRowRange() || []) statement in the original version. I'm not sure when getBufferRowRange will return a falsey value, but that would shift the arguments of the function, moving the options parameter to start.

Originally, if it does evaluate to [], no error is thrown but the top line would be comment toggled. That seems like a bug, but isn't related to this PR. I've changed the empty array to [null, null] though, to ensure the argument positions are not changed.

Verification Process

I tested manually, with the CSS grammar. I'll try to write proper tests after some feedback for changes, etc.

For now, this is what I'm expecting (where | is selection head/tail)

## Single cursor in line
ab|c
->
/* ab|c */

## Selection in line
a|b|c
->
/* a|b|c */

## Single cursor at end
abc|
->
/* abc| */

## Single cursor at beginning
|abc
->
/* |abc */

## Selection on whole line
|abc|
->
/* |abc| */

## Multi-whole line selection
|a
b|
 ->
/* |a
b| */

## Multi-partial line selection
a|bc
abc
abc|
->
/* a|bc
abc
abc| */

a|bc
abc
ab|c
->
/* a|bc
abc
ab|c */

Applicable Issues

Fix #4784

@lee-dohm
Copy link
Contributor

Would you mind adding tests for this?

@Aerijo
Copy link
Contributor Author

Aerijo commented Nov 23, 2018

Moved to #18471

@Aerijo Aerijo closed this Nov 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants