refactor decorations to use selections #1221

ianstormtaylor · 2017-10-12T20:59:43Z

This changes how decorations are applied in Slate.

Previously, you would use a decorate(text, block) function in the schema. It would be called for each text node in the block, and you would add the marks to the characters yourself. But this led to issues because there are cases (like multi-line comments in syntax highlighting) where the decoration applies across multiple text nodes, and this system wasn't really built to handle that well.

Now, you use a decorate(block) function in the schema. It is no longer called for each of the text nodes. Instead of applying the new marks to the characters yourself, you return an array of selection objects, with associated marks. For example:

{
  match: (object) => {
    return object.type == 'code_block',
  },
  decorate: (block) => {
    const text = block.getFirstText()

    return [{
      anchorKey: text.key,
      anchorOffset: 0,
      focusKey: text.key,
      focusOffset: 1,
      marks: [{ type: 'bold' }]
    }]
  }
}

This would turn add a bold mark to the first character of the matched blocks.

This would also make implementing things like multiple-cursor rendering easier, since the "decorations" now map exactly to the existing selections/ranges logic.

cc @Soreine, how does this sound? I think it should solve #893 and GitbookIO/slate-prism#5.

lxcid · 2017-10-13T04:18:56Z

This is awesome!

I do have question about does it improve dynamic decorator? When force rerender based on state not directly related to the editor (e.g. search result highlighting)

ianstormtaylor · 2017-10-13T17:10:03Z

Hey @lxcid good question!

This PR itself doesn't support that, because the decorate logic in the schema is still based on node in the document themselves.

But! It does get us closer to being able to do things like that. I was thinking of adding a state.decorations property and a change.setDecorations(decorations) method that would allow you do to that. That way you could change the decorations in the editor as the outside information changes (eg. someone performs a search) and then things will re-render.

I haven't tested the performance of doing that for large editors, since you'll be re-decorating the entire document at once, but I'm sure there are things we can do to improve it.

Soreine · 2017-10-13T08:22:28Z

packages/slate-react/src/components/text.js

