search

pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.24k stars 137 forks source link

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

Closed savetheclocktower closed 3 months ago

savetheclocktower commented 3 months ago

This reverts commit 8c6bef4d071dc22c65365aec24b5f24a5f213b6c, reversing changes made to 4dd32931dc95cc931d09e5bbdd5debfdf5789c5e.

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.

savetheclocktower commented 3 months ago

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 commented 3 months ago

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.