From 17fdfa0c8fd4c2f687457f0be47e51266959d0a4 Mon Sep 17 00:00:00 2001
From: Matthew Hodgson <matthew@matrix.org>
Date: Mon, 18 Jan 2016 14:00:47 +0000
Subject: [PATCH] incorporate kegan PR feedback

---
 src/components/views/elements/EditableText.js | 13 ++++++------
 src/components/views/rooms/RoomSettings.js    | 20 +++++++++++++++++--
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/src/components/views/elements/EditableText.js b/src/components/views/elements/EditableText.js
index 2db4bec556..683cfe4fc8 100644
--- a/src/components/views/elements/EditableText.js
+++ b/src/components/views/elements/EditableText.js
@@ -41,9 +41,6 @@ module.exports = React.createClass({
         Edit: "edit",
     },
 
-    value: '',
-    placeholder: false,
-
     getDefaultProps: function() {
         return {
             onValueChanged: function() {},
@@ -69,6 +66,13 @@ module.exports = React.createClass({
         }
     },
 
+    componentWillMount: function() {
+        // we track value as an JS object field rather than in React state
+        // as React doesn't play nice with contentEditable.
+        this.value = '';
+        this.placeholder = false;
+    },
+
     componentDidMount: function() {
         this.value = this.props.initialValue;
         if (this.refs.editable_div) {
@@ -76,9 +80,6 @@ module.exports = React.createClass({
         }
     },
 
-    componentDidUpdate: function() {
-    },
-
     showPlaceholder: function(show) {
         if (show) {
             this.refs.editable_div.textContent = this.props.placeholder;
diff --git a/src/components/views/rooms/RoomSettings.js b/src/components/views/rooms/RoomSettings.js
index 3dd9655617..0ff15a4f43 100644
--- a/src/components/views/rooms/RoomSettings.js
+++ b/src/components/views/rooms/RoomSettings.js
@@ -168,6 +168,9 @@ module.exports = React.createClass({
             oldAliases[domain] = alias_events[i].getContent().aliases.slice(); // shallow copy
         }
 
+        // FIXME: this whole delta-based set comparison function used for domains, aliases & tags
+        // should be factored out asap rather than duplicated like this.
+
         // work out whether any domains have entirely disappeared or appeared
         var domainDelta = {}
         Object.keys(oldAliases).forEach(function(domain) {
@@ -208,9 +211,15 @@ module.exports = React.createClass({
                             ops.push({ type: "put", alias: alias });
                         } else if (delta[alias] == -1) {
                             ops.push({ type: "delete", alias: alias });
+                        } else {
+                            console.error("Calculated alias delta of " + delta[alias] +
+                                          " - this should never happen!");                            
                         }
                     });
-
+                    break;
+                default:
+                    console.error("Calculated domain delta of " + domainDelta[domain] +
+                                  " - this should never happen!");
                     break;
             }
         });
@@ -237,6 +246,9 @@ module.exports = React.createClass({
                 ops.push({ type: "put", tag: tag });
             } else if (delta[tag] == -1) {
                 ops.push({ type: "delete", tag: tag });
+            } else {
+                console.error("Calculated tag delta of " + delta[tag] +
+                              " - this should never happen!");
             }
         });
 
@@ -286,7 +298,11 @@ module.exports = React.createClass({
     },
 
     onAliasDeleted: function(domain, index) {
-        // XXX: can we edit state directly and then set, or should we copy it first?
+        // It's a bit naughty to directly manipulate this.state, and React would
+        // normally whine at you, but it can't see us doing the splice.  Given we
+        // promptly setState anyway, it's just about acceptable.  The alternative
+        // would be to arbitrarily deepcopy to a temp variable and then setState
+        // that, but why bother when we can cut this corner.
         var alias = this.state.aliases[domain].splice(index, 1);
         this.setState({ 
             aliases: this.state.aliases