From d799b7e424d54a52bc91d83ab17e06dead767964 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 20 May 2018 16:30:39 +0100 Subject: [PATCH] refactor roundtripping into a single place and fix isRichTextEnabled to be correctly camelCased everywhere... --- src/ComposerHistoryManager.js | 34 +---- src/components/views/rooms/MessageComposer.js | 8 +- .../views/rooms/MessageComposerInput.js | 129 ++++++++++-------- .../views/rooms/MessageComposerInput-test.js | 2 +- 4 files changed, 84 insertions(+), 89 deletions(-) diff --git a/src/ComposerHistoryManager.js b/src/ComposerHistoryManager.js index 28749ace15..f997e1d1cd 100644 --- a/src/ComposerHistoryManager.js +++ b/src/ComposerHistoryManager.js @@ -28,8 +28,8 @@ type MessageFormat = 'rich' | 'markdown'; class HistoryItem { - // Keeping message for backwards-compatibility - message: string; + // We store history items in their native format to ensure history is accurate + // and then convert them if our RTE has subsequently changed format. value: Value; format: MessageFormat = 'rich'; @@ -51,32 +51,6 @@ class HistoryItem { format: this.format }; } - - // FIXME: rather than supporting storing history in either format, why don't we pick - // one canonical form? - toValue(outputFormat: MessageFormat): Value { - if (outputFormat === 'markdown') { - if (this.format === 'rich') { - // convert a rich formatted history entry to its MD equivalent - return Plain.deserialize(Md.serialize(this.value)); - // return ContentState.createFromText(RichText.stateToMarkdown(contentState)); - } - else if (this.format === 'markdown') { - return this.value; - } - } else if (outputFormat === 'rich') { - if (this.format === 'markdown') { - // convert MD formatted string to its rich equivalent. - return Md.deserialize(Plain.serialize(this.value)); - // return RichText.htmlToContentState(new Markdown(contentState.getPlainText()).toHTML()); - } - else if (this.format === 'rich') { - return this.value; - } - } - console.error("unknown format -> outputFormat conversion"); - return this.value; - } } export default class ComposerHistoryManager { @@ -110,9 +84,9 @@ export default class ComposerHistoryManager { sessionStorage.setItem(`${this.prefix}[${this.lastIndex++}]`, JSON.stringify(item.toJSON())); } - getItem(offset: number, format: MessageFormat): ?Value { + getItem(offset: number): ?HistoryItem { this.currentIndex = _clamp(this.currentIndex + offset, 0, this.lastIndex - 1); const item = this.history[this.currentIndex]; - return item ? item.toValue(format) : null; + return item; } } diff --git a/src/components/views/rooms/MessageComposer.js b/src/components/views/rooms/MessageComposer.js index 9aaa33f0fa..157dc9e704 100644 --- a/src/components/views/rooms/MessageComposer.js +++ b/src/components/views/rooms/MessageComposer.js @@ -46,7 +46,7 @@ export default class MessageComposer extends React.Component { inputState: { marks: [], blockType: null, - isRichtextEnabled: SettingsStore.getValue('MessageComposerInput.isRichTextEnabled'), + isRichTextEnabled: SettingsStore.getValue('MessageComposerInput.isRichTextEnabled'), }, showFormatting: SettingsStore.getValue('MessageComposer.showFormatting'), isQuoting: Boolean(RoomViewStore.getQuotingEvent()), @@ -227,7 +227,7 @@ export default class MessageComposer extends React.Component { onToggleMarkdownClicked(e) { e.preventDefault(); // don't steal focus from the editor! - this.messageComposerInput.enableRichtext(!this.state.inputState.isRichtextEnabled); + this.messageComposerInput.enableRichtext(!this.state.inputState.isRichTextEnabled); } render() { @@ -380,10 +380,10 @@ export default class MessageComposer extends React.Component {
{ formatButtons }
- + src={`img/button-md-${!this.state.inputState.isRichTextEnabled}.png`} /> ${body}`); - if (!this.state.isRichtextEnabled) { + if (!this.state.isRichTextEnabled) { content = ContentState.createFromText(RichText.stateToMarkdown(content)); } @@ -374,7 +374,7 @@ export default class MessageComposerInput extends React.Component { startSelection, blockMap); startSelection = SelectionState.createEmpty(contentState.getFirstBlock().getKey()); - if (this.state.isRichtextEnabled) { + if (this.state.isRichTextEnabled) { contentState = Modifier.setBlockType(contentState, startSelection, 'blockquote'); } let editorState = EditorState.push(this.state.editorState, contentState, 'insert-characters'); @@ -582,52 +582,61 @@ export default class MessageComposerInput extends React.Component { }); }; - enableRichtext(enabled: boolean) { - if (enabled === this.state.isRichtextEnabled) return; + mdToRichEditorState(editorState: Value): Value { + // for consistency when roundtripping, we could use slate-md-serializer rather than + // commonmark, but then we would lose pills as the MD deserialiser doesn't know about + // them and doesn't have any extensibility hooks. + // + // The code looks like this: + // + // const markdown = this.plainWithMdPills.serialize(editorState); + // + // // weirdly, the Md serializer can't deserialize '' to a valid Value... + // if (markdown !== '') { + // editorState = this.md.deserialize(markdown); + // } + // else { + // editorState = Plain.deserialize('', { defaultBlock: DEFAULT_NODE }); + // } - // FIXME: this duplicates similar conversions which happen in the history & store. - // they should be factored out. + // so, instead, we use commonmark proper (which is arguably more logical to the user + // anyway, as they'll expect the RTE view to match what they'll see in the timeline, + // but the HTML->MD conversion is anyone's guess). + + const textWithMdPills = this.plainWithMdPills.serialize(editorState); + const markdown = new Markdown(textWithMdPills); + // HTML deserialize has custom rules to turn matrix.to links into pill objects. + return this.html.deserialize(markdown.toHTML()); + } + + richToMdEditorState(editorState: Value): Value { + // FIXME: this conversion loses pills (turning them into pure MD links). + // We need to add a pill-aware deserialize method + // to PlainWithPillsSerializer which recognises pills in raw MD and turns them into pills. + return Plain.deserialize( + // FIXME: we compile the MD out of the RTE state using slate-md-serializer + // which doesn't roundtrip symmetrically with commonmark, which we use for + // compiling MD out of the MD editor state above. + this.md.serialize(editorState), + { defaultBlock: DEFAULT_NODE } + ); + } + + enableRichtext(enabled: boolean) { + if (enabled === this.state.isRichTextEnabled) return; let editorState = null; if (enabled) { - // for consistency when roundtripping, we could use slate-md-serializer rather than - // commonmark, but then we would lose pills as the MD deserialiser doesn't know about - // them and doesn't have any extensibility hooks. - // - // The code looks like this: - // - // const markdown = this.plainWithMdPills.serialize(this.state.editorState); - // - // // weirdly, the Md serializer can't deserialize '' to a valid Value... - // if (markdown !== '') { - // editorState = this.md.deserialize(markdown); - // } - // else { - // editorState = Plain.deserialize('', { defaultBlock: DEFAULT_NODE }); - // } - - // so, instead, we use commonmark proper (which is arguably more logical to the user - // anyway, as they'll expect the RTE view to match what they'll see in the timeline, - // but the HTML->MD conversion is anyone's guess). - - const sourceWithPills = this.plainWithMdPills.serialize(this.state.editorState); - const markdown = new Markdown(sourceWithPills); - editorState = this.html.deserialize(markdown.toHTML()); + editorState = this.mdToRichEditorState(this.state.editorState); } else { - // let markdown = RichText.stateToMarkdown(this.state.editorState.getCurrentContent()); - // value = ContentState.createFromText(markdown); - - editorState = Plain.deserialize( - this.md.serialize(this.state.editorState), - { defaultBlock: DEFAULT_NODE } - ); + editorState = this.richToMdEditorState(this.state.editorState); } Analytics.setRichtextMode(enabled); this.setState({ editorState: this.createEditorState(enabled, editorState), - isRichtextEnabled: enabled, + isRichTextEnabled: enabled, }, ()=>{ this.refs.editor.focus(); }); @@ -710,7 +719,7 @@ export default class MessageComposerInput extends React.Component { }; onBackspace = (ev: Event, change: Change): Change => { - if (this.state.isRichtextEnabled) { + if (this.state.isRichTextEnabled) { // let backspace exit lists const isList = this.hasBlock('list-item'); const { editorState } = this.state; @@ -740,14 +749,14 @@ export default class MessageComposerInput extends React.Component { handleKeyCommand = (command: string): boolean => { if (command === 'toggle-mode') { - this.enableRichtext(!this.state.isRichtextEnabled); + this.enableRichtext(!this.state.isRichTextEnabled); return true; } let newState: ?Value = null; // Draft handles rich text mode commands by default but we need to do it ourselves for Markdown. - if (this.state.isRichtextEnabled) { + if (this.state.isRichTextEnabled) { const type = command; const { editorState } = this.state; const change = editorState.change(); @@ -913,7 +922,7 @@ export default class MessageComposerInput extends React.Component { // FIXME: https://github.com/ianstormtaylor/slate/issues/1497 means // that we will silently discard nested blocks (e.g. nested lists) :( const fragment = this.html.deserialize(transfer.html); - if (this.state.isRichtextEnabled) { + if (this.state.isRichTextEnabled) { return change.insertFragment(fragment.document); } else { @@ -954,7 +963,7 @@ export default class MessageComposerInput extends React.Component { if (cmd) { if (!cmd.error) { - this.historyManager.save(editorState, this.state.isRichtextEnabled ? 'rich' : 'markdown'); + this.historyManager.save(editorState, this.state.isRichTextEnabled ? 'rich' : 'markdown'); this.setState({ editorState: this.createEditorState(), }); @@ -986,7 +995,7 @@ export default class MessageComposerInput extends React.Component { const replyingToEv = RoomViewStore.getQuotingEvent(); const mustSendHTML = Boolean(replyingToEv); - if (this.state.isRichtextEnabled) { + if (this.state.isRichTextEnabled) { // We should only send HTML if any block is styled or contains inline style let shouldSendHTML = false; @@ -1032,7 +1041,7 @@ export default class MessageComposerInput extends React.Component { this.historyManager.save( editorState, - this.state.isRichtextEnabled ? 'rich' : 'markdown', + this.state.isRichTextEnabled ? 'rich' : 'markdown', ); if (commandText && commandText.startsWith('/me')) { @@ -1119,7 +1128,7 @@ export default class MessageComposerInput extends React.Component { if (up) { const scrollCorrection = editorNode.scrollTop; const distanceFromTop = cursorRect.top - editorRect.top + scrollCorrection; - console.log(`Cursor distance from editor top is ${distanceFromTop}`); + //console.log(`Cursor distance from editor top is ${distanceFromTop}`); if (distanceFromTop < EDGE_THRESHOLD) { navigateHistory = true; } @@ -1128,7 +1137,7 @@ export default class MessageComposerInput extends React.Component { const scrollCorrection = editorNode.scrollHeight - editorNode.clientHeight - editorNode.scrollTop; const distanceFromBottom = editorRect.bottom - cursorRect.bottom + scrollCorrection; - console.log(`Cursor distance from editor bottom is ${distanceFromBottom}`); + //console.log(`Cursor distance from editor bottom is ${distanceFromBottom}`); if (distanceFromBottom < EDGE_THRESHOLD) { navigateHistory = true; } @@ -1168,7 +1177,19 @@ export default class MessageComposerInput extends React.Component { return; } - let editorState = this.historyManager.getItem(delta, this.state.isRichtextEnabled ? 'rich' : 'markdown'); + let editorState; + const historyItem = this.historyManager.getItem(delta); + if (historyItem) { + if (historyItem.format === 'rich' && !this.state.isRichTextEnabled) { + editorState = this.richToMdEditorState(historyItem.value); + } + else if (historyItem.format === 'markdown' && this.state.isRichTextEnabled) { + editorState = this.mdToRichEditorState(historyItem.value); + } + else { + editorState = historyItem.value; + } + } // Move selection to the end of the selected history const change = editorState.change().collapseToEndOf(editorState.document); @@ -1468,8 +1489,8 @@ export default class MessageComposerInput extends React.Component {
+ title={this.state.isRichTextEnabled ? _t("Markdown is disabled") : _t("Markdown is enabled")} + src={`img/button-md-${!this.state.isRichTextEnabled}.png`} /> { 'mx_MessageComposer_input_markdownIndicator'); ReactTestUtils.Simulate.click(indicator); - expect(mci.state.isRichtextEnabled).toEqual(false, 'should have changed mode'); + expect(mci.state.isRichTextEnabled).toEqual(false, 'should have changed mode'); done(); }); });