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

Rewrite editor rendering layout to use new browser features and virtual DOM #13880

Merged
merged 306 commits into from May 5, 2017

Conversation

Projects
None yet
@nathansobo
Contributor

nathansobo commented Feb 26, 2017

In this PR, I plan to completely rewrite editor rendering to take advantage of CSS layout containment and intersection observers. This PR will also use Etch to perform declarative updates instead of the existing presenter with manual DOM manipulation, which has not aged well. I'm optimistic that a simpler layout, a cleaner approach to DOM updates, and better use of features such as layout containment can make our rendering faster and much more maintainable.

My goal is to rethink all of the DOM interactions associated with the editor to make sure things are as optimal as possible.

Tasks

  • Content
    • Render initial lines
    • Render initial line numbers
    • Update on scroll
    • Update on buffer changes
    • Adjust soft wrap on resize
    • Scroll past end option
    • Mini editors working
    • Hide line numbers option
    • Placeholder text
    • Auto height and width
    • Synthetic scrolling
    • Dummy scrollbars
  • Cursors
    • Basic rendering
    • Auto-scroll
      • Vertical
      • Horizontal
    • Blinking
    • Option to hide with non-empty selections
    • Cursors positioned after a fold marker are not displayed correctly
    • Official API for customizing cursor styling
  • Input handling
    • Position hidden input element
    • Focus
    • Editor commands
    • Text input
    • IME input
    • Disabled input
    • Mouse handling
      • Single click to set cursor position
      • Double click to select word
      • Triple click to select line
      • Add cursors with cmd-click
      • Expand selections with shift-click
      • Drag selections
      • Autoscroll when dragging selections
      • Autoscroll when dragging gutter
      • Click/drag gutter to select lines
      • Click gutter to toggle folding
      • Destroy folds when clicking fold markers
      • Middle mouse paste on Linux
      • Scroll horizontally on shift-wheel
  • Decorations
    • Make marker layer events synchronous
    • Line
    • Line Number
    • Highlights
    • Highlight flashes
    • Overlays
    • Custom gutters
    • Blocks
    • Never decorate invalidated markers
    • Fix git-diff gutter decorations being shown at the end of the tile
  • Update on external changes
    • Resize
    • Gutter container resize
    • Font size and styling changes
    • Show/hide
    • Refresh scrollbar appearance when scrollbar styling changes
  • Other
    • Core tests passing
    • Package tests passing
    • Delete obsolete tests
    • Do we need to support onDidChangeFirstVisibleScreenRow?
    • Add screen-row data fields to line nodes for package compatibility
    • Audit API of previous TextEditorElement implementation
    • Clean up editor style sheets
    • Revisit virtual node recycling and explore other approaches
    • Schedule updates directly in editor model instead of via subscriptions
    • Ensure proper setup/teardown on attach/detach
    • Failed to activate the pigments package, cannot find highlights-component in the require cache We'll need to introduce a new API to support the use case that @abe33 is monkey patching private implementation details to support. We'll need to merge this before doing that.
    • Maintain logical scroll position when changing font size
  • Regressions
    • Ensure autoscrolling works correctly (when clicking "go to code" the cursor is positioned correctly, but scroll top is wrong).
    • Stack-overflow exception with Nuclide Debugger (#13880 (comment))
    • Failed to activate the minimap-autohider package, cannot read property onDidChangeScrollTop of null
    • Uncaught TypeError: this.textEditorElement.getFirstVisibleScreenRow is not a function from minimap
    • Uncaught TypeError: editorElement.component.pixelPositionForMouseEvent is not a function from linter-ui-default
    • With atom-material-syntax matching brackets are not highlighted
    • highlight-selected does not highlight selected words but it works when setting a custom highlight-selected style in the personal style sheet.
    • Matching brackets are not highlighted when they occur on the same line and material-syntax is on (#13880 (comment))
    • Uncaught TypeError: Cannot read property 'id' of null (#13880 (comment))
    • Content of mini editors is shifted right as if there were a gutter, but there isn't. (#13880 (comment))
    • Subpixel anti-aliasing sometimes breaks for a specific tile, possibly due to its contents overflowing.
    • Tiles are laid out incorrectly during the first frame (#13880 (comment))
    • Editor width for soft-wrap should take into account vertical scroll bar width
    • Clicking past the last line does not update cursor position when scrollPastEnd is disabled.
  • Optimization (any of these can happen after integrating and merging)
    • Contain layout of cursor position and selection count in status bar
    • When a tile is assigned a different start row, discard all of its lines rather than removing them 1 by 1
    • Recycle DOM nodes?
    • Optimize line numbers query in display layer
    • Cache isFoldable
    • Low-hanging fruits in keystroke code path

@nathansobo nathansobo changed the base branch from master to tj-upgrade-electron Feb 27, 2017

@as-cii as-cii referenced this pull request Mar 30, 2017

Merged

Upgrade Electron to v1.6.x #12696

14 of 17 tasks complete
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 10, 2017

Contributor

@t9md I was just looking for usages of the cursor visibility API and noticed vim-mode-plus is the only real user. I also noticed you were reaching into some private state to get DOM nodes for the cursors. You may want to take a look at this PR since it changes editor rendering pretty dramatically. I'm planning to drop the visibility APIs. If you describe the API features you need with respect to cursors perhaps we can find a supported way to give you what you need.

Contributor

nathansobo commented Apr 10, 2017

@t9md I was just looking for usages of the cursor visibility API and noticed vim-mode-plus is the only real user. I also noticed you were reaching into some private state to get DOM nodes for the cursors. You may want to take a look at this PR since it changes editor rendering pretty dramatically. I'm planning to drop the visibility APIs. If you describe the API features you need with respect to cursors perhaps we can find a supported way to give you what you need.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 10, 2017

Contributor

@as-cii Not sure where you're at with block decorations but I'm seeing a small failure in a test related to them.

Contributor

nathansobo commented Apr 10, 2017

@as-cii Not sure where you're at with block decorations but I'm seeing a small failure in a test related to them.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Apr 11, 2017

Contributor

Thanks @nathansobo for pinging me.

Here is purpose of each.
(I use the word vmp to indicate vim-mode-plus for shortness.)

What I wrote here are all done in cursor-style-manager.coffee.

Why vmp directly access cursor DOM node?

  • To modify cursor style dynamically and respectively for each cursor.
  • For example, in vmp's linewise selection mode( when you type V ), cursor is stil movable via normal l, h or w motions.
  • But in Atom, linewise selection's cursor is always at column-0 of actually selected line.
  • So what I'm doing in vmp is here
    1. Save column before select whole-line, then select whole-line(thats linewise selection).
    2. Modifify curosr node's style prop to set offset( top and left ) so that cursor is displayed at preserved column.
    3. Since cursor column can be different on each cursor, this style modification must be done against each cursor node.
  • In characterwise selection
    • Vmp shows non-reversed selection's cursor position at one column left.
    • In other word, when selection-head was same as end position, cursor is shown at one column left of end position of that selection.

cursor-mode-in-linewise-selection

Why I use cursor visibility API?

  • To hide specific cursor in visual-blockwise-mode.
  • In vmp, visual-blockwise mode is implemented by using multiple selections.
  • blockwiseSelection consists of multiple selections and all member selection in same blockwiseSelection start with same column and end with same column.
  • In blockwise mode only leader-selections's cursor is visible and other member selection's cursor is hidden( by calling cursor.setVisible ).

In following GIF, blockwise selection consists of multiple selections, and show cursor for only leader-selection.
blockwise-selection

What feature vmp want?

  • Same as previous release.
  • Want to modify cursor's style dynamically on each cursor.
    • What vmp currently doing is set top and left to make cursor displayed at different place from actual position.
  • Show/hide cursor API is preferable, but I'm OK at least I can control cursor visibility via direct cursor style modification( such like display: none).
Contributor

t9md commented Apr 11, 2017

Thanks @nathansobo for pinging me.

Here is purpose of each.
(I use the word vmp to indicate vim-mode-plus for shortness.)

What I wrote here are all done in cursor-style-manager.coffee.

Why vmp directly access cursor DOM node?

  • To modify cursor style dynamically and respectively for each cursor.
  • For example, in vmp's linewise selection mode( when you type V ), cursor is stil movable via normal l, h or w motions.
  • But in Atom, linewise selection's cursor is always at column-0 of actually selected line.
  • So what I'm doing in vmp is here
    1. Save column before select whole-line, then select whole-line(thats linewise selection).
    2. Modifify curosr node's style prop to set offset( top and left ) so that cursor is displayed at preserved column.
    3. Since cursor column can be different on each cursor, this style modification must be done against each cursor node.
  • In characterwise selection
    • Vmp shows non-reversed selection's cursor position at one column left.
    • In other word, when selection-head was same as end position, cursor is shown at one column left of end position of that selection.

cursor-mode-in-linewise-selection

Why I use cursor visibility API?

  • To hide specific cursor in visual-blockwise-mode.
  • In vmp, visual-blockwise mode is implemented by using multiple selections.
  • blockwiseSelection consists of multiple selections and all member selection in same blockwiseSelection start with same column and end with same column.
  • In blockwise mode only leader-selections's cursor is visible and other member selection's cursor is hidden( by calling cursor.setVisible ).

In following GIF, blockwise selection consists of multiple selections, and show cursor for only leader-selection.
blockwise-selection

What feature vmp want?

  • Same as previous release.
  • Want to modify cursor's style dynamically on each cursor.
    • What vmp currently doing is set top and left to make cursor displayed at different place from actual position.
  • Show/hide cursor API is preferable, but I'm OK at least I can control cursor visibility via direct cursor style modification( such like display: none).

nathansobo added a commit that referenced this pull request Apr 13, 2017

nathansobo added a commit that referenced this pull request Apr 17, 2017

nathansobo added a commit that referenced this pull request Apr 18, 2017

@as-cii as-cii referenced this pull request Apr 18, 2017

Merged

Fix tests on Electron >= 1.6 #33

nathansobo added a commit that referenced this pull request Apr 18, 2017

as-cii added a commit to atom/fuzzy-finder that referenced this pull request Apr 19, 2017

Don't use textContent to verify the empty project scenario
This will make tests pass on atom/atom#13880 and
is a more reliable approach in general.

thomasjo added a commit that referenced this pull request Apr 19, 2017

nathansobo added a commit that referenced this pull request Apr 19, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 20, 2017

Contributor

@t9md Can you check out the cursor decorations API I just added and see if it would work for your use case?

Contributor

nathansobo commented Apr 20, 2017

@t9md Can you check out the cursor decorations API I just added and see if it would work for your use case?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Apr 20, 2017

Member

Found a cursor regression while testing this branch out: the cursor will switch to the "default" pointer after the text ends on each line or in between lines.
cursor-default

(Also, line numbers look off, like they're too far to the left or something)

Member

50Wliu commented Apr 20, 2017

Found a cursor regression while testing this branch out: the cursor will switch to the "default" pointer after the text ends on each line or in between lines.
cursor-default

(Also, line numbers look off, like they're too far to the left or something)

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Apr 21, 2017

Contributor

@nathansobo Thanks, you are super smart! As far as I read spec and api-doc in comment, seems great!

Want to use this new decoration type: cursor to evaluate more specifically.
But how?
Can I build this branch locally? how did you test this branch @50Wliu?

Contributor

t9md commented Apr 21, 2017

@nathansobo Thanks, you are super smart! As far as I read spec and api-doc in comment, seems great!

Want to use this new decoration type: cursor to evaluate more specifically.
But how?
Can I build this branch locally? how did you test this branch @50Wliu?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Apr 21, 2017

Member
git checkout ns-editor-rendering
script/build --install
Member

50Wliu commented Apr 21, 2017

git checkout ns-editor-rendering
script/build --install
@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Apr 21, 2017

Contributor

Oh, OK that is what I'm now trying.

Contributor

t9md commented Apr 21, 2017

Oh, OK that is what I'm now trying.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Apr 21, 2017

Contributor

When I checked with commit 6979b43 , editorElement.classList always result in just editor.
It have set atom-text-editor initially and modify classList to add vim-mode-plus like scope, but in somewhere cleared and become just editor.
This maybe reason of what @50Wliu reported( cursor-visibility become normal pointer ).

Contributor

t9md commented Apr 21, 2017

When I checked with commit 6979b43 , editorElement.classList always result in just editor.
It have set atom-text-editor initially and modify classList to add vim-mode-plus like scope, but in somewhere cleared and become just editor.
This maybe reason of what @50Wliu reported( cursor-visibility become normal pointer ).

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Apr 21, 2017

Contributor

macos_md_ _build-instructions_ ___github_atom_org

This is output of same editor.element.classList, first two is in my plugin(vim-mode-plus) before and after add vim-mode-plus class, last one is manually check classList on same editor.

Contributor

t9md commented Apr 21, 2017

macos_md_ _build-instructions_ ___github_atom_org

This is output of same editor.element.classList, first two is in my plugin(vim-mode-plus) before and after add vim-mode-plus class, last one is manually check classList on same editor.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Apr 21, 2017

Member

Did some quick testing this morning here is what I come up with:

I am using 18d7a6a Windows 7 64-bit and safe mode (Except for the packages section)

  • Scrolling horizontally with shift-wheel does not work
  • Editor scroll position is not restored across reloads or restarts. The editor is scrolled to the correct location in the file that was opened when reloading but the scrollbar is at the top of the file.
  • Following warning in the console when reloading, maybe this also happens on master have not checked yet
IntersectionObserver.observe(target): target element is not a descendant of root.

TreeView @ <embedded>:98028
getTreeViewInstance @ <embedded>:59258
deserialize @ <embedded>:67791
deserialize @ <embedded>:19110
(anonymous) @ <embedded>:27723
module.exports.Pane.deserialize @ <embedded>:27722
deserialize @ <embedded>:19110
deserialize @ <embedded>:27188
deserialize @ <embedded>:29194
deserialize @ <embedded>:25078
module.exports.AtomEnvironment.deserialize @ <embedded>:1475
(anonymous) @ <embedded>:1056
  • Everything does feel faster 💯 ⚡️

Applicable issues?

I can't reproduce these. They are either fixed by this or other changes between the report and now. Have not checked if I can reproduce them on master yet.

Packages

  • Failed to activate the minimap-autohider package, cannot read property onDidChangeScrollTop of null
  • Failed to activate the pigments package, cannot find highlights-component in the require cache
  • Uncaught TypeError: this.textEditorElement.getFirstVisibleScreenRow is not a function from minimap
  • Uncaught TypeError: editorElement.component.pixelPositionForMouseEvent is not a function from linter-ui-default
  • With atom-material-syntax matching brackets by the bracket-matcher are not highlighted
  • highlight-selected does not highlight selected words. It does when I use my custom highlight-selected styles that I have in my stylesheet but not without it.
Member

Ben3eeE commented Apr 21, 2017

Did some quick testing this morning here is what I come up with:

I am using 18d7a6a Windows 7 64-bit and safe mode (Except for the packages section)

  • Scrolling horizontally with shift-wheel does not work
  • Editor scroll position is not restored across reloads or restarts. The editor is scrolled to the correct location in the file that was opened when reloading but the scrollbar is at the top of the file.
  • Following warning in the console when reloading, maybe this also happens on master have not checked yet
IntersectionObserver.observe(target): target element is not a descendant of root.

TreeView @ <embedded>:98028
getTreeViewInstance @ <embedded>:59258
deserialize @ <embedded>:67791
deserialize @ <embedded>:19110
(anonymous) @ <embedded>:27723
module.exports.Pane.deserialize @ <embedded>:27722
deserialize @ <embedded>:19110
deserialize @ <embedded>:27188
deserialize @ <embedded>:29194
deserialize @ <embedded>:25078
module.exports.AtomEnvironment.deserialize @ <embedded>:1475
(anonymous) @ <embedded>:1056
  • Everything does feel faster 💯 ⚡️

Applicable issues?

I can't reproduce these. They are either fixed by this or other changes between the report and now. Have not checked if I can reproduce them on master yet.

Packages

  • Failed to activate the minimap-autohider package, cannot read property onDidChangeScrollTop of null
  • Failed to activate the pigments package, cannot find highlights-component in the require cache
  • Uncaught TypeError: this.textEditorElement.getFirstVisibleScreenRow is not a function from minimap
  • Uncaught TypeError: editorElement.component.pixelPositionForMouseEvent is not a function from linter-ui-default
  • With atom-material-syntax matching brackets by the bracket-matcher are not highlighted
  • highlight-selected does not highlight selected words. It does when I use my custom highlight-selected styles that I have in my stylesheet but not without it.
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Apr 21, 2017

Member

Scrolling horizontally with shift-wheel does not work

This looks like a bug in Electron, as I am able to shift-scroll on macOS. I tested Ubuntu as well and it doesn't work there either. I will provide a workaround for it on this branch, but cc: @kevinsawicki for 👀 on the Electron side of things.

Editor scroll position is not restored across reloads or restarts. The editor is scrolled to the correct location in the file that was opened when reloading but the scrollbar is at the top of the file.

Great catch, I am working on fixing this right now. 👍

Following warning in the console when reloading, maybe this also happens on master have not checked yet

Yeah, I don't think it's related to this branch. If it doesn't happen on master, it's possible that this warning was introduced in a recent Chrome version. I haven't checked out tree-view yet to see how IntersectionObserver is used, but in the worst case I think we could keep the warning as it's harmless.

Failed to activate the minimap-autohider package, cannot read property onDidChangeScrollTop of null

Failed to activate the pigments package, cannot find highlights-component in the require cache

We should probably write an issue in those packages to notify authors about the upcoming changes.

Uncaught TypeError: this.textEditorElement.getFirstVisibleScreenRow is not a function from minimap

Uncaught TypeError: editorElement.component.pixelPositionForMouseEvent is not a function from linter-ui-default

With atom-material-syntax matching brackets by the bracket-matcher are not highlighted
highlight-selected does not highlight selected words. It does when I use my custom highlight-selected styles that I have in my stylesheet but not without it.

We should definitely provide these methods and investigate why bracket-matcher doesn't highlight things with atom-material-syntax. I will add these regressions to a section of this pull request's body.

Member

as-cii commented Apr 21, 2017

Scrolling horizontally with shift-wheel does not work

This looks like a bug in Electron, as I am able to shift-scroll on macOS. I tested Ubuntu as well and it doesn't work there either. I will provide a workaround for it on this branch, but cc: @kevinsawicki for 👀 on the Electron side of things.

Editor scroll position is not restored across reloads or restarts. The editor is scrolled to the correct location in the file that was opened when reloading but the scrollbar is at the top of the file.

Great catch, I am working on fixing this right now. 👍

Following warning in the console when reloading, maybe this also happens on master have not checked yet

Yeah, I don't think it's related to this branch. If it doesn't happen on master, it's possible that this warning was introduced in a recent Chrome version. I haven't checked out tree-view yet to see how IntersectionObserver is used, but in the worst case I think we could keep the warning as it's harmless.

Failed to activate the minimap-autohider package, cannot read property onDidChangeScrollTop of null

Failed to activate the pigments package, cannot find highlights-component in the require cache

We should probably write an issue in those packages to notify authors about the upcoming changes.

Uncaught TypeError: this.textEditorElement.getFirstVisibleScreenRow is not a function from minimap

Uncaught TypeError: editorElement.component.pixelPositionForMouseEvent is not a function from linter-ui-default

With atom-material-syntax matching brackets by the bracket-matcher are not highlighted
highlight-selected does not highlight selected words. It does when I use my custom highlight-selected styles that I have in my stylesheet but not without it.

We should definitely provide these methods and investigate why bracket-matcher doesn't highlight things with atom-material-syntax. I will add these regressions to a section of this pull request's body.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Apr 21, 2017

Contributor

When reloading the window, I'm seeing one frame where the text is laid out incorrectly. Here's a GIF:

glitch

And here's the two frames where the text changes:

glitch-frame-1

glitch-frame-2

Contributor

maxbrunsfeld commented Apr 21, 2017

When reloading the window, I'm seeing one frame where the text is laid out incorrectly. Here's a GIF:

glitch

And here's the two frames where the text changes:

glitch-frame-1

glitch-frame-2

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 24, 2017

Contributor

screen shot 2017-04-23 at 9 09 16 pm

Another rendering quirk to investigate. The nv-atom mini editor's scroll view is initially shifted over as if to account for the gutter position, but there is no gutter. At some point things got re-measured and it shifted to the correct position.

Contributor

nathansobo commented Apr 24, 2017

screen shot 2017-04-23 at 9 09 16 pm

Another rendering quirk to investigate. The nv-atom mini editor's scroll view is initially shifted over as if to account for the gutter position, but there is no gutter. At some point things got re-measured and it shifted to the correct position.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Apr 24, 2017

Member

I still see problems with highlighting matching brackets when using atom-material-syntax on a75d0ab. It works on master. For brackets on the same line there is no highlighting at all. Brackets on other lines are highlighted. With other themes like one-dark-syntax the matching brackets are highlighted correctly.

On master:

master not same line
master same line

On this branch:
editor rendering not same line
editor rendering same line

Member

Ben3eeE commented Apr 24, 2017

I still see problems with highlighting matching brackets when using atom-material-syntax on a75d0ab. It works on master. For brackets on the same line there is no highlighting at all. Brackets on other lines are highlighted. With other themes like one-dark-syntax the matching brackets are highlighted correctly.

On master:

master not same line
master same line

On this branch:
editor rendering not same line
editor rendering same line

thomasjo added a commit that referenced this pull request Apr 24, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 24, 2017

Contributor

@Ben3eeE Should have fixed your bracket matcher issue with 550e703.

Contributor

nathansobo commented Apr 24, 2017

@Ben3eeE Should have fixed your bracket matcher issue with 550e703.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 24, 2017

Contributor

@t9md Your classes should be preserved as of the latest commit. Previously we were using diff-based rendering to assign the class name, but that didn't account for classes that were added from the outside. I updated it to a more imperative implementation. It's not as pretty but it shouldn't break vim-mode. Please let me know if anything else breaks for you as vim-mode-plus is a super important package for the ecosystem.

Contributor

nathansobo commented Apr 24, 2017

@t9md Your classes should be preserved as of the latest commit. Previously we were using diff-based rendering to assign the class name, but that didn't account for classes that were added from the outside. I updated it to a more imperative implementation. It's not as pretty but it shouldn't break vim-mode. Please let me know if anything else breaks for you as vim-mode-plus is a super important package for the ecosystem.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Apr 25, 2017

Contributor

@nathansobo

new-cursor-decoration-type

I could achieve same cursor style customization with commit ca9d678. 🎉

  • add top, left style to show cursor in different position from actual one.
  • hide specific cursor by setting visibility style.

What I'm still not sure is
- How I can dispose these custom style(I'm now doing manually)?
- Do I need to destroy or dispose decoration created by type: 'cursor'?

EDIT: Answer to myself. I can dispose style by destroying decoration returned by editor.decorateMarker. and should destroy to avoid managed decorations increase infinitely.

decoration = editor.decorateMarker(cursor.getMarker, type: 'cursor', style: {left: '0ch'}

# Does this necessary? or any way to dispose custom style in consistent manner?
decoration.destroy()
Contributor

t9md commented Apr 25, 2017

@nathansobo

new-cursor-decoration-type

I could achieve same cursor style customization with commit ca9d678. 🎉

  • add top, left style to show cursor in different position from actual one.
  • hide specific cursor by setting visibility style.

What I'm still not sure is
- How I can dispose these custom style(I'm now doing manually)?
- Do I need to destroy or dispose decoration created by type: 'cursor'?

EDIT: Answer to myself. I can dispose style by destroying decoration returned by editor.decorateMarker. and should destroy to avoid managed decorations increase infinitely.

decoration = editor.decorateMarker(cursor.getMarker, type: 'cursor', style: {left: '0ch'}

# Does this necessary? or any way to dispose custom style in consistent manner?
decoration.destroy()
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Apr 26, 2017

Member
  1. Scroll in minimap using mouse wheel

Atom: 1.18.0-dev-a89bd67 x64
Electron: 1.6.5
OS: Microsoft Windows 7 Professional
Thrown From: minimap package 4.27.1

Stack Trace

Uncaught TypeError: this.getTextEditorElement(...).component.onMouseWheel is not a function

At C:\Users\lineri\.atom\packages\minimap\lib\minimap-element.js:1138

TypeError: this.getTextEditorElement(...).component.onMouseWheel is not a function
    at HTMLElement.relayMousewheelEvent (/packages/minimap/lib/minimap-element.js:1138:45)
    at /packages/minimap/lib/minimap-element.js:492:16)
    at HTMLElement.listener (/packages/minimap/node_modules/atom-utils/lib/mixins/events-delegation.js:108:41)

Commands

     -1:23.8.0 intentions:highlight (input.hidden-input)
     -1:23.6.0 core:paste (input.hidden-input)
     -1:23.1.0 core:undo (input.hidden-input)
     -1:15.7.0 intentions:highlight (input.hidden-input)
     -1:15.5.0 core:paste (input.hidden-input)
     -1:15 core:undo (input.hidden-input)
     -0:59.4.0 intentions:highlight (input.hidden-input)
     -0:59.2.0 core:paste (input.hidden-input)
     -0:58.7.0 core:undo (input.hidden-input)
     -0:56.6.0 settings-view:check-for-package-updates (atom-workspace.workspace.scrollbars-visible-always.theme-atom-material-syntax.theme-one-dark-ui)
     -0:27.4.0 intentions:highlight (input.hidden-input)
     -0:27.3.0 core:paste (input.hidden-input)
     -0:26.8.0 core:undo (input.hidden-input)
     -0:22 intentions:highlight (input.hidden-input)
     -0:21.9.0 core:paste (input.hidden-input)
     -0:21.2.0 core:undo (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.23 
atom-clock 0.1.7 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.9 
atom-typescript 11.0.3 
autocomplete-emojis 2.5.0 
busy-signal 1.4.0 
column-select 0.2.0 
dark-side-of-the-moon-syntax 0.6.24 
file-icons 2.1.3 
git-pear 1.2.1 
highlight-selected 0.13.1 
honor-altgraph 0.1.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.57.7 
language-markdown 0.21.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.4 
linter-eslint 8.1.6 
linter-js-standard 3.9.0 
linter-php 1.3.2 
linter-ui-default 1.2.3 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.27.1 
minimap-autohide 0.10.1 
minimap-autohider 1.4.0 
minimap-highlight-selected 4.6.1 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.1 
project-manager 3.3.4 
seti-syntax 1.1.3 
seti-ui 1.7.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.3.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.91.0 
zentabs 0.8.8 
Member

Ben3eeE commented Apr 26, 2017

  1. Scroll in minimap using mouse wheel

Atom: 1.18.0-dev-a89bd67 x64
Electron: 1.6.5
OS: Microsoft Windows 7 Professional
Thrown From: minimap package 4.27.1

Stack Trace

Uncaught TypeError: this.getTextEditorElement(...).component.onMouseWheel is not a function

At C:\Users\lineri\.atom\packages\minimap\lib\minimap-element.js:1138

TypeError: this.getTextEditorElement(...).component.onMouseWheel is not a function
    at HTMLElement.relayMousewheelEvent (/packages/minimap/lib/minimap-element.js:1138:45)
    at /packages/minimap/lib/minimap-element.js:492:16)
    at HTMLElement.listener (/packages/minimap/node_modules/atom-utils/lib/mixins/events-delegation.js:108:41)

Commands

     -1:23.8.0 intentions:highlight (input.hidden-input)
     -1:23.6.0 core:paste (input.hidden-input)
     -1:23.1.0 core:undo (input.hidden-input)
     -1:15.7.0 intentions:highlight (input.hidden-input)
     -1:15.5.0 core:paste (input.hidden-input)
     -1:15 core:undo (input.hidden-input)
     -0:59.4.0 intentions:highlight (input.hidden-input)
     -0:59.2.0 core:paste (input.hidden-input)
     -0:58.7.0 core:undo (input.hidden-input)
     -0:56.6.0 settings-view:check-for-package-updates (atom-workspace.workspace.scrollbars-visible-always.theme-atom-material-syntax.theme-one-dark-ui)
     -0:27.4.0 intentions:highlight (input.hidden-input)
     -0:27.3.0 core:paste (input.hidden-input)
     -0:26.8.0 core:undo (input.hidden-input)
     -0:22 intentions:highlight (input.hidden-input)
     -0:21.9.0 core:paste (input.hidden-input)
     -0:21.2.0 core:undo (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.23 
atom-clock 0.1.7 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.9 
atom-typescript 11.0.3 
autocomplete-emojis 2.5.0 
busy-signal 1.4.0 
column-select 0.2.0 
dark-side-of-the-moon-syntax 0.6.24 
file-icons 2.1.3 
git-pear 1.2.1 
highlight-selected 0.13.1 
honor-altgraph 0.1.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.57.7 
language-markdown 0.21.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.4 
linter-eslint 8.1.6 
linter-js-standard 3.9.0 
linter-php 1.3.2 
linter-ui-default 1.2.3 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.27.1 
minimap-autohide 0.10.1 
minimap-autohider 1.4.0 
minimap-highlight-selected 4.6.1 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.1 
project-manager 3.3.4 
seti-syntax 1.1.3 
seti-ui 1.7.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.3.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.91.0 
zentabs 0.8.8 
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Apr 26, 2017

Member
  1. Switch tabs, create new untitled tab, open settings-view and so on.

This happens sometimes not every time you do any of the above actions. Without reliable steps to reproduce I have not been able to check if it reproduces on master but I have only seen it on this branch.

Atom: 1.18.0-dev-a89bd67 x64
Electron: 1.6.5
OS: Microsoft Windows 7 Professional
Thrown From: linter-ui-default package 1.2.3

Stack Trace

Uncaught TypeError: Cannot read property 'id' of null

At C:\Users\lineri\AppData\Local\atom\app-1.18.0-dev-a89bd67\resources\app\src\text-editor-component.js:2060

TypeError: Cannot read property 'id' of null
    at TextEditorComponent.screenPositionForPixelPosition (~/AppData/Local/atom/app-1.18.0-dev-a89bd67/resources/app/src/text-editor-component.js:2060:1)
    at getBufferPositionFromMouseEvent (/packages/linter-ui-default/lib/editor/helpers.js:9:50)
    at /packages/linter-ui-default/lib/editor/index.js:143:29)
    at later (/packages/linter/node_modules/sb-debounce/index.js:9:14)

Commands

     -0:13.1.0 application:new-file (input.hidden-input)
     -0:12.7.0 core:copy (input.hidden-input)
     -0:10.1.0 intentions:highlight (input.hidden-input)
     -0:10 core:paste (input.hidden-input)
 22x -0:08.6.0 intentions:highlight (input.hidden-input)
     -0:07.5.0 core:select-all (input.hidden-input)
     -0:07.1.0 core:delete (input.hidden-input)
     -0:05.2.0 intentions:highlight (input.hidden-input)
     -0:05.1.0 core:paste (input.hidden-input)
     -0:03.0 intentions:highlight (input.hidden-input)
     -0:02.8.0 core:copy (input.hidden-input)
     -0:02.1.0 core:select-all (input.hidden-input)
     -0:01.4.0 core:delete (input.hidden-input)
     -0:01.2.0 intentions:highlight (input.hidden-input)
     -0:01.0 core:paste (input.hidden-input)
     -0:00.6.0 core:select-all (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.23 
atom-clock 0.1.7 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.10 
atom-typescript 11.0.3 
autocomplete-emojis 2.5.0 
busy-signal 1.4.1 
column-select 0.2.0 
dark-side-of-the-moon-syntax 0.6.24 
file-icons 2.1.4 
git-pear 1.2.1 
highlight-selected 0.13.1 
honor-altgraph 0.1.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.57.7 
language-markdown 0.21.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.4 
linter-eslint 8.1.6 
linter-js-standard 3.9.0 
linter-php 1.3.2 
linter-ui-default 1.2.3 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.27.1 
minimap-autohide 0.10.1 
minimap-autohider 1.4.0 
minimap-highlight-selected 4.6.1 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.1 
project-manager 3.3.4 
seti-syntax 1.1.3 
seti-ui 1.7.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.3.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.91.0 
zentabs 0.8.8 
Member

Ben3eeE commented Apr 26, 2017

  1. Switch tabs, create new untitled tab, open settings-view and so on.

This happens sometimes not every time you do any of the above actions. Without reliable steps to reproduce I have not been able to check if it reproduces on master but I have only seen it on this branch.

Atom: 1.18.0-dev-a89bd67 x64
Electron: 1.6.5
OS: Microsoft Windows 7 Professional
Thrown From: linter-ui-default package 1.2.3

Stack Trace

Uncaught TypeError: Cannot read property 'id' of null

At C:\Users\lineri\AppData\Local\atom\app-1.18.0-dev-a89bd67\resources\app\src\text-editor-component.js:2060

TypeError: Cannot read property 'id' of null
    at TextEditorComponent.screenPositionForPixelPosition (~/AppData/Local/atom/app-1.18.0-dev-a89bd67/resources/app/src/text-editor-component.js:2060:1)
    at getBufferPositionFromMouseEvent (/packages/linter-ui-default/lib/editor/helpers.js:9:50)
    at /packages/linter-ui-default/lib/editor/index.js:143:29)
    at later (/packages/linter/node_modules/sb-debounce/index.js:9:14)

Commands

     -0:13.1.0 application:new-file (input.hidden-input)
     -0:12.7.0 core:copy (input.hidden-input)
     -0:10.1.0 intentions:highlight (input.hidden-input)
     -0:10 core:paste (input.hidden-input)
 22x -0:08.6.0 intentions:highlight (input.hidden-input)
     -0:07.5.0 core:select-all (input.hidden-input)
     -0:07.1.0 core:delete (input.hidden-input)
     -0:05.2.0 intentions:highlight (input.hidden-input)
     -0:05.1.0 core:paste (input.hidden-input)
     -0:03.0 intentions:highlight (input.hidden-input)
     -0:02.8.0 core:copy (input.hidden-input)
     -0:02.1.0 core:select-all (input.hidden-input)
     -0:01.4.0 core:delete (input.hidden-input)
     -0:01.2.0 intentions:highlight (input.hidden-input)
     -0:01.0 core:paste (input.hidden-input)
     -0:00.6.0 core:select-all (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.23 
atom-clock 0.1.7 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.10 
atom-typescript 11.0.3 
autocomplete-emojis 2.5.0 
busy-signal 1.4.1 
column-select 0.2.0 
dark-side-of-the-moon-syntax 0.6.24 
file-icons 2.1.4 
git-pear 1.2.1 
highlight-selected 0.13.1 
honor-altgraph 0.1.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.57.7 
language-markdown 0.21.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.4 
linter-eslint 8.1.6 
linter-js-standard 3.9.0 
linter-php 1.3.2 
linter-ui-default 1.2.3 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.27.1 
minimap-autohide 0.10.1 
minimap-autohider 1.4.0 
minimap-highlight-selected 4.6.1 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.1 
project-manager 3.3.4 
seti-syntax 1.1.3 
seti-ui 1.7.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.3.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.91.0 
zentabs 0.8.8 
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 27, 2017

Contributor

@abe33 Are you willing to fix some of your monkey-patches in packages to work with this new editor? We're adding an API to enable you to inject styled spans into the markup, but there some other places where things just need adjustment.

Contributor

nathansobo commented Apr 27, 2017

@abe33 Are you willing to fix some of your monkey-patches in packages to work with this new editor? We're adding an API to enable you to inject styled spans into the markup, but there some other places where things just need adjustment.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo May 5, 2017

Contributor

We will probably merge this later today after running a couple of benchmarks and verifying that performance-wise everything looks good.

It's worth noting that we'll be merging this into the tj-upgrade-electron branch. That PR won't be merged to master until the next release cycle in roughly 10 days. So we're close but we'll be waiting a bit longer.

How agressive, ambitious and exciting to rewrite this fundamental part AFTER Atom have already get popularity

Thanks very much @t9md! We'll see how painful it ends up being. Hopefully we've found the majority of regressions already. 🤞

Contributor

nathansobo commented May 5, 2017

We will probably merge this later today after running a couple of benchmarks and verifying that performance-wise everything looks good.

It's worth noting that we'll be merging this into the tj-upgrade-electron branch. That PR won't be merged to master until the next release cycle in roughly 10 days. So we're close but we'll be waiting a bit longer.

How agressive, ambitious and exciting to rewrite this fundamental part AFTER Atom have already get popularity

Thanks very much @t9md! We'll see how painful it ends up being. Hopefully we've found the majority of regressions already. 🤞

@as-cii as-cii merged commit ecef2af into tj-upgrade-electron May 5, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details

@as-cii as-cii deleted the ns-editor-rendering branch May 5, 2017

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst May 6, 2017

Is there a new issue for the remaining optimization tasks that I can follow?

alexandernst commented May 6, 2017

Is there a new issue for the remaining optimization tasks that I can follow?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld May 8, 2017

Contributor

👏 Congratulations guys. Such awesome work.

Contributor

maxbrunsfeld commented May 8, 2017

👏 Congratulations guys. Such awesome work.

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Jul 3, 2017

Contributor

So... uh... sorry if it's a wrong place to ask this question, but before this PR, all custom gutter decorations had an automatic .decoration CSS class (see here). Here, this is not the case (see here and here). Is it intentional? Because it's going to lead to some broken stylesheets and maybe even code.

P.S. Created #14941 for good measure.

Contributor

lierdakil commented Jul 3, 2017

So... uh... sorry if it's a wrong place to ask this question, but before this PR, all custom gutter decorations had an automatic .decoration CSS class (see here). Here, this is not the case (see here and here). Is it intentional? Because it's going to lead to some broken stylesheets and maybe even code.

P.S. Created #14941 for good measure.

@Levertion

This comment has been minimized.

Show comment
Hide comment
@Levertion

Levertion Jul 23, 2017

(at)abe33 Are you willing to fix some of your monkey-patches in packages to work with this new editor? We're adding an API to enable you to inject styled spans into the markup, but there some other places where things just need adjustment.

Will this deal with #8669 ? - it reads like it will. Thanks if so, that would be awesome.
Sorry abe33 if this sent you a message, not sure about how github comments work

Levertion commented Jul 23, 2017

(at)abe33 Are you willing to fix some of your monkey-patches in packages to work with this new editor? We're adding an API to enable you to inject styled spans into the markup, but there some other places where things just need adjustment.

Will this deal with #8669 ? - it reads like it will. Thanks if so, that would be awesome.
Sorry abe33 if this sent you a message, not sure about how github comments work

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst Jul 23, 2017

alexandernst commented Jul 23, 2017

@Levertion

This comment has been minimized.

Show comment
Hide comment
@Levertion

Levertion Jul 23, 2017

@alexandernst

No, this PR has nothing to do with #8669, but you can follow tree-sitter,
which does.

This api sounds like you would be able to recreate the highlighting though by putting your own spans in for custom highlighting, is this correct.

EDIT2:Found the source code from the atom.io blog page in https://github.com/atom/atom/pull/14790/files#diff-561e90889eebfd1149f1b29006d738c4R1790

Levertion commented Jul 23, 2017

@alexandernst

No, this PR has nothing to do with #8669, but you can follow tree-sitter,
which does.

This api sounds like you would be able to recreate the highlighting though by putting your own spans in for custom highlighting, is this correct.

EDIT2:Found the source code from the atom.io blog page in https://github.com/atom/atom/pull/14790/files#diff-561e90889eebfd1149f1b29006d738c4R1790

@theFroh

This comment has been minimized.

Show comment
Hide comment
@theFroh

theFroh Jul 28, 2017

Not entirely certain how related it is, but linter-ui-default#355. Steps to reproduce for me are to open a couple of files which will be linted, and then close them with CTRL-W (core:close). Interestingly, using the mouse to navigate to and close the tabs via their close button does not trigger this.

theFroh commented Jul 28, 2017

Not entirely certain how related it is, but linter-ui-default#355. Steps to reproduce for me are to open a couple of files which will be linted, and then close them with CTRL-W (core:close). Interestingly, using the mouse to navigate to and close the tabs via their close button does not trigger this.

@steveoh

This comment has been minimized.

Show comment
Hide comment
@steveoh

steveoh Aug 9, 2017

would this be the pr that changed the way fonts are rendering?

steveoh commented Aug 9, 2017

would this be the pr that changed the way fonts are rendering?

@Arcanemagus

This comment has been minimized.

Show comment
Hide comment
@Arcanemagus

Arcanemagus Aug 9, 2017

Contributor

@steveoh That would be part of #12696 which updated Atom from Electron v1.3.5 (Chrome 52) to Electron v1.6.9 (Chrome 56).

Contributor

Arcanemagus commented Aug 9, 2017

@steveoh That would be part of #12696 which updated Atom from Electron v1.3.5 (Chrome 52) to Electron v1.6.9 (Chrome 56).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment