Slate: introduce a `Point` model

[ianstormtaylor]

Do you want to request a _feature_ or report a _bug_?

Improvement.

What's the current behavior?

Right now ranges have properties that represent their two points:

anchorOffset
focusKey
focusOffset

This is alright, but this idea of a "point" is only inferred. There are already places in the codebase that refer to "points" informally, like in the findPoint utility. And we've thought about adding even more of them.

What's the new behavior?

We'd introduce a new model called a Point:

Point {
  key
  offset
}

And ranges would just have two points, range.anchor and range.focus.

This would standardize the data structure, making it easier to use in some cases since we can provide the utility functions on the Point model directly, and make it more of a known entity for userland code to build with.

[zhujinxuan]

@ianstormtaylor Each time we use a point is used temporally. If we make a point a class with immutable-js, then it will slows down the creation of Point. (though it will not be the bottleneck of performance)

Because we do not have functions for Point, perhaps we can make point only a plain object as {offset, key}, implementing getAnchorPoint, getFocusPoint into Range, getPointByOffset into node, and do not cache point functions. Do you think that a good idea?

[ianstormtaylor]

@zhujinxuan I think I’d like to see it actually become a performance issue before making that change, since having the data models be mixed between immutable and mutable is not ideal.

[zhujinxuan]

@ianstormtaylor Provided in https://github.com/ianstormtaylor/slate/pull/1912

[ianstormtaylor]

Hey @zhujinxuan thanks for trying that. It looks like #1912 doesn't actually implement the Point model in the core use cases of Slate, and instead just provides it as an add-on. That's not really what I was after.

Instead, the Range object should implement it's own anchor/focus with the point:

Range {
  anchor: Point,
  focus: Point,
  isFocused: Boolean,
  isBackward: Boolean,
}

Point {
  key: String,
  offset: Number,
}

And then accessing it would be done via: range.anchor.key instead of the current range.anchorKey, etc. This way points are used as a core concept all throughout the codebase.

[ianstormtaylor]

Similarly, it allows for simplifications. For example, right now "collapsing to anchor" requires doing something like:

const next = range
  .set('focusKey', range.anchorKey)
  .set('focusOffset', range.anchorOffset)
  .set('isBackward', false)

Whereas it can be simplified to:

const next = range
  .set('focus', range.anchor)
  .set('isBackward', false)

This is the kind of improvement I'm after by making Point a core model.

[zhujinxuan]

@ianstormtaylor Great! We want the Point model as the base of range model and decoration model. I will make it done today.

BTW, we may not be able to use focus as the key, because Range already has the focus function.

[ianstormtaylor]

@zhujinxuan ah good point. I think I'd like to deprecate the .focus()/.blur() methods (on Range, not on Change) and rename them to something like .setIsFocused(boolean) instead. Or even just remove them in favor of using .set('isFocused', true) from Immutable.js honestly.

(This follows with a separate idea I have that ranges themselves shouldn't even be where the "focused" state lives, since they are lower level than that, but that's a discussion for another time.)

[zhujinxuan]

@ianstormtaylor I will use anchorPoint and focusPoint as the first step. If they are working, then I will try to rename them and move range.focus to value in the same PR.

[ianstormtaylor]

@zhujinxuan sounds good! Although we'll probably need to do it in two separate PRs, with deprecation in the middle, otherwise it's a breaking change without any deprecation notice which is much harder to upgrade through.

[zhujinxuan]

@ianstormtaylor I have just finished it in #1912. I agree we shall remove focus in another PR in future.

I rename range.merge to range.loadProps, because I do not want to mess up with the immutable-js's default merge method. (though we can decorate merge to keep the same interface, personally I think the decoration makes code difficult to maintain)