+    const decs = decorations.filter((d) => {
+      const { startKey, endKey } = d
+      if (startKey == key || endKey == key) return true
+      const startsBefore = document.areDescendantsSorted(startKey, key)


These calls can become quite expensive. But for a classic use of decorators, the decorations should be scoped to a few blocks. And if need be, there are ways to improve the performance of areDescendantsSorted, or maybe to process decorations upfront to get a list of individual text ranges instead (where startKey === endKey).
We don't want premature optimization anyway.

I agree though, this can be improved.

One improvement I think would be a method that checks (before, key, after) or something about checking whether a node "overlaps" a range. Because here we have to do two separate checks which each iterate keys, etc. when really it could be handled in one go.

Soreine · 2017-10-13T08:25:39Z

packages/slate-react/src/components/text.js

@@ -81,6 +73,9 @@ class Text extends React.Component {
      if (p.node == pLast && n.node != nLast) return true
    }

+    // Re-render if the current decorations have changed.
+    if (!n.decorations.equals(p.decorations)) return true


Having reified decorations allows to run checks like these, which is great. It will probably fix a lot of "decorations not updating" issues.

Soreine · 2017-10-13T17:25:22Z

packages/slate/src/models/node.js

@@ -165,19 +165,12 @@ class Node {
    first = normalizeKey(first)
    second = normalizeKey(second)

-    let sorted
+    const keys = this.getKeysAsArray()


Oh, here is the optimization :D

Soreine · 2017-10-13T17:27:08Z

packages/slate/src/models/schema.js

+      if (!rule.decorate) return
+      if (!rule.match(object)) return
+
+      const decorations = rule.decorate(object)


So the decorate calls themselves now live under a memoized function, which is nice :)

ianstormtaylor · 2017-10-13T17:45:06Z

packages/slate-react/src/plugins/core.js

-    const a = getPoint(native.anchorNode, native.anchorOffset, state, editor)
-    const f = getPoint(native.focusNode, native.focusOffset, state, editor)
+    const a = findPoint(native.anchorNode, native.anchorOffset, state)
+    const f = findPoint(native.focusNode, native.focusOffset, state)


Actually any place where we are doing findPoint for both anchor and focus can be slightly improved by checking if collapsed and if so only doing it once. But I'm not sure this is a bottleneck, so leaving for later.

ianstormtaylor · 2017-10-13T17:46:44Z

packages/slate/src/models/node.js

   */

-  getKeys() {
+  getKeysAsArray() {


I think this method can also be improved in performance if it was recursive, such that each nested child's keys would be cached, instead of having to iterate the entire tree each time the document changes.

ianstormtaylor · 2017-10-13T19:01:28Z

Actually @lxcid I've just implemented that behavior that you're asking about! I added state.decorations, which can be set based on any outside information. Along with a simple search-highlighting example to show you how it works.

lxcid · 2017-10-16T05:36:20Z

Wow! Thank for considering my request @ianstormtaylor and getting it in so soon! I can't wait to use it soon! The demo looks great!

* refactor decorations to use selections * update docs * cleanup * add Selection.createList * fix tests * fix for nested blocks * fix lint * actually merge * revert small change * add state.decorations, with search example

ianstormtaylor added 3 commits October 12, 2017 13:31

refactor decorations to use selections

19877d4

update docs

58bb930

cleanup

508e065

ianstormtaylor added improvement ✶ breaking labels Oct 12, 2017

add Selection.createList

0bb4ab0

ianstormtaylor requested a review from Soreine October 12, 2017 22:15

ianstormtaylor added 4 commits October 12, 2017 15:19

fix tests

cb13046

fix for nested blocks

0d5b3cc

fix lint

a64b98e

Merge branch 'master' into refactor-decorators

5060074

Soreine approved these changes Oct 13, 2017

View reviewed changes

ianstormtaylor commented Oct 13, 2017

View reviewed changes

ianstormtaylor added 2 commits October 13, 2017 10:57

merge master

02a7b63

actually merge

3625665

ianstormtaylor force-pushed the refactor-decorators branch from 4a9108c to 3625665 Compare October 13, 2017 18:01

ianstormtaylor added 2 commits October 13, 2017 11:02

revert small change

4717e14

add state.decorations, with search example

d5a76d6

ianstormtaylor merged commit e53cee3 into master Oct 13, 2017

ianstormtaylor deleted the refactor-decorators branch October 26, 2017 00:34

jasonphillips mentioned this pull request Apr 20, 2018

Decorations are slow #1788

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor decorations to use selections #1221

refactor decorations to use selections #1221

ianstormtaylor commented Oct 12, 2017

lxcid commented Oct 13, 2017

ianstormtaylor commented Oct 13, 2017

Soreine Oct 13, 2017

ianstormtaylor Oct 13, 2017

Soreine Oct 13, 2017

Soreine Oct 13, 2017

Soreine Oct 13, 2017

ianstormtaylor Oct 13, 2017 •

edited

Loading

ianstormtaylor Oct 13, 2017

ianstormtaylor commented Oct 13, 2017

lxcid commented Oct 16, 2017

refactor decorations to use selections #1221

refactor decorations to use selections #1221

Conversation

ianstormtaylor commented Oct 12, 2017

lxcid commented Oct 13, 2017

ianstormtaylor commented Oct 13, 2017

Soreine Oct 13, 2017

Choose a reason for hiding this comment

ianstormtaylor Oct 13, 2017

Choose a reason for hiding this comment

Soreine Oct 13, 2017

Choose a reason for hiding this comment

Soreine Oct 13, 2017

Choose a reason for hiding this comment

Soreine Oct 13, 2017

Choose a reason for hiding this comment

ianstormtaylor Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

ianstormtaylor Oct 13, 2017

Choose a reason for hiding this comment

ianstormtaylor commented Oct 13, 2017

lxcid commented Oct 16, 2017

ianstormtaylor Oct 13, 2017 •

edited

Loading