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 cursor placement #18471

Merged
merged 17 commits into from Apr 10, 2019

Conversation

Projects
None yet
3 participants
@Aerijo
Copy link
Member

commented Nov 21, 2018

Requirements for Adding, Changing, or Removing a Feature

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must contribute a change that has been endorsed by the maintainer team. See details in the template below.
  • The pull request must update the test suite to exercise the updated functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/atom/tree/master/CONTRIBUTING.md#pull-requests.

Issue or RFC Endorsed by Atom's Maintainers

#17519 (made this new one because I'd deleted the old PR branch)

Description of the Change

As in the above PR, this changes selection and cursor behaviour when toggling block comments.

Alternate Designs

An option to do this at the same place that changes the range in the first place would be better, but I don't know how to go about that.

Possible Drawbacks

It makes the behaviour more complicated, and could have a performance decrease. The behaviour is toggleable though, at the TextEditor level (I did not pass the options received by the Selection method though).

Verification Process

Manual testing, with all the scenarios laid out in the linked PR.

Also tested with indentation.

Release Notes

  • More intuitive cursor positioning when toggling block comments
@Aerijo

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

OK, so now the changes are quite different to the original.

I've added the exclusive: true flag to all cursors made through the TextEditor API. I may be missing something, but the behaviour described in the docs seems exactly what we're looking for

  #   * `exclusive` {Boolean} indicating whether insertions at the start or end
  #     of the marked range should be interpreted as happening *outside* the
  #     marker. Defaults to `false`, except when using the `inside`
  #     invalidation strategy or when when the marker has no tail, in which
  #     case it defaults to true. Explicitly assigning this option overrides
  #     behavior in all circumstances.

In general, inserting text nearby a cursor should not affect the cursor, so that sounds good. unfortunately, it doesn't handle inserting text on the same Point as a cursor as well as I'd hoped. The default seems to be to add it to the left, which is why we have a case for if the cursor's at the end, but not at the start. Indicating which way to "push" exclusive cursors would help here, and make it less dependent on defaults.

For selections, I stayed with the shift calculations from before. It seems to be working fine.

The following shows about all the tests I can think of. Indentation, multiple cursors, multiline. The one test that doesn't change was sort of shown by accident, because it only has 2 cursors and so they cancel each other out.
examples

Also want to add that it handles cursors on the comment delims too
example2

@Aerijo

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

This does change functionality for pressing tab when text is selected (the failing test). I'll have to remove the exclusive flag, which is a little disappointing. Unless that can be special cased? I still think exclusive fits better, but I don't know what else it might break.

let endDelta = oldRange.end.column === endLineLength ? [0, -commentEndString.length - 1] : [0, 0]
options.selection.setBufferRange(oldRange.translate(startDelta, endDelta), {autoscroll: false})
}
}

This comment has been minimized.

Copy link
@thomasjo

thomasjo Nov 21, 2018

Member

All of the let statements above can/should be const.

if (oldRange.start.column === endLineLength) {
let endCol = endLineLength - commentEndString.length - 1
options.selection.setBufferRange([[end, endCol], [end, endCol]], {autoscroll: false})
return

This comment has been minimized.

Copy link
@thomasjo

thomasjo Nov 21, 2018

Member

Maybe I'm missing something, but as far as I can tell this return statement is unnecessary.

Aerijo added some commits Nov 21, 2018

@Aerijo

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

There is one undesired behaviour now (that I know of), for three selections where the first goes right to left on the leftmost character (line 13 in first GIF).

It currently will select the start delim, but only when there are three or more of them. It's weird. (I do suggest we beat VSCode here though, because it will just insert three nested sets of delims).

Edit: I don't think it's possible to support 3+ cursors cleanly. My understanding is, as changes are applied in order from left to right, the first selection is handled properly (as seen with only one cursor), the second reverts the comment, and the third makes the first do it's default expand-when-text-is-inserted behaviour. This was fixed by exclusive: true, but sometimes we actually want the selection to expand to the added text (like when indenting). We could track all this, or temporarily set all selection (cursors) to exclusive, but I don't know if it's worth the effort / complexity.

@Aerijo

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

FYI @maxbrunsfeld This test seems to have randomly failed.

Aerijo added some commits Nov 23, 2018

@Aerijo

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

@lee-dohm (I moved to a different PR)

I'm pretty happy with the behaviour now. There are technically still more cases that can be tested, but at the same time I don't think we can exhaustively cover them all. The behaviour, except for as note about line 13, is as in the GIFs above.

@nathansobo nathansobo self-assigned this Apr 8, 2019

@@ -7314,6 +7314,41 @@ describe('TextEditor', () => {
editor.toggleLineCommentsForBufferRows(0, 0)
expect(editor.lineTextForBufferRow(0)).toBe('test')
})

it('does not select the new delimiters'), () => {

This comment has been minimized.

Copy link
@nathansobo

nathansobo Apr 8, 2019

Contributor

Hi. Unfortunately, the closing parentheses after the description string in this test means that the block following the test isn't running at all. So the test is green only because it isn't really running. I'll try and get the test passing, but I may kick this back to you if it ends up being too much work.

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Okay, I merged in master and fixed a couple problems with the test once I got it actually running. I'd like to stare at this a bit longer and see if we can simplify it at all. Stay tuned.

Clean up assertion style
I prefer to express only one assertion per line rather than &&-ing 
together multiple assertions into a condition. I also prefer to use 
equality assertions so that failure messages include more information 
about the actual and expected values.

I used nested scope blocks so we could re-define the `range` constant in 
a local scope without needing to mutate a variable across unrelated 
tests.

@nathansobo nathansobo force-pushed the Aerijo:comment-cursor-placement branch from 7bede80 to 6277cef Apr 8, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Alright. Your earlier comments about using exclusive prompted some investigation. As you also seemed to discover, the exclusive option on the selection's marker works great unless the selection is empty. In that case, the cursor is still pushed to the right when commenting at the end of the line. We really need a mechanism like a bias that causes the endpoints of a marker to prefer to stay left or move right when there is an insertion at their location. It's something we had in Xray. But for the purposes of fixing this issue, I think your approach is reasonable. Just gonna wait on a green build.

nathansobo added some commits Apr 8, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Oh jeez I left a test focused. Rebuilding 🙄.

@nathansobo nathansobo merged commit 57d00b6 into atom:master Apr 10, 2019

2 checks passed

Atom Pull Requests #20190409.4 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.