From 021409aafe176d9783e5234f649e8f0414f6cd43 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 9 Jul 2018 00:52:27 +0100 Subject: [PATCH] apply review feedback from @lukebarnard1 (cherry picked from commit 37d4bce) Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/HtmlUtils.js | 36 ------- src/Markdown.js | 8 -- src/autocomplete/Autocompleter.js | 6 +- src/autocomplete/UserProvider.js | 2 +- .../views/rooms/MessageComposerInput.js | 94 ++++++------------- .../views/rooms/MessageComposerInput-test.js | 8 +- 6 files changed, 38 insertions(+), 116 deletions(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index ccfecb8081..09ce1187d5 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -112,42 +112,6 @@ export function charactersToImageNode(alt, useSvg, ...unicode) { />; } -/* -export function processHtmlForSending(html: string): string { - const contentDiv = document.createElement('div'); - contentDiv.innerHTML = html; - - if (contentDiv.children.length === 0) { - return contentDiv.innerHTML; - } - - let contentHTML = ""; - for (let i=0; i < contentDiv.children.length; i++) { - const element = contentDiv.children[i]; - if (element.tagName.toLowerCase() === 'p') { - contentHTML += element.innerHTML; - // Don't add a
for the last

- if (i !== contentDiv.children.length - 1) { - contentHTML += '
'; - } - } else if (element.tagName.toLowerCase() === 'pre') { - // Replace "
\n" with "\n" within `

` tags because the 
 is
-            // redundant. This is a workaround for a bug in draft-js-export-html:
-            //   https://github.com/sstur/draft-js-export-html/issues/62
-            contentHTML += '
' +
-                element.innerHTML.replace(/
\n/g, '\n').trim() +
-                '
';
-        } else {
-            const temp = document.createElement('div');
-            temp.appendChild(element.cloneNode(true));
-            contentHTML += temp.innerHTML;
-        }
-    }
-
-    return contentHTML;
-}
-*/
-
 /*
  * Given an untrusted HTML string, return a React node with an sanitized version
  * of that HTML.
diff --git a/src/Markdown.js b/src/Markdown.js
index 9d9a8621c9..dc0d5962fd 100644
--- a/src/Markdown.js
+++ b/src/Markdown.js
@@ -180,14 +180,6 @@ export default class Markdown {
             if (is_multi_line(node) && node.next) this.lit('\n\n');
         };
 
-        // convert MD links into console-friendly ' < http://foo >' style links
-        // ...except given this function never gets called with links, it's useless.
-        // renderer.link = function(node, entering) {
-        //     if (!entering) {
-        //         this.lit(` < ${node.destination} >`);
-        //     }
-        // };
-
         return renderer.render(this.parsed);
     }
 }
diff --git a/src/autocomplete/Autocompleter.js b/src/autocomplete/Autocompleter.js
index b3c20a713c..7f91676cc3 100644
--- a/src/autocomplete/Autocompleter.js
+++ b/src/autocomplete/Autocompleter.js
@@ -29,9 +29,9 @@ import NotifProvider from './NotifProvider';
 import Promise from 'bluebird';
 
 export type SelectionRange = {
-    beginning: boolean,
-    start: number,
-    end: number
+    beginning: boolean, // whether the selection is in the first block of the editor or not
+    start: number, // byte offset relative to the start anchor of the current editor selection.
+    end: number, // byte offset relative to the end anchor of the current editor selection.
 };
 
 export type Completion = {
diff --git a/src/autocomplete/UserProvider.js b/src/autocomplete/UserProvider.js
index 156aac2eb8..7998337e2e 100644
--- a/src/autocomplete/UserProvider.js
+++ b/src/autocomplete/UserProvider.js
@@ -111,7 +111,7 @@ export default class UserProvider extends AutocompleteProvider {
                     // relies on the length of the entity === length of the text in the decoration.
                     completion: user.rawDisplayName.replace(' (IRC)', ''),
                     completionId: user.userId,
-                    suffix: (selection.beginning && range.start === 0) ? ': ' : ' ',
+                    suffix: (selection.beginning && selection.start === 0) ? ': ' : ' ',
                     href: makeUserPermalink(user.userId),
                     component: (
                          {
                         if (obj.object !== 'inline') return;
                         switch (obj.type) {
@@ -288,9 +296,6 @@ export default class MessageComposerInput extends React.Component {
                 }
             ]
         });
-
-        this.suppressAutoComplete = false;
-        this.direction = '';
     }
 
     /*
@@ -298,7 +303,8 @@ export default class MessageComposerInput extends React.Component {
      * - whether we've got rich text mode enabled
      * - contentState was passed in
      */
-    createEditorState(richText: boolean, editorState: ?Value): Value {
+    createEditorState(richText: boolean, // eslint-disable-line no-unused-vars
+                      editorState: ?Value): Value { 
         if (editorState instanceof Value) {
             return editorState;
         }
@@ -371,7 +377,6 @@ export default class MessageComposerInput extends React.Component {
                     let fragmentChange = fragment.change();
                     fragmentChange.moveToRangeOf(fragment.document)
                                   .wrapBlock(quote);
-                                  //.setBlocks('block-quote');
 
                     // FIXME: handle pills and use commonmark rather than md-serialize
                     const md = this.md.serialize(fragmentChange.value);
@@ -459,6 +464,7 @@ export default class MessageComposerInput extends React.Component {
         if (this.direction !== '') {
             const focusedNode = editorState.focusInline || editorState.focusText;
             if (focusedNode.isVoid) {
+                // XXX: does this work in RTL?
                 const edge = this.direction === 'Previous' ? 'End' : 'Start';
                 if (editorState.isCollapsed) {
                     change = change[`collapseTo${ edge }Of${ this.direction }Text`]();
@@ -478,13 +484,6 @@ export default class MessageComposerInput extends React.Component {
             this.onFinishedTyping();
         }
 
-        /*
-        // XXX: what was this ever doing?
-        if (!state.hasOwnProperty('originalEditorState')) {
-            state.originalEditorState = null;
-        }
-        */
-
         if (editorState.startText !== null) {
             const text = editorState.startText.text;
             const currentStartOffset = editorState.startOffset;
@@ -512,9 +511,7 @@ export default class MessageComposerInput extends React.Component {
         }
 
         // emojioneify any emoji
-
-        // XXX: is getTextsAsArray a private API?
-        editorState.document.getTextsAsArray().forEach(node => {
+        editorState.document.getTexts().forEach(node => {
             if (node.text !== '' && HtmlUtils.containsEmoji(node.text)) {
                 let match;
                 while ((match = EMOJI_REGEX.exec(node.text)) !== null) {
@@ -546,36 +543,6 @@ export default class MessageComposerInput extends React.Component {
             editorState = change.value;
         }
 
-/*
-        const currentBlock = editorState.getSelection().getStartKey();
-        const currentSelection = editorState.getSelection();
-        const currentStartOffset = editorState.getSelection().getStartOffset();
-
-        const block = editorState.getCurrentContent().getBlockForKey(currentBlock);
-        const text = block.getText();
-
-        const entityBeforeCurrentOffset = block.getEntityAt(currentStartOffset - 1);
-        const entityAtCurrentOffset = block.getEntityAt(currentStartOffset);
-
-        // If the cursor is on the boundary between an entity and a non-entity and the
-        // text before the cursor has whitespace at the end, set the entity state of the
-        // character before the cursor (the whitespace) to null. This allows the user to
-        // stop editing the link.
-        if (entityBeforeCurrentOffset && !entityAtCurrentOffset &&
-            /\s$/.test(text.slice(0, currentStartOffset))) {
-            editorState = RichUtils.toggleLink(
-                editorState,
-                currentSelection.merge({
-                    anchorOffset: currentStartOffset - 1,
-                    focusOffset: currentStartOffset,
-                }),
-                null,
-            );
-            // Reset selection
-            editorState = EditorState.forceSelection(editorState, currentSelection);
-        }
-*/
-
         if (this.props.onInputStateChanged && editorState.blocks.size > 0) {
             let blockType = editorState.blocks.first().type;
             // console.log("onInputStateChanged; current block type is " + blockType + " and marks are " + editorState.activeMarks);
@@ -605,7 +572,6 @@ export default class MessageComposerInput extends React.Component {
             editor_state: editorState,
         });
 
-        /* Since a modification was made, set originalEditorState to null, since newState is now our original */
         this.setState({
             editorState,
             originalEditorState: originalEditorState || null
@@ -683,7 +649,7 @@ export default class MessageComposerInput extends React.Component {
 
     hasMark = type => {
         const { editorState } = this.state
-        return editorState.activeMarks.some(mark => mark.type == type)
+        return editorState.activeMarks.some(mark => mark.type === type)
     };
 
     /**
@@ -695,7 +661,7 @@ export default class MessageComposerInput extends React.Component {
 
     hasBlock = type => {
         const { editorState } = this.state
-        return editorState.blocks.some(node => node.type == type)
+        return editorState.blocks.some(node => node.type === type)
     };
 
     onKeyDown = (ev: KeyboardEvent, change: Change, editor: Editor) => {
@@ -828,7 +794,7 @@ export default class MessageComposerInput extends React.Component {
                     // Handle the extra wrapping required for list buttons.
                     const isList = this.hasBlock('list-item');
                     const isType = editorState.blocks.some(block => {
-                        return !!document.getClosest(block.key, parent => parent.type == type);
+                        return !!document.getClosest(block.key, parent => parent.type === type);
                     });
 
                     if (isList && isType) {
@@ -839,7 +805,7 @@ export default class MessageComposerInput extends React.Component {
                     } else if (isList) {
                         change
                             .unwrapBlock(
-                                type == 'bulleted-list' ? 'numbered-list' : 'bulleted-list'
+                                type === 'bulleted-list' ? 'numbered-list' : 'bulleted-list'
                             )
                             .wrapBlock(type);
                     } else {
@@ -1009,7 +975,7 @@ export default class MessageComposerInput extends React.Component {
         let contentHTML;
 
         // only look for commands if the first block contains simple unformatted text
-        // i.e. no pills or rich-text formatting.
+        // i.e. no pills or rich-text formatting and begins with a /.
         let cmd, commandText;
         const firstChild = editorState.document.nodes.get(0);
         const firstGrandChild = firstChild && firstChild.nodes.get(0);
@@ -1090,8 +1056,8 @@ export default class MessageComposerInput extends React.Component {
                 // didn't contain any formatting in the first place...
                 contentText = mdWithPills.toPlaintext();
             } else {
-                // to avoid ugliness clients which can't parse HTML we don't send pills
-                // in the plaintext body.
+                // to avoid ugliness on clients which ignore the HTML body we don't
+                // send pills in the plaintext body.
                 contentText = this.plainWithPlainPills.serialize(editorState);
                 contentHTML = mdWithPills.toHTML();
             }
@@ -1255,11 +1221,8 @@ export default class MessageComposerInput extends React.Component {
         // Move selection to the end of the selected history
         const change = editorState.change().collapseToEndOf(editorState.document);
 
-        // XXX: should we be calling this.onChange(change) now?
-        // Answer: yes, if we want it to do any of the fixups on stuff like emoji.
-        // however, this should already have been done and persisted in the history,
-        // so shouldn't be necessary.
-
+        // We don't call this.onChange(change) now, as fixups on stuff like emoji
+        // should already have been done and persisted in the history.
         editorState = change.value;
 
         this.suppressAutoComplete = true;
@@ -1362,6 +1325,8 @@ export default class MessageComposerInput extends React.Component {
                                 .insertText(suffix)
                                 .focus();
         }
+        // for good hygiene, keep editorState updated to track the result of the change
+        // even though we don't do anything subsequently with it
         editorState = change.value;
 
         this.onChange(change, activeEditorState);
@@ -1460,10 +1425,11 @@ export default class MessageComposerInput extends React.Component {
     };
 
     onFormatButtonClicked = (name, e) => {
-        e.preventDefault(); // don't steal focus from the editor!
+        e.preventDefault();
 
         // XXX: horrible evil hack to ensure the editor is focused so the act
         // of focusing it doesn't then cancel the format button being pressed
+        // FIXME: can we just tell handleKeyCommand's change to invoke .focus()?
         if (document.activeElement && document.activeElement.className !== 'mx_MessageComposer_editor') {
             this.refs.editor.focus();
             setTimeout(()=>{
diff --git a/test/components/views/rooms/MessageComposerInput-test.js b/test/components/views/rooms/MessageComposerInput-test.js
index 708071df23..662fbc7104 100644
--- a/test/components/views/rooms/MessageComposerInput-test.js
+++ b/test/components/views/rooms/MessageComposerInput-test.js
@@ -10,7 +10,6 @@ const MessageComposerInput = sdk.getComponent('views.rooms.MessageComposerInput'
 import MatrixClientPeg from '../../../../src/MatrixClientPeg';
 import RoomMember from 'matrix-js-sdk';
 
-/*
 function addTextToDraft(text) {
     const components = document.getElementsByClassName('public-DraftEditor-content');
     if (components && components.length) {
@@ -21,7 +20,9 @@ function addTextToDraft(text) {
     }
 }
 
-describe('MessageComposerInput', () => {
+// FIXME: These tests need to be updated from Draft to Slate.
+
+xdescribe('MessageComposerInput', () => {
     let parentDiv = null,
         sandbox = null,
         client = null,
@@ -300,5 +301,4 @@ describe('MessageComposerInput', () => {
         expect(spy.args[0][1].body).toEqual('[Click here](https://some.lovely.url)');
         expect(spy.args[0][1].formatted_body).toEqual('Click here');
     });
-});
-*/
\ No newline at end of file
+});
\ No newline at end of file