From bb5d6beffa393176c55392b07ccbb0de63bd2138 Mon Sep 17 00:00:00 2001 From: Bryan Haakman Date: Tue, 2 Apr 2019 16:34:06 +0300 Subject: [PATCH] Make all ops invertible and remove `value` from ops (#2225) --- docs/reference/slate/commands.md | 6 +- docs/reference/slate/operation.md | 10 +- examples/history/index.js | 4 +- packages/slate/src/commands/by-path.js | 49 +++++----- packages/slate/src/commands/on-selection.js | 23 +++-- packages/slate/src/commands/on-value.js | 15 +-- packages/slate/src/interfaces/element.js | 4 +- packages/slate/src/models/operation.js | 91 ++++++++++--------- packages/slate/src/models/text.js | 8 +- packages/slate/src/operations/apply.js | 22 +++-- packages/slate/src/operations/invert.js | 44 ++------- ...s => marked-text-with-some-other-marks.js} | 0 .../remove-node/decoration-across-blocks.js | 5 + 13 files changed, 135 insertions(+), 146 deletions(-) rename packages/slate/test/models/text/marks/update-mark/{marked-text-with-some-other-makrs.js => marked-text-with-some-other-marks.js} (100%) diff --git a/docs/reference/slate/commands.md b/docs/reference/slate/commands.md index 2aad7188b..b496d9725 100644 --- a/docs/reference/slate/commands.md +++ b/docs/reference/slate/commands.md @@ -472,10 +472,10 @@ Remove `length` characters of text starting at an `offset` in a [`Node`](./node. ### `setMarkByKey/Path` -`setMarkByKey(key: String, offset: Number, length: Number, mark: Mark, properties: Object) => Editor` -`setMarkByPath(path: List, offset: Number, length: Number, mark: Mark, properties: Object) => Editor` +`setMarkByKey(key: String, offset: Number, length: Number, properties: Object, newProperties: Object) => Editor` +`setMarkByPath(path: List, offset: Number, length: Number, properties: Object, newProperties: Object) => Editor` -Set a dictionary of `properties` on a [`mark`](./mark.md) on a [`Node`](./node.md) by its `key` or `path`. +Set a dictionary of `newProperties` on a [`mark`](./mark.md) on a [`Node`](./node.md) by its `key` or `path`. ### `setNodeByKey/Path` diff --git a/docs/reference/slate/operation.md b/docs/reference/slate/operation.md index cc3450bb6..6d59f8070 100644 --- a/docs/reference/slate/operation.md +++ b/docs/reference/slate/operation.md @@ -83,13 +83,13 @@ Removes a `mark` from a text node at `path` starting at an `offset` and spanning path: List, offset: Number, length: Number, - mark: Mark, properties: Object, + newProperties: Object, data: Map, } ``` -Set new `properties` on any marks that match an existing `mark` in a text node at `path`, starting at an `offset` and spanning `length` characters. +Set new `newProperties` on any marks that match an existing `properties` mark in a text node at `path`, starting at an `offset` and spanning `length` characters. ## Node Operations @@ -153,7 +153,7 @@ Remove the node at `path`. type: 'set_node', path: List, properties: Object, - node: Node, + newProperties: Object, data: Map, } ``` @@ -183,7 +183,7 @@ Split the node at `path` at `position`. The `position` refers to either the inde { type: 'set_selection', properties: Object, - selection: Selection, + newProperties: Object, data: Map, } ``` @@ -196,7 +196,7 @@ Set new `properties` on the selection. { type: 'set_value', properties: Object, - value: Value, + newProperties: Object, data: Map, } ``` diff --git a/examples/history/index.js b/examples/history/index.js index a041deead..45eaf2e85 100644 --- a/examples/history/index.js +++ b/examples/history/index.js @@ -71,8 +71,8 @@ class History extends React.Component { * @param {Editor} editor */ - onChange = ({ value }) => { - this.setState({ value }) + onChange = change => { + this.setState({ value: change.value }) } /** diff --git a/packages/slate/src/commands/by-path.js b/packages/slate/src/commands/by-path.js index 84e61a413..b9073a79c 100644 --- a/packages/slate/src/commands/by-path.js +++ b/packages/slate/src/commands/by-path.js @@ -1,3 +1,4 @@ +import pick from 'lodash/pick' import Block from '../models/block' import Inline from '../models/inline' import Mark from '../models/mark' @@ -52,7 +53,6 @@ Commands.addMarkByPath = (editor, path, offset, length, mark) => { operations.push({ type: 'add_mark', - value, path, offset: start, length: end - start, @@ -88,11 +88,8 @@ Commands.insertFragmentByPath = (editor, path, index, fragment) => { */ Commands.insertNodeByPath = (editor, path, index, node) => { - const { value } = editor - editor.applyOperation({ type: 'insert_node', - value, path: path.concat(index), node, }) @@ -137,7 +134,6 @@ Commands.insertTextByPath = (editor, path, offset, text, marks) => { editor.applyOperation({ type: 'insert_text', - value, path, offset, text, @@ -169,7 +165,6 @@ Commands.mergeNodeByPath = (editor, path) => { editor.applyOperation({ type: 'merge_node', - value, path, position, // for undos to succeed we only need the type and data because @@ -192,8 +187,6 @@ Commands.mergeNodeByPath = (editor, path) => { */ Commands.moveNodeByPath = (editor, path, newParentPath, newIndex) => { - const { value } = editor - // If the operation path and newParentPath are the same, // this should be considered a NOOP if (PathUtils.isEqual(path, newParentPath)) { @@ -208,7 +201,6 @@ Commands.moveNodeByPath = (editor, path, newParentPath, newIndex) => { editor.applyOperation({ type: 'move_node', - value, path, newPath, }) @@ -254,7 +246,6 @@ Commands.removeMarkByPath = (editor, path, offset, length, mark) => { operations.push({ type: 'remove_mark', - value, path, offset: start, length: end - start, @@ -299,7 +290,6 @@ Commands.removeNodeByPath = (editor, path) => { editor.applyOperation({ type: 'remove_node', - value, path, node, }) @@ -370,7 +360,6 @@ Commands.removeTextByPath = (editor, path, offset, length) => { removals.push({ type: 'remove_text', - value, path, offset: start, text: string, @@ -447,28 +436,35 @@ Commands.replaceTextByPath = (editor, path, offset, length, text, marks) => { } /** - * Set `properties` on mark on text at `offset` and `length` in node by `path`. + * Set `newProperties` on mark on text at `offset` and `length` in node by `path`. * * @param {Editor} editor * @param {Array} path * @param {Number} offset * @param {Number} length - * @param {Mark} mark + * @param {Object|Mark} properties + * @param {Object} newProperties */ -Commands.setMarkByPath = (editor, path, offset, length, mark, properties) => { - mark = Mark.create(mark) - properties = Mark.createProperties(properties) - const { value } = editor +Commands.setMarkByPath = ( + editor, + path, + offset, + length, + properties, + newProperties +) => { + // we call Mark.create() here because we need the complete previous mark instance + properties = Mark.create(properties) + newProperties = Mark.createProperties(newProperties) editor.applyOperation({ type: 'set_mark', - value, path, offset, length, - mark, properties, + newProperties, }) } @@ -477,21 +473,21 @@ Commands.setMarkByPath = (editor, path, offset, length, mark, properties) => { * * @param {Editor} editor * @param {Array} path - * @param {Object|String} properties + * @param {Object|String} newProperties */ -Commands.setNodeByPath = (editor, path, properties) => { - properties = Node.createProperties(properties) +Commands.setNodeByPath = (editor, path, newProperties) => { const { value } = editor const { document } = value const node = document.assertNode(path) + newProperties = Node.createProperties(newProperties) + const prevProperties = pick(node, Object.keys(newProperties)) editor.applyOperation({ type: 'set_node', - value, path, - node, - properties, + properties: prevProperties, + newProperties, }) } @@ -529,7 +525,6 @@ Commands.splitNodeByPath = (editor, path, position, options = {}) => { editor.applyOperation({ type: 'split_node', - value, path, position, target, diff --git a/packages/slate/src/commands/on-selection.js b/packages/slate/src/commands/on-selection.js index 3d2c6647b..fd1bea760 100644 --- a/packages/slate/src/commands/on-selection.js +++ b/packages/slate/src/commands/on-selection.js @@ -592,7 +592,7 @@ Commands.select = (editor, properties, options = {}) => { const { snapshot = false } = options const { value } = editor const { document, selection } = value - const props = {} + const newProperties = {} let next = selection.setProperties(properties) next = document.resolveSelection(next) @@ -604,27 +604,34 @@ Commands.select = (editor, properties, options = {}) => { // are being changed, for the inverse operation. for (const k in properties) { if (snapshot === true || !is(properties[k], selection[k])) { - props[k] = properties[k] + newProperties[k] = properties[k] } } // If the selection moves, clear any marks, unless the new selection - // properties editor the marks in some way. - if (selection.marks && !props.marks && (props.anchor || props.focus)) { - props.marks = null + // properties change the marks in some way. + if ( + selection.marks && + !newProperties.marks && + (newProperties.anchor || newProperties.focus) + ) { + newProperties.marks = null } // If there are no new properties to set, abort to avoid extra operations. - if (Object.keys(props).length === 0) { + if (Object.keys(newProperties).length === 0) { return } + // TODO: for some reason toJSON() is required here (it breaks selections between blocks)? - 2018-10-10 + const prevProperties = pick(selection.toJSON(), Object.keys(newProperties)) + editor.applyOperation( { type: 'set_selection', value, - properties: props, - selection: selection.toJSON(), + properties: prevProperties, + newProperties, }, snapshot ? { skip: false, merge: false } : {} ) diff --git a/packages/slate/src/commands/on-value.js b/packages/slate/src/commands/on-value.js index 0ee0d7fdd..5077e8177 100644 --- a/packages/slate/src/commands/on-value.js +++ b/packages/slate/src/commands/on-value.js @@ -1,3 +1,4 @@ +import pick from 'lodash/pick' import Value from '../models/value' /** @@ -16,13 +17,14 @@ const Commands = {} */ Commands.setData = (editor, data = {}) => { - const properties = Value.createProperties({ data }) const { value } = editor + const newProperties = Value.createProperties({ data }) + const prevProperties = pick(value, Object.keys(newProperties)) editor.applyOperation({ type: 'set_value', - properties, - value, + properties: prevProperties, + newProperties, }) } @@ -34,13 +36,14 @@ Commands.setData = (editor, data = {}) => { */ Commands.setDecorations = (editor, decorations = []) => { - const properties = Value.createProperties({ decorations }) const { value } = editor + const newProperties = Value.createProperties({ decorations }) + const prevProperties = pick(value, Object.keys(newProperties)) editor.applyOperation({ type: 'set_value', - properties, - value, + properties: prevProperties, + newProperties, }) } diff --git a/packages/slate/src/interfaces/element.js b/packages/slate/src/interfaces/element.js index 480280126..7ccff5e8f 100644 --- a/packages/slate/src/interfaces/element.js +++ b/packages/slate/src/interfaces/element.js @@ -2240,9 +2240,9 @@ class ElementInterface { * @return {Node} */ - setMark(path, offset, length, mark, properties) { + setMark(path, offset, length, properties, newProperties) { let node = this.assertNode(path) - node = node.updateMark(offset, length, mark, properties) + node = node.updateMark(offset, length, properties, newProperties) const ret = this.replaceNode(path, node) return ret } diff --git a/packages/slate/src/models/operation.js b/packages/slate/src/models/operation.js index 2be427322..05829a1bc 100644 --- a/packages/slate/src/models/operation.js +++ b/packages/slate/src/models/operation.js @@ -16,19 +16,19 @@ import invert from '../operations/invert' */ const OPERATION_ATTRIBUTES = { - add_mark: ['value', 'path', 'offset', 'length', 'mark', 'data'], - insert_node: ['value', 'path', 'node', 'data'], - insert_text: ['value', 'path', 'offset', 'text', 'marks', 'data'], - merge_node: ['value', 'path', 'position', 'properties', 'target', 'data'], - move_node: ['value', 'path', 'newPath', 'data'], - remove_mark: ['value', 'path', 'offset', 'length', 'mark', 'data'], - remove_node: ['value', 'path', 'node', 'data'], - remove_text: ['value', 'path', 'offset', 'text', 'marks', 'data'], - set_mark: ['value', 'path', 'offset', 'length', 'mark', 'properties', 'data'], - set_node: ['value', 'path', 'node', 'properties', 'data'], - set_selection: ['value', 'selection', 'properties', 'data'], - set_value: ['value', 'properties', 'data'], - split_node: ['value', 'path', 'position', 'properties', 'target', 'data'], + add_mark: ['path', 'offset', 'length', 'mark', 'data'], + insert_node: ['path', 'node', 'data'], + insert_text: ['path', 'offset', 'text', 'marks', 'data'], + merge_node: ['path', 'position', 'properties', 'target', 'data'], + move_node: ['path', 'newPath', 'data'], + remove_mark: ['path', 'offset', 'length', 'mark', 'data'], + remove_node: ['path', 'node', 'data'], + remove_text: ['path', 'offset', 'text', 'marks', 'data'], + set_mark: ['path', 'offset', 'length', 'properties', 'newProperties', 'data'], + set_node: ['path', 'properties', 'newProperties', 'data'], + set_selection: ['properties', 'newProperties', 'data'], + set_value: ['properties', 'newProperties', 'data'], + split_node: ['path', 'position', 'properties', 'target', 'data'], } /** @@ -47,11 +47,10 @@ const DEFAULTS = { path: undefined, position: undefined, properties: undefined, - selection: undefined, + newProperties: undefined, target: undefined, text: undefined, type: undefined, - value: undefined, data: undefined, } @@ -132,13 +131,6 @@ class Operation extends Record(DEFAULTS) { } if (v === undefined) { - // Skip keys for objects that should not be serialized, and are only used - // for providing the local-only invert behavior for the history stack. - if (key === 'document') continue - if (key === 'selection') continue - if (key === 'value') continue - if (key === 'node' && type !== 'insert_node') continue - throw new Error( `\`Operation.fromJSON\` was passed a "${type}" operation without the required "${key}" attribute.` ) @@ -160,31 +152,35 @@ class Operation extends Record(DEFAULTS) { v = Node.create(v) } - if (key === 'selection') { - v = Selection.create(v) - } - - if (key === 'value') { - v = Value.create(v) - } - if (key === 'properties' && type === 'merge_node') { v = Node.createProperties(v) } - if (key === 'properties' && type === 'set_mark') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_mark' + ) { v = Mark.createProperties(v) } - if (key === 'properties' && type === 'set_node') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_node' + ) { v = Node.createProperties(v) } - if (key === 'properties' && type === 'set_selection') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_selection' + ) { v = Selection.createProperties(v) } - if (key === 'properties' && type === 'set_value') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_value' + ) { v = Value.createProperties(v) } @@ -252,13 +248,6 @@ class Operation extends Record(DEFAULTS) { for (const key of ATTRIBUTES) { let value = this[key] - // Skip keys for objects that should not be serialized, and are only used - // for providing the local-only invert behavior for the history stack. - if (key === 'document') continue - if (key === 'selection') continue - if (key === 'value') continue - if (key === 'node' && type !== 'insert_node') continue - if ( key === 'mark' || key === 'marks' || @@ -276,21 +265,30 @@ class Operation extends Record(DEFAULTS) { value = v } - if (key === 'properties' && type === 'set_mark') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_mark' + ) { const v = {} if ('data' in value) v.data = value.data.toJS() if ('type' in value) v.type = value.type value = v } - if (key === 'properties' && type === 'set_node') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_node' + ) { const v = {} if ('data' in value) v.data = value.data.toJS() if ('type' in value) v.type = value.type value = v } - if (key === 'properties' && type === 'set_selection') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_selection' + ) { const v = {} if ('anchor' in value) v.anchor = value.anchor.toJSON() if ('focus' in value) v.focus = value.focus.toJSON() @@ -299,7 +297,10 @@ class Operation extends Record(DEFAULTS) { value = v } - if (key === 'properties' && type === 'set_value') { + if ( + (key === 'properties' || key === 'newProperties') && + type === 'set_value' + ) { const v = {} if ('data' in value) v.data = value.data.toJS() if ('decorations' in value) v.decorations = value.decorations.toJS() diff --git a/packages/slate/src/models/text.js b/packages/slate/src/models/text.js index 633d16e2f..9eecab333 100644 --- a/packages/slate/src/models/text.js +++ b/packages/slate/src/models/text.js @@ -2,6 +2,7 @@ import isPlainObject from 'is-plain-object' import warning from 'tiny-warning' import { List, OrderedSet, Record, Set } from 'immutable' +import Mark from './mark' import Leaf from './leaf' import KeyUtils from '../utils/key-utils' import memoize from '../utils/memoize' @@ -572,13 +573,14 @@ class Text extends Record(DEFAULTS) { * * @param {Number} index * @param {Number} length - * @param {Mark} mark * @param {Object} properties + * @param {Object} newProperties * @return {Text} */ - updateMark(index, length, mark, properties) { - const newMark = mark.merge(properties) + updateMark(index, length, properties, newProperties) { + const mark = Mark.create(properties) + const newMark = mark.merge(newProperties) if (this.text === '' && length === 0 && index === 0) { const { leaves } = this diff --git a/packages/slate/src/operations/apply.js b/packages/slate/src/operations/apply.js index 9a93abf75..d8ef964cc 100644 --- a/packages/slate/src/operations/apply.js +++ b/packages/slate/src/operations/apply.js @@ -79,26 +79,32 @@ function applyOperation(value, op) { } case 'set_mark': { - const { path, offset, length, mark, properties } = op - const next = value.setMark(path, offset, length, mark, properties) + const { path, offset, length, properties, newProperties } = op + const next = value.setMark( + path, + offset, + length, + properties, + newProperties + ) return next } case 'set_node': { - const { path, properties } = op - const next = value.setNode(path, properties) + const { path, newProperties } = op + const next = value.setNode(path, newProperties) return next } case 'set_selection': { - const { properties } = op - const next = value.setSelection(properties) + const { newProperties } = op + const next = value.setSelection(newProperties) return next } case 'set_value': { - const { properties } = op - const next = value.setProperties(properties) + const { newProperties } = op + const next = value.setProperties(newProperties) return next } diff --git a/packages/slate/src/operations/invert.js b/packages/slate/src/operations/invert.js index c530ad101..9f7e5467e 100644 --- a/packages/slate/src/operations/invert.js +++ b/packages/slate/src/operations/invert.js @@ -1,5 +1,4 @@ import Debug from 'debug' -import pick from 'lodash/pick' import Operation from '../models/operation' import PathUtils from '../utils/path-utils' @@ -74,13 +73,14 @@ function invertOperation(op) { return inverse } - case 'set_node': { - const { properties, node } = op - const inverseNode = node.merge(properties) - const inverseProperties = pick(node, Object.keys(properties)) + case 'set_node': + case 'set_value': + case 'set_selection': + case 'set_mark': { + const { properties, newProperties } = op const inverse = op - .set('node', inverseNode) - .set('properties', inverseProperties) + .set('properties', newProperties) + .set('newProperties', properties) return inverse } @@ -104,36 +104,6 @@ function invertOperation(op) { return inverse } - case 'set_mark': { - const { properties, mark } = op - const inverseMark = mark.merge(properties) - const inverseProperties = pick(mark, Object.keys(properties)) - const inverse = op - .set('mark', inverseMark) - .set('properties', inverseProperties) - return inverse - } - - case 'set_selection': { - const { properties, selection } = op - const inverseSelection = selection.merge(properties) - const inverseProps = pick(selection, Object.keys(properties)) - const inverse = op - .set('selection', inverseSelection) - .set('properties', inverseProps) - return inverse - } - - case 'set_value': { - const { properties, value } = op - const inverseValue = value.merge(properties) - const inverseProperties = pick(value, Object.keys(properties)) - const inverse = op - .set('value', inverseValue) - .set('properties', inverseProperties) - return inverse - } - default: { throw new Error(`Unknown operation type: "${type}".`) } diff --git a/packages/slate/test/models/text/marks/update-mark/marked-text-with-some-other-makrs.js b/packages/slate/test/models/text/marks/update-mark/marked-text-with-some-other-marks.js similarity index 100% rename from packages/slate/test/models/text/marks/update-mark/marked-text-with-some-other-makrs.js rename to packages/slate/test/models/text/marks/update-mark/marked-text-with-some-other-marks.js diff --git a/packages/slate/test/operations/apply/remove-node/decoration-across-blocks.js b/packages/slate/test/operations/apply/remove-node/decoration-across-blocks.js index afaa86306..20a70f83d 100644 --- a/packages/slate/test/operations/apply/remove-node/decoration-across-blocks.js +++ b/packages/slate/test/operations/apply/remove-node/decoration-across-blocks.js @@ -6,6 +6,11 @@ export default [ { type: 'remove_node', path: [0], + node: ( + + one + + ), }, ]