This PR changes our approach to text editor rendering by centralizing all state in a TextEditorPresenter object that sits between the view and the model.
The presenter manages a mutable state object that is synchronously updated whenever there are changes from the model or changes to measurements from the view. The view consumes this state object when updating the DOM asynchronously so we don't need to recompute it before every render like we do now.
This reconfiguration paves the way to perform all DOM updates manually rather than relying on React's virtual DOM reconciliation system. The view will maintain a copy of the presenter state, and it will perform a comparison of the presenter's state against the copy when updating the DOM.
The net result will be a design very similar to React's reconciliation mechanism, but my hope is that a lower-level implementation targeting our specific problem domain will make this design easier to profile and open up specific optimizations that wouldn't be possible at a higher level of abstraction. We already perform DOM updates to lines and line numbers manually to optimize around reconciliation overhead; this would just complete that process.
Remaining work
Optimizations
Perform a single decorations query when the visible row range changes to cache the visible decorations, then read decoration classes from that when building lines, line numbers, etc rather than querying multiple times on every change.
When decorations are added / removed, update only the decoration classes on lines and line numbers rather than doing a full pass on their content.
Cache foldable status for each line in the tokenized buffer instead of recomputing it every time we update the line numbers.
Fixes
Fix velocity scrolling upward
Fix TextEditorComponent specs
Compute row id correctly when the first visible line is soft-wrapped.
Eliminate TextEditorPresenter::getHeight
Audit handling of scrollTop/scrollLeft
Enforce maximum scrollTop/Left based on height/width and content. This should hopefully address some flakiness in the random test.
Editor losing focus when switching grammars with grammar switcher. Does that happen on master too?
Gutter background color isn't changing when themes get changed.
This PR changes our approach to text editor rendering by centralizing all
state in a TextEditorPresenter object that sits between the view and the
model.
The presenter manages a mutable state object that is synchronously updated
whenever there are changes from the model or changes to measurements from
the view. The view consumes this state object when updating the DOM
asynchronously so we don't need to recompute it before every render like we
do now.
This reconfiguration paves the way to perform all DOM updates manually
rather than relying on React's virtual DOM reconciliation system. The view
will maintain a copy of the presenter state, and it will perform a
comparison of the presenter's state against the copy when updating the DOM.
The net result will be a design very similar to React's reconciliation
mechanism, but my hope is that a lower-level implementation targeting our
specific problem domain will make this design easier to profile and open up
specific optimizations that wouldn't be possible at a higher level of
abstraction. We already perform DOM updates to lines and line numbers
manually to optimize around reconciliation overhead; this would just
complete that process.
You can view, comment on, or merge this pull request online at:
@mark-hahn Yeah it shouldn't interfere. Although one thing I'm hoping to solve is to provide a hook for appropriate times to modify the DOM yourself to prevent reflows. That's fairly fuzzy at the moment though and a ways off.
@mark-hahnhttps://github.com/mark-hahn Yeah it shouldn't interfere.
Although one thing I'm hoping to solve is to provide a hook for appropriate times to modify the DOM yourself to prevent reflows. That's fairly
fuzzy at the moment though and a ways off.
—
Reply to this email directly or view it on GitHub #5293 (comment).
The basic structure is in place here, so now it's time to optimize. It breaks down to about 6ms updating the model and presenter state and 4ms updating the DOM for a single character insert. I have reason to believe I can improve on this substantially.
There's a ton of low hanging fruit on the presenter update because I did everything in the most naive way, so that's the work I'll do on this PR and save DOM update optimizations for a subsequent PR. To ensure optimizations don't introduce regressions, I'm going to build a spec that randomly mutates an editor and compares the state of a brand new presenter to that of a preexisting presenter to be sure updates were performed correctly.
After that, some of the presenter optimizations I have in mind:
Perform a single decorations query when the visible row range changes to cache the visible decorations, then read decoration classes from that when building lines, line numbers, etc rather than querying multiple times on every change.
When decorations are added / removed, update only the decoration classes on lines and line numbers rather than doing a full pass on their content.
Cache foldability status on tokenized lines so we don't have to recompute it when updating line numbers.
I have more ideas, but I'd like to try these things first and re-evaluate.
I also want to re-introduce shouldComponentUpdate methods on all the React components which should improve reconciliation performance.
This is based on the ::baseCharacterWidth property for now. To be fully
correct, we need to base the scrollWidth on the actual width of
individual characters.
Removed shouldComponentUpdate because we will always update the
component manually once this is done, but I don’t want to accidentally
prevent the component from updating during the conversion process.
This commit has a failing spec due to the presenter not accounting for
individual character widths.
It contains the .scrollWidth and then all the lines in a nested .lines
object. The .width has been removed from each line and replaced with
.content.scrollWidth.
This isn’t a super efficient approach, but it is simple and should be
correct. Once we move all state to the presenter we can perform a more
efficient synchronous update when markers change.
Forgot to rename it when we renamed decorations of type ‘gutter’ to
‘line-number’. It’s not used in core or any packages at the time of
this commit.
Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
We’re still doing some sync DOM reads and computation in the view that
should eventually be made async and moved into the presenter, but I’m
going to leave it alone for now.
This allows us to use the presenter for all stages of the component
lifecycle rather than needing to wait until it is created.
Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
The target of mousewheel events needs to be preserved when scrolling.
It used to be dealt with in the view, but now we can do it in the
presenter for a simpler view implementation.
This means we have some duplicated values in different parts of the
tree, but it’s cleaner in the view since each component only consumes
a single object. Seems like the presenter should convey the correct
data to the correct locations and minimize the logic in the view. A
few duplicated integers is a reasonable trade-off.
This test performs random operations on the editor and assigns random
measurements from the view. After each operation, the state of a
pre-existing presenter is compared with that of a new presenter created
with the same parameters.
Since it’s easier to reason about building fresh state than it is to
reason about state updates, I hope this will catch any bugs in our
update logic as we optimize it and explore every corner case.
When there were lots of off-screen markers, we were performing lots of
redundant updates when off-screen markers changed. Now we only perform
updates if they intersect the visible row range.
@maxbrunsfeld this should improve the situation for folding/unfolding
when there are lots of others folds. Let me know.
This commit also adds to the list of required measurements and updates
the spec with a buildPresenter helper to more easily supply default
values for required measurements in each spec when they aren’t relevant
to that spec’s content.
A wider content frame can mean the horizontal scrollbar gets hidden,
which could in turn mean we need to adjust the scrollTop because the
clientHeight changed.
I'm going to go ahead and cache foldable in the tokenized buffer before merging since we're recomputing the gutter more frequently with this design. I'm thinking that's the last step on this unless any other performance issues crop up.
Finished caching "foldable" status on tokenized lines. The sync presenter update for a character entry is down to about 3.4ms, and only about 1ms is spent in the presenter. Still room for optimization there, but I think we have other 🐟 to 🥚 before we worry about it. Want to spend a day using this in its final form before merging.
Previously, we were delaying handling, but still requesting redundant
handling by requesting multiple frames. Now we don’t request a new
update if there is already one outstanding.
nathansobo
merged commit 
nathansobo commentedJan 28, 2015
This PR changes our approach to text editor rendering by centralizing all state in a
TextEditorPresenterobject that sits between the view and the model.The presenter manages a mutable state object that is synchronously updated whenever there are changes from the model or changes to measurements from the view. The view consumes this state object when updating the DOM asynchronously so we don't need to recompute it before every render like we do now.
This reconfiguration paves the way to perform all DOM updates manually rather than relying on React's virtual DOM reconciliation system. The view will maintain a copy of the presenter state, and it will perform a comparison of the presenter's state against the copy when updating the DOM.
The net result will be a design very similar to React's reconciliation mechanism, but my hope is that a lower-level implementation targeting our specific problem domain will make this design easier to profile and open up specific optimizations that wouldn't be possible at a higher level of abstraction. We already perform DOM updates to lines and line numbers manually to optimize around reconciliation overhead; this would just complete that process.
Remaining work
Optimizations
foldablestatus for each line in the tokenized buffer instead of recomputing it every time we update the line numbers.Fixes
TextEditorPresenter::getHeight