Revert "Merge pull request #810 from pulsar-edit/fix-on-change-cursor-pos"

[savetheclocktower]

This reverts commit 8c6bef4, reversing changes made to 4dd3293.

#810 caused a bug in the Selection#deleteToBeginningOfLine method:

1. Make sure Soft Wrap is enabled.
2. Open a file with at least one line long enough to be soft-wrapped. Make your window narrower if need be.
3. Put the cursor anywhere in the file after the occurrence of at least one soft wrap.
4. Invoke Editor: Delete To Beginning Of Line (keyboard shortcuts are platform-specific; on macOS it's Cmd+Delete.
5. Observe how the wrong text is deleted.

This happens because we're constructing a Range to decide which text gets deleted, but the beginning of the range is specified using a screen coordinate rather than a buffer coordinate.

(Suppose you have a file with one line of content 160 characters wide, and you soft-wrap at 80. When you put the cursor on the second screen row, textEditor.getLastCursor().getScreenRow() will return 1; textEditor.getLastCursor().getBufferRow() will return 0.)

This explains why this bug didn't occur in the specs, and why the behavior of this command will vary based on where you are in the file. (And folks who've disabled editor.softWrap might not even notice this bug, since buffer and screen coordinates are identical when soft-wrap is disabled.)

There are fixes we could apply short of reverting #810, but I'm proposing that we merge this in the short term so that we can at least point people at a rolling release if they complain about it.

In a couple days, we can figure out whether 1.118.1 is just a revert of #810… or a reapplication of #810 with fixes and more test coverage.

[DeeDeeG]

Am I allowed to approve my own commit?

In all seriousness, it feels right to give others a chance to look at this, so lite-approve from me, if no-one else opines on the matter within ~24 hours-ish, you can take this as a full-approve I guess.

[savetheclocktower]

I mean, I'd approve if I hadn't opened the PR; I think your approval counts. I think we might as well land this once CI passes.

Nothing prevents us from reintroducing #810 with fixes — even tomorrow, if we managed to do it that quickly — but I can't imagine anyone would have an issue with temporarily un-regressing some code.

[DeeDeeG]

I think this may be a couple of genuine test failure, somehow?

In the find-and-replace tests:

Expected undefined to have class 'selected'.
Expected [really long HTML snippet] to have class 'selected'.

ResultsView
  core:move-to-top and core:move-to-bottom
    it selects the first/last item when core:move-to-top/move-to-bottom is triggered
      Expected undefined to have class 'selected'.
        at jasmine.Spec.<anonymous> (results-view-spec.js:321:71)
    it selects the path when when core:move-to-bottom is triggered and last item is collapsed
      Expected '<li class="list-nested-item"><div class="list-item path-row" data-file-path="/home/runner/work/pulsar/pulsar/node_modules/find-and-replace/spec/fixtures/project/sample.coffee"><span data-name="sample.coffee" class="icon-file-text icon"></span><span class="path-name bright">sample.coffee</span><span class="path-match-number">(5 matches)</span></div></li>' to have class 'selected'.
        at jasmine.Spec.<anonymous> (results-view-spec.js:338:101)


ALL TESTS THAT FAILED:
ResultsView core:move-to-top and core:move-to-bottom selects the first/last item when core:move-to-top/move-to-bottom is triggered
ResultsView core:move-to-top and core:move-to-bottom selects the path when when core:move-to-bottom is triggered and last item is collapsed

EDIT: Passed on a re-run, it would seem.