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

Use display layers facility of text-buffer; delete all the code they replace #11414

Merged
merged 151 commits into from May 4, 2016

Conversation

nathansobo
Copy link
Contributor

This PR replaces a bunch of old and twisty code in TextEditor with usage of a new facility in the text-buffer library introduced in atom/text-buffer#149. You can read that pull request for more information about what replaces all the code we're deleting here.

Nathan Sobo and others added 30 commits January 13, 2016 12:17
Markers are still translating via the DisplayBuffer, but that will
change when display markers are moved into DisplayLayers.
I’m creating the DisplayLayer in the DisplayBuffer. In places where our
API deviates from DisplayBuffer, I’ll use the DisplayLayer directly from
as a property of TextEditor. Otherwise I’ll continue to delegate from
the DisplayLayer into the DisplayLayer to minimize impact until the
DisplayBuffer can be removed entirely.
This causes DisplayLayer to emit change events when syntax is updated
asynchronously.
# Conflicts:
#	package.json
#	src/display-buffer.coffee
#	src/text-editor.coffee
#	src/tokenized-buffer.coffee
Calling ::updateHorizontalDimensions might cause the editor vertical
coordinates (e.g. height, scroll top) to change, so we need to fetch
lines again from `DisplayLayer`.
@50Wliu
Copy link
Contributor

50Wliu commented Apr 29, 2016

Noticed something while trying to move off a long line:
long-line
When clicking another line that's not visible, the screen does not update. I have to type something or move the cursor using the arrow keys in order for the correct location to show up.
Not sure if this is related to this branch though.

@as-cii
Copy link
Contributor

as-cii commented Apr 29, 2016

When clicking another line that's not visible, the screen does not update. I have to type something or move the cursor using the arrow keys in order for the correct location to show up.
Not sure if this is related to this branch though.

Yeah, seems like master exhibits the same behavior too: I think we explicitly disabled autoscrolling on mouse click because the resulting UX is somewhat worse (e.g. clicking on a visible character which is close to the left margin of the rendered area causes the editor to scroll). Maybe we need finer grained control over autoscroll, but I'd say to address this on a separate pull-request. Thanks for the heads-up, @50Wliu!

Antonio Scandurra added 2 commits April 30, 2016 11:10
...because the only possible scenario when a logical position in a text
node cannot be found is when the requested pixel position is exactly at
the end of the node.
@50Wliu
Copy link
Contributor

50Wliu commented Apr 30, 2016

Here's another regression when trying to select lines (ignore the double-click in the beginning, my mouse is running out of battery):
incorrect-selection

Something similar happens when trying to move the cursor to the first non-whitespace character, though this one is harder to reproduce:
incorrect-cursor-move

This is on Atom 1.9.0-dev-7b2f049.

@as-cii
Copy link
Contributor

as-cii commented Apr 30, 2016

@50Wliu: I am pretty sure this got fixed with the latest commit. Can you try reproducing it on 334b4c1?

@50Wliu
Copy link
Contributor

50Wliu commented Apr 30, 2016

Yup, fixed. Thanks!

@amytruong
Copy link

@as-cii 👋 I was testing find & replace (in buffer) and came upon this error message.
screen shot 2016-05-02 at 2 26 44 pm

Then, I switched to find & replace in project and got an issue with mini map (which seems to be known?) but trying to reproduce it and it isn't happening again (minimap is working fine now)
screen shot 2016-05-02 at 2 30 12 pm

Antonio Scandurra added 2 commits May 4, 2016 18:56
abe33 added a commit to atom-minimap/minimap that referenced this pull request May 4, 2016
@as-cii as-cii merged commit ca04138 into master May 4, 2016
@as-cii as-cii deleted the ns-switch-to-display-layers branch May 4, 2016 18:46
@maxbrunsfeld
Copy link
Contributor

👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏

@t9md
Copy link
Contributor

t9md commented May 6, 2016

I'm working on catching-up displayLayer change in my vim-mode-plus package t9md/atom-vim-mode-plus#269.

Noticed two thing.

Deprecation warning when I pass persistent option like blow.

Reported to atom/text-buffer#154

editor.markBufferRange editor.getSelectedBufferRange(),
    invalidate: 'never'
    persistent: false

Text wrap point diff when editor.setEditorWidthInChars()

Not sure this is intended of not.

Noticed by some failing spec(checking behavior in wrapped line).

Maybe related to this commit?

bef7539

Here minimum spec to reproduce

describe "Reproduce wrap diff", ->
  it "wrap line diff with 1.9.0-dev-d06da3f and 1.8.3-beta3", ->
    editor = null
    waitsForPromise ->
      atom.workspace.open(null).then (_editor) ->
        editor = _editor
        editor.setSoftWrapped(true)
        editor.setEditorWidthInChars(10)
        editor.setDefaultCharWidth(1)

        editor.setText """
          1st line of buffer
          2nd line of buffer, Very long line
          3rd line of buffer

          5th line of buffer\n
          """

    runs ->
      {inspect} = require 'util'

      console.log atom.appVersion
      lineTexts = (editor.lineTextForScreenRow(screenRow) for screenRow in [0..4])
      console.log inspect(lineTexts)

      # v1.8.0-beta3
      # [ '1st line ',
      #   'of buffer',
      #   '2nd line ',
      #   'of buffer, ',
      #   'Very long ' ]

      # 1.9.0-dev-d06da3f
      # [ '1st line ', 'of buffer', '2nd line ', 'of ', 'buffer, ' ]

@as-cii
Copy link
Contributor

as-cii commented May 6, 2016

Not sure this is intended of not.

Yes, while in the middle of redesigning this part of Atom, we took some time to revisit how lines are soft-wrapped so that edits to the buffer could have been displayed in a very explicit manner to the user.

In the specific case you've mentioned, Atom notices that of buffer, is 11 characters long and will split the line at the beginning of the last word that fitted within 10 characters. In addition, if a line is made of whitespaces only, Atom will wrap that too, as opposed to keep any number of contiguous whitespace characters within the same line even if they wouldn't fit the requested soft-wrap column.

@t9md
Copy link
Contributor

t9md commented May 6, 2016

So new wrapping strategy is more intelligent from humans expectation viewpoin.
Great, thanks for explaining that.

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.

None yet

10 participants