General code cleanup / tweaks / fixes

- Swap Phases enum to be using string literals
- Swap roomId prop on UserSettings for a more sane onUserSettingsClose and
  make MatrixChat responsible for swapping the room.
- s/then/done/ when terminating Promise chains to avoid subtle errors.
- Rejig render() of UserSettings so we don't need to indent quite so much.

src/UserSettingsStore.js

@@ -17,19 +17,10 @@ limitations under the License.
 'use strict';
 
 var MatrixClientPeg = require("./MatrixClientPeg");
-var sdk = require('./index');
+var Notifier = require("./Notifier");
-
-// XXX: should we be doing something here to use this as a singleton rather than
-// class methods?
 
 module.exports = {
 
-    // we add these wrappers to the js-sdk here in case we want to do react-specific
-    // dispatches or similar in future, and to give us a place to clearly separate
-    // business logic specific from the 'thin' react component and parent wiring component
-    //  which actually handles the UI.
-    // XXX: I'm not convinced this abstraction is worth it though.
-
     loadProfileInfo: function() {
         var cli = MatrixClientPeg.get();
         return cli.getProfileInfo(cli.credentials.userId);
@@ -48,13 +39,10 @@ module.exports = {
     },
 
     getEnableNotifications: function() {
-        var Notifier = sdk.getComponent('organisms.Notifier');
         return Notifier.isEnabled();
     },
 
     setEnableNotifications: function(enable) {
-        var Notifier = sdk.getComponent('organisms.Notifier');
-        var self = this;
         if (!Notifier.supportsDesktopNotifications()) {
             return;
         }
@@ -70,11 +58,6 @@ module.exports = {
             password: old_password
         };
 
-        this.setState({
-            phase: this.Phases.Uploading,
-            errorString: '',
-        })
-
         return cli.setPassword(authDict, new_password);
     },
-}
+};

src/components/structures/MatrixChat.js

@@ -628,6 +628,22 @@ module.exports = React.createClass({
         this.showScreen("settings");
     },
 
+    onUserSettingsClose: function() {
+        // XXX: use browser history instead to find the previous room?
+        if (this.state.currentRoom) {
+            dis.dispatch({
+                action: 'view_room',
+                room_id: this.state.currentRoom,
+            });
+        }
+        else {
+            dis.dispatch({
+                action: 'view_indexed_room',
+                roomIndex: 0,
+            });
+        }
+    },
+
     render: function() {
         var LeftPanel = sdk.getComponent('structures.LeftPanel');
         var RoomView = sdk.getComponent('structures.RoomView');
@@ -660,7 +676,7 @@ module.exports = React.createClass({
                     right_panel = <RightPanel roomId={this.state.currentRoom} collapsed={this.state.collapse_rhs} />
                     break;
                 case this.PageTypes.UserSettings:
-                    page_element = <UserSettings roomId={this.state.currentRoom} />
+                    page_element = <UserSettings onClose={this.onUserSettingsClose} />
                     right_panel = <RightPanel collapsed={this.state.collapse_rhs}/>
                     break;
                 case this.PageTypes.CreateRoom:

src/components/structures/UserSettings.js

@@ -25,10 +25,14 @@ var UserSettingsStore = require('../../UserSettingsStore');
 module.exports = React.createClass({
     displayName: 'UserSettings',
 
-    Phases: {
+    propTypes: {
-        Loading: "loading",
+        onClose: React.PropTypes.func
-        Saving: "saving",
+    },
-        Display: "display",
+
+    getDefaultProps: function() {
+        return {
+            onClose: function() {}
+        };
     },
 
     getInitialState: function() {
@@ -37,38 +41,34 @@ module.exports = React.createClass({
             displayName: null,
             threePids: [],
             clientVersion: version,
-            phase: this.Phases.Loading,
+            phase: "UserSettings.LOADING", // LOADING, DISPLAY, SAVING
         };
     },
 
     componentWillMount: function() {
         var self = this;
 
-        var profilePromise = UserSettingsStore.loadProfileInfo();
+        q.all([
-        var threepidPromise = UserSettingsStore.loadThreePids();
+            UserSettingsStore.loadProfileInfo(), UserSettingsStore.loadThreePids()
+        ]).done(function(resps) {
+            self.setState({
+                avatarUrl: resps[0].avatar_url,
+                displayName: resps[0].displayname,
+                threepids: resps[1].threepids,
+                phase: "UserSettings.DISPLAY",
+            });
 
-        q.all([profilePromise, threepidPromise]).then(
+            // keep a copy of the original state in order to track changes
-            function(resps) {
+            self.setState({
-                self.setState({
+                originalState: self.state
-                    avatarUrl: resps[0].avatar_url,
+            });
-                    displayName: resps[0].displayname,
+        }, function(error) {
-                    threepids: resps[1].threepids,
+            var ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
-                    phase: self.Phases.Display,
+            Modal.createDialog(ErrorDialog, {
-                });
+                title: "Can't load user settings",
-
+                description: error.toString()
-                // keep a copy of the original state in order to track changes
+            });
-                self.setState({
+        });
-                    originalState: self.state
-                });
-            },
-            function(error) {
-                var ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
-                Modal.createDialog(ErrorDialog, {
-                    title: "Can't load user settings",
-                    description: error.toString()
-                });
-            }
-        );
     },
 
     componentDidMount: function() {
@@ -95,51 +95,33 @@ module.exports = React.createClass({
         }
 
         if (this.state.originalState.threepids.length !== this.state.threepids.length ||
-            this.state.originalState.threepids.every(function(element, index) {
+                this.state.originalState.threepids.every((element, index) => {
                     return element === this.state.threepids[index];
-            }))
+                })) {
-        {
+            savePromises.push(
-            savePromises.push( UserSettingsStore.saveThreePids(this.state.threepids) );
+                UserSettingsStore.saveThreePids(this.state.threepids)
+            );
         }
 
         self.setState({
-            phase: self.Phases.Saving,
+            phase: "UserSettings.SAVING",
         });
 
-        q.all(savePromises).then(
+        q.all(savePromises).done(function(resps) {
-            function(resps) {
+            self.setState({
-                self.setState({
+                phase: "UserSettings.DISPLAY",
-                    phase: self.Phases.Display,
-                });
-                self.onClose();
-            },
-            function(error) {
-                self.setState({
-                    phase: self.Phases.Display,
-                });
-                var ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
-                Modal.createDialog(ErrorDialog, {
-                    title: "Can't save user settings",
-                    description: error.toString()
-                });
-            }
-        );        
-    },
-
-    onClose: function(ev) {
-        // XXX: use browser history instead to find the previous room?
-        if (this.props.roomId) {
-            dis.dispatch({
-                action: 'view_room',
-                room_id: this.props.roomId,
             });
-        }
+            self.props.onClose();
-        else {
+        }, function(error) {
-            dis.dispatch({
+            self.setState({
-                action: 'view_indexed_room',
+                phase: "UserSettings.DISPLAY",
-                roomIndex: 0,
             });
-        }
+            var ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
+            Modal.createDialog(ErrorDialog, {
+                title: "Can't save user settings",
+                description: error.toString()
+            });
+        });        
     },
 
     onAction: function(payload) {
@@ -170,7 +152,9 @@ module.exports = React.createClass({
 
     onLogoutClicked: function(ev) {
         var LogoutPrompt = sdk.getComponent('dialogs.LogoutPrompt');
-        this.logoutModal = Modal.createDialog(LogoutPrompt, {onCancel: this.onLogoutPromptCancel});
+        this.logoutModal = Modal.createDialog(
+            LogoutPrompt, {onCancel: this.onLogoutPromptCancel}
+        );
     },
 
     onLogoutPromptCancel: function() {
@@ -193,100 +177,121 @@ module.exports = React.createClass({
         var Loader = sdk.getComponent("elements.Spinner");
         var saving;
         switch (this.state.phase) {
-            case this.Phases.Loading:
+            case "UserSettings.LOADING":
                 return <Loader />
-            case this.Phases.Saving:
+            case "UserSettings.SAVING":
                 saving = <Loader />
-            case this.Phases.Display:
+                // intentional fall through
-                var RoomHeader = sdk.getComponent('rooms.RoomHeader');
+            case "UserSettings.DISPLAY":
-                return (
+                break; // quit the switch to return the common state
-                    <div className="mx_UserSettings">
+            default:
-                        <RoomHeader simpleHeader="Settings" onCancelClick={ this.onClose } />
+                throw new Error("Unknown state.phase => " + this.state.phase);
+        }
+        // can only get here if phase is UserSettings.DISPLAY
+        var RoomHeader = sdk.getComponent('rooms.RoomHeader');
+        return (
+            <div className="mx_UserSettings">
+                <RoomHeader simpleHeader="Settings" onCancelClick={ this.props.onClose } />
 
-                        <h2>Profile</h2>
+                <h2>Profile</h2>
 
-                        <div className="mx_UserSettings_section">
+                <div className="mx_UserSettings_section">
-                            <div className="mx_UserSettings_profileTable">
+                    <div className="mx_UserSettings_profileTable">
-                                <div className="mx_UserSettings_profileTableRow">
+                        <div className="mx_UserSettings_profileTableRow">
+                            <div className="mx_UserSettings_profileLabelCell">
+                                <label htmlFor="displayName">Display name</label>
+                            </div>
+                            <div className="mx_UserSettings_profileInputCell">
+                                <input id="displayName" ref="displayName"
+                                    value={ this.state.displayName }
+                                    onChange={ this.onDisplayNameChange } />
+                            </div>
+                        </div>
+
+                        {this.state.threepids.map(function(val, pidIndex) {
+                            var id = "email-" + val.address;
+                            return (
+                                <div className="mx_UserSettings_profileTableRow" key={pidIndex}>
                                     <div className="mx_UserSettings_profileLabelCell">
-                                        <label htmlFor="displayName">Display name</label>
+                                        <label htmlFor={id}>Email</label>
                                     </div>
                                     <div className="mx_UserSettings_profileInputCell">
-                                        <input id="displayName" ref="displayName" value={ this.state.displayName } onChange={ this.onDisplayNameChange } />
+                                        <input key={val.address} id={id} value={val.address} disabled />
-                                    </div>
-                                </div>
-
-                                {this.state.threepids.map(function(val) {
-                                    var id = "email-" + val.address;
-                                    return (
-                                        <div className="mx_UserSettings_profileTableRow">
-                                            <div className="mx_UserSettings_profileLabelCell">
-                                                <label htmlFor={ id }>Email</label>
-                                            </div>
-                                            <div className="mx_UserSettings_profileInputCell">
-                                                <input key={val.address} id={ id } value={ val.address } disabled />
-                                            </div>
-                                        </div>
-                                    );
-                                })}
-
-                                <div className="mx_UserSettings_profileTableRow">
-                                    <div className="mx_UserSettings_profileLabelCell">
-                                        <label htmlFor="password1">New password</label>
-                                    </div>
-                                    <div className="mx_UserSettings_profileInputCell">
-                                        <input id="password1" ref="password1" value={ this.state.password1 } />
-                                    </div>
-                                </div>
-                                <div className="mx_UserSettings_profileTableRow">
-                                    <div className="mx_UserSettings_profileLabelCell">
-                                        <label htmlFor="password2">Confirm new password</label>
-                                    </div>
-                                    <div className="mx_UserSettings_profileInputCell">
-                                        <input id="password2" ref="password2" value={ this.state.password2 } />
                                     </div>
                                 </div>
+                            );
+                        })}
 
+                        <div className="mx_UserSettings_profileTableRow">
+                            <div className="mx_UserSettings_profileLabelCell">
+                                <label htmlFor="password1">New password</label>
                             </div>
-
+                            <div className="mx_UserSettings_profileInputCell">
-                            <div className="mx_UserSettings_avatarPicker">
+                                <input id="password1" ref="password1"
-                                <div className="mx_UserSettings_avatarPicker_edit" onClick={this.editAvatar}></div>
+                                    value={ this.state.password1 } />
+                            </div>
+                        </div>
+                        <div className="mx_UserSettings_profileTableRow">
+                            <div className="mx_UserSettings_profileLabelCell">
+                                <label htmlFor="password2">Confirm new password</label>
+                            </div>
+                            <div className="mx_UserSettings_profileInputCell">
+                                <input id="password2" ref="password2"
+                                    value={ this.state.password2 } />
                             </div>
                         </div>
 
-                        <div className="mx_UserSettings_logout">
+                    </div>                        
-                            <div className="mx_UserSettings_button" onClick={this.onLogoutClicked}>Log out</div>
-                        </div>
 
-                        <h2>Notifications</h2>
+                    <div className="mx_UserSettings_avatarPicker">
-
+                        <div className="mx_UserSettings_avatarPicker_edit"
-                        <div className="mx_UserSettings_section">
+                            onClick={this.editAvatar}>
-                            <div className="mx_UserSettings_notifTable">
-                                <div className="mx_UserSettings_notifTableRow">
-                                    <div className="mx_UserSettings_notifInputCell">
-                                        <input id="enableNotifications" ref="enableNotifications" type="checkbox" checked={ this.state.enableNotifications } onChange={ this.onEnableNotificationsChange } />
-                                    </div>
-                                    <div className="mx_UserSettings_notifLabelCell">
-                                        <label htmlFor="enableNotifications">Enable desktop notifications</label>
-                                    </div>
-                                </div>
-                            </div>
-                        </div>
-
-                        <h2>Advanced</h2>
-
-                        <div className="mx_UserSettings_section">
-                            <div className="mx_UserSettings_advanced">
-                                Version {this.state.clientVersion}
-                            </div>
-                        </div>
-
-                        <div className="mx_UserSettings_save">
-                            <div className="mx_UserSettings_spinner">{ saving }</div>
-                            <div className="mx_UserSettings_button" onClick={this.onSaveClicked}>Save and close</div>
                         </div>
                     </div>
-                );
+                </div>
-        }
+
+                <div className="mx_UserSettings_logout">
+                    <div className="mx_UserSettings_button" onClick={this.onLogoutClicked}>
+                        Log out
+                    </div>
+                </div>
+
+                <h2>Notifications</h2>
+
+                <div className="mx_UserSettings_section">
+                    <div className="mx_UserSettings_notifTable">
+                        <div className="mx_UserSettings_notifTableRow">
+                            <div className="mx_UserSettings_notifInputCell">
+                                <input id="enableNotifications"
+                                    ref="enableNotifications"
+                                    type="checkbox"
+                                    checked={ this.state.enableNotifications }
+                                    onChange={ this.onEnableNotificationsChange } />
+                            </div>
+                            <div className="mx_UserSettings_notifLabelCell">
+                                <label htmlFor="enableNotifications">
+                                    Enable desktop notifications
+                                </label>
+                            </div>
+                        </div>
+                    </div>
+                </div>
+
+                <h2>Advanced</h2>
+
+                <div className="mx_UserSettings_section">
+                    <div className="mx_UserSettings_advanced">
+                        Version {this.state.clientVersion}
+                    </div>
+                </div>
+
+                <div className="mx_UserSettings_save">
+                    <div className="mx_UserSettings_spinner">{ saving }</div>
+                    <div className="mx_UserSettings_button" onClick={this.onSaveClicked}>
+                        Save and close
+                    </div>
+                </div>
+            </div>
+        );
     }
 });