From ce42a9a06f0fb12bf0bf434e68fcfeae4d214566 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 9 Jun 2017 17:18:45 +0100 Subject: [PATCH 1/6] Replace MatrixChat.state.screen with 'view' 'screen' is overloaded, as it us used for the parameter of `showScreen` (and, by implication, `state.screenAfterLogin`). Attempt to clear up the confusion by replacing 'screen' with 'view' and using some constants for the potential values. This should be a no-op! --- src/components/structures/MatrixChat.js | 43 ++++++++++++++++--------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index efca22cc85..f6eb56c06f 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -43,6 +43,15 @@ import createRoom from "../../createRoom"; import * as UDEHandler from '../../UnknownDeviceErrorHandler'; import { _t, getCurrentLanguage } from '../../languageHandler'; +/** constants for MatrixChat.state.view */ +const VIEWS = { + DEFAULT: 0, + LOGIN: 1, + REGISTER: 2, + POST_REGISTRATION: 3, + FORGOT_PASSWORD: 4, +}; + module.exports = React.createClass({ displayName: 'MatrixChat', @@ -94,7 +103,11 @@ module.exports = React.createClass({ getInitialState: function() { const s = { loading: true, - screen: undefined, + + // the master view we are showing. + view: VIEWS.DEFAULT, + + // a thing to call showScreen with once login completes. screenAfterLogin: this.props.initialScreenAfterLogin, // Stashed guest credentials if the user logs out @@ -317,9 +330,9 @@ module.exports = React.createClass({ } }, - setStateForNewScreen: function(state) { + setStateForNewView: function(state) { const newState = { - screen: undefined, + view: VIEWS.DEFAULT, viewUserId: null, loggedIn: false, ready: false, @@ -347,19 +360,19 @@ module.exports = React.createClass({ guestCreds: MatrixClientPeg.getCredentials(), }); } - this.setStateForNewScreen({ - screen: 'login', + this.setStateForNewView({ + view: VIEWS.LOGIN, }); this.notifyNewScreen('login'); break; case 'start_post_registration': this.setState({ // don't clobber loggedIn status - screen: 'post_registration', + view: VIEWS.POST_REGISTRATION, }); break; case 'start_password_recovery': - this.setStateForNewScreen({ - screen: 'forgot_password', + this.setStateForNewView({ + view: VIEWS.FORGOT_PASSWORD, }); this.notifyNewScreen('forgot_password'); break; @@ -537,8 +550,8 @@ module.exports = React.createClass({ }, _startRegistration: function(params) { - this.setStateForNewScreen({ - screen: 'register', + this.setStateForNewView({ + view: VIEWS.REGISTER, // these params may be undefined, but if they are, // unset them from our state: we don't want to // resume a previous registration session if the @@ -969,7 +982,7 @@ module.exports = React.createClass({ */ _onLoggedOut: function() { this.notifyNewScreen('login'); - this.setStateForNewScreen({ + this.setStateForNewView({ loggedIn: false, ready: false, collapse_lhs: false, @@ -1253,7 +1266,7 @@ module.exports = React.createClass({ onFinishPostRegistration: function() { // Don't confuse this with "PageType" which is the middle window to show this.setState({ - screen: undefined, + view: VIEWS.DEFAULT, }); this.showScreen("settings"); }, @@ -1335,7 +1348,7 @@ module.exports = React.createClass({ } // needs to be before normal PageTypes as you are logged in technically - if (this.state.screen == 'post_registration') { + if (this.state.view === VIEWS.POST_REGISTRATION) { const PostRegistration = sdk.getComponent('structures.login.PostRegistration'); return ( ); - } else if (this.state.screen == 'register') { + } else if (this.state.view == VIEWS.REGISTER) { const Registration = sdk.getComponent('structures.login.Registration'); return ( ); - } else if (this.state.screen == 'forgot_password') { + } else if (this.state.view == VIEWS.FORGOT_PASSWORD) { const ForgotPassword = sdk.getComponent('structures.login.ForgotPassword'); return ( Date: Wed, 14 Jun 2017 20:04:55 +0100 Subject: [PATCH 2/6] MatrixChat: Replace state.{loading,loggedIn,loggingIn} with views MatrixChat is essentially a glorified state machine, with its states partially determined by the loading, loggedIn and loggingIn state flags. If we actually make each of the states implied by those flags an explicit 'view', then everything gets much clearer. --- src/components/structures/MatrixChat.js | 77 ++++++++++++++++--------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index f6eb56c06f..a34bafe086 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -45,14 +45,38 @@ import { _t, getCurrentLanguage } from '../../languageHandler'; /** constants for MatrixChat.state.view */ const VIEWS = { - DEFAULT: 0, + // a special initial state which is only used at startup, while we are + // trying to re-animate a matrix client or register as a guest. + LOADING: 0, + + // we are showing the login view LOGIN: 1, + + // we are showing the registration view REGISTER: 2, + + // completeing the registration flow POST_REGISTRATION: 3, + + // showing the 'forgot password' view FORGOT_PASSWORD: 4, + + // we have valid matrix credentials (either via an explicit login, via the + // initial re-animation/guest registration, or via a registration), and are + // now setting up a matrixclient to talk to it. This isn't an instant + // process because (a) we need to clear out indexeddb, and (b) we need to + LOGGING_IN: 5, + + // we are logged in with an active matrix client. + LOGGED_IN: 6, }; module.exports = React.createClass({ + // we export this so that the integration tests can use it :-S + statics: { + VIEWS: VIEWS, + }, + displayName: 'MatrixChat', propTypes: { @@ -102,10 +126,8 @@ module.exports = React.createClass({ getInitialState: function() { const s = { - loading: true, - // the master view we are showing. - view: VIEWS.DEFAULT, + view: VIEWS.LOADING, // a thing to call showScreen with once login completes. screenAfterLogin: this.props.initialScreenAfterLogin, @@ -126,8 +148,6 @@ module.exports = React.createClass({ // If we're trying to just view a user ID (i.e. /user URL), this is it viewUserId: null, - loggedIn: false, - loggingIn: false, collapse_lhs: false, collapse_rhs: false, ready: false, @@ -289,7 +309,6 @@ module.exports = React.createClass({ firstScreen === 'register' || firstScreen === 'forgot_password') { this.props.onLoadCompleted(); - this.setState({loading: false}); this._showScreenAfterLogin(); return; } @@ -331,17 +350,18 @@ module.exports = React.createClass({ }, setStateForNewView: function(state) { + if (state.view === undefined) { + throw new Error("setStateForNewView with no view!"); + } const newState = { - view: VIEWS.DEFAULT, viewUserId: null, - loggedIn: false, - ready: false, }; Object.assign(newState, state); this.setState(newState); }, onAction: function(payload) { + // console.log(`MatrixClientPeg.onAction: ${payload.action}`); const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog"); @@ -366,7 +386,7 @@ module.exports = React.createClass({ this.notifyNewScreen('login'); break; case 'start_post_registration': - this.setState({ // don't clobber loggedIn status + this.setState({ view: VIEWS.POST_REGISTRATION, }); break; @@ -516,7 +536,10 @@ module.exports = React.createClass({ // and also that we're not ready (we'll be marked as logged // in once the login completes, then ready once the sync // completes). - this.setState({loggingIn: true, ready: false}); + this.setStateForNewView({ + view: VIEWS.LOGGING_IN, + ready: false, + }); break; case 'on_logged_in': this._onLoggedIn(payload.teamToken); @@ -864,7 +887,12 @@ module.exports = React.createClass({ */ _onLoadCompleted: function() { this.props.onLoadCompleted(); - this.setState({loading: false}); + + // if we've got this far without leaving the 'loading' view, then + // login must have failed, so start the login process + if (this.state.view === VIEWS.LOADING) { + dis.dispatch({action: "start_login"}); + } }, /** @@ -919,9 +947,8 @@ module.exports = React.createClass({ */ _onLoggedIn: function(teamToken) { this.setState({ + view: VIEWS.LOGGED_IN, guestCreds: null, - loggedIn: true, - loggingIn: false, }); if (teamToken) { @@ -983,7 +1010,7 @@ module.exports = React.createClass({ _onLoggedOut: function() { this.notifyNewScreen('login'); this.setStateForNewView({ - loggedIn: false, + view: VIEWS.LOGIN, ready: false, collapse_lhs: false, collapse_rhs: false, @@ -1146,7 +1173,7 @@ module.exports = React.createClass({ // we can't view a room unless we're logged in // (a guest account is fine) - if (this.state.loggedIn) { + if (this.state.view === VIEWS.LOGGED_IN) { dis.dispatch(payload); } } else if (screen.indexOf('user/') == 0) { @@ -1266,7 +1293,7 @@ module.exports = React.createClass({ onFinishPostRegistration: function() { // Don't confuse this with "PageType" which is the middle window to show this.setState({ - view: VIEWS.DEFAULT, + view: VIEWS.LOGGED_IN, }); this.showScreen("settings"); }, @@ -1334,11 +1361,9 @@ module.exports = React.createClass({ }, render: function() { - // `loading` might be set to false before `loggedIn = true`, causing the default - // (``) to be visible for a few MS (say, whilst a request is in-flight to - // the RTS). So in the meantime, use `loggingIn`, which is true between - // actions `on_logging_in` and `on_logged_in`. - if (this.state.loading || this.state.loggingIn) { + // console.log(`Rendering MatrixChat with view ${this.state.view}`); + + if (this.state.view === VIEWS.LOADING || this.state.view === VIEWS.LOGGING_IN) { const Spinner = sdk.getComponent('elements.Spinner'); return (
@@ -1356,10 +1381,10 @@ module.exports = React.createClass({ ); } - // `ready` and `loggedIn` may be set before `page_type` (because the + // `ready` and `view==LOGGED_IN` may be set before `page_type` (because the // latter is set via the dispatcher). If we don't yet have a `page_type`, // keep showing the spinner for now. - if (this.state.loggedIn && this.state.ready && this.state.page_type) { + if (this.state.view === VIEWS.LOGGED_IN && this.state.ready && this.state.page_type) { /* for now, we stuff the entirety of our props and state into the LoggedInView. * we should go through and figure out what we actually need to pass down, as well * as using something like redux to avoid having a billion bits of state kicking around. @@ -1376,7 +1401,7 @@ module.exports = React.createClass({ {...this.state} /> ); - } else if (this.state.loggedIn) { + } else if (this.state.view === VIEWS.LOGGED_IN) { // we think we are logged in, but are still waiting for the /sync to complete const Spinner = sdk.getComponent('elements.Spinner'); return ( From 7b526308fdf2f82e8c2ceb5b7f923ee0e5c24e0e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 15 Jun 2017 17:36:57 +0100 Subject: [PATCH 3/6] Rearrange MatrixChat.render for sanity no-op to make it into a nice simple switch-like arrangement --- src/components/structures/MatrixChat.js | 79 ++++++++++++++----------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index a34bafe086..c63a0cfaca 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -1381,38 +1381,42 @@ module.exports = React.createClass({ ); } - // `ready` and `view==LOGGED_IN` may be set before `page_type` (because the - // latter is set via the dispatcher). If we don't yet have a `page_type`, - // keep showing the spinner for now. - if (this.state.view === VIEWS.LOGGED_IN && this.state.ready && this.state.page_type) { - /* for now, we stuff the entirety of our props and state into the LoggedInView. - * we should go through and figure out what we actually need to pass down, as well - * as using something like redux to avoid having a billion bits of state kicking around. - */ - const LoggedInView = sdk.getComponent('structures.LoggedInView'); - return ( - - ); - } else if (this.state.view === VIEWS.LOGGED_IN) { - // we think we are logged in, but are still waiting for the /sync to complete - const Spinner = sdk.getComponent('elements.Spinner'); - return ( - - ); - } else if (this.state.view == VIEWS.REGISTER) { + if (this.state.view === VIEWS.LOGGED_IN) { + // `ready` and `view==LOGGED_IN` may be set before `page_type` (because the + // latter is set via the dispatcher). If we don't yet have a `page_type`, + // keep showing the spinner for now. + if (this.state.ready && this.state.page_type) { + /* for now, we stuff the entirety of our props and state into the LoggedInView. + * we should go through and figure out what we actually need to pass down, as well + * as using something like redux to avoid having a billion bits of state kicking around. + */ + const LoggedInView = sdk.getComponent('structures.LoggedInView'); + return ( + + ); + } else { + // we think we are logged in, but are still waiting for the /sync to complete + const Spinner = sdk.getComponent('elements.Spinner'); + return ( + + ); + } + } + + if (this.state.view === VIEWS.REGISTER) { const Registration = sdk.getComponent('structures.login.Registration'); return ( ); - } else if (this.state.view == VIEWS.FORGOT_PASSWORD) { + } + + + if (this.state.view === VIEWS.FORGOT_PASSWORD) { const ForgotPassword = sdk.getComponent('structures.login.ForgotPassword'); return ( ); - } else { + } + + if (this.state.view === VIEWS.LOGIN) { const Login = sdk.getComponent('structures.login.Login'); return ( ); } + + throw new Error(`Unknown view ${this.state.view}`); }, }); From 5e5639b7307e006a81c59545e91f7d53c4682b98 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 16 Jun 2017 11:50:53 +0100 Subject: [PATCH 4/6] Fix half-written comment --- src/components/structures/MatrixChat.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index c63a0cfaca..3dbde90141 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -65,6 +65,7 @@ const VIEWS = { // initial re-animation/guest registration, or via a registration), and are // now setting up a matrixclient to talk to it. This isn't an instant // process because (a) we need to clear out indexeddb, and (b) we need to + // talk to the team server; while it is going on we show a big spinner. LOGGING_IN: 5, // we are logged in with an active matrix client. From 3884c5ccf061411843e17f1305d2fd6dbe7f5693 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 16 Jun 2017 11:51:12 +0100 Subject: [PATCH 5/6] Log an error on unknown state instead of throwing --- src/components/structures/MatrixChat.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 3dbde90141..2fde44aaeb 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -1476,6 +1476,6 @@ module.exports = React.createClass({ ); } - throw new Error(`Unknown view ${this.state.view}`); + console.error(`Unknown view ${this.state.view}`); }, }); From db3d9c057349099e7186e8ab51d76f5e24a0a972 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 16 Jun 2017 12:20:52 +0100 Subject: [PATCH 6/6] Make Lifecycle.loadSession return an explicit result - rather than inferring it from the fact it didn't call logging_in. --- src/Lifecycle.js | 10 +++++++--- src/components/structures/MatrixChat.js | 24 ++++++------------------ 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index 1c5d35a642..3733ba1ea5 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -62,6 +62,8 @@ import { _t } from './languageHandler'; * true; defines the IS to use. * * @returns {Promise} a promise which resolves when the above process completes. + * Resolves to `true` if we ended up starting a session, or `false` if we + * failed. */ export function loadSession(opts) { const fragmentQueryParams = opts.fragmentQueryParams || {}; @@ -86,12 +88,12 @@ export function loadSession(opts) { homeserverUrl: guestHsUrl, identityServerUrl: guestIsUrl, guest: true, - }, true); + }, true).then(() => true); } return _restoreFromLocalStorage().then((success) => { if (success) { - return; + return true; } if (enableGuest) { @@ -99,6 +101,7 @@ export function loadSession(opts) { } // fall back to login screen + return false; }); } @@ -176,9 +179,10 @@ function _registerAsGuest(hsUrl, isUrl, defaultDeviceDisplayName) { homeserverUrl: hsUrl, identityServerUrl: isUrl, guest: true, - }, true); + }, true).then(() => true); }, (err) => { console.error("Failed to register as guest: " + err + " " + err.data); + return false; }); } diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 2c95386811..831fcdd9b2 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -335,10 +335,12 @@ module.exports = React.createClass({ }); }).catch((e) => { console.error("Unable to load session", e); - }).then(()=>{ - // stuff this through the dispatcher so that it happens - // after the on_logged_in action. - dis.dispatch({action: 'load_completed'}); + return false; + }).then((loadedSession) => { + if (!loadedSession) { + // fall back to showing the login screen + dis.dispatch({action: "start_login"}); + } }); }).done(); }, @@ -560,9 +562,6 @@ module.exports = React.createClass({ case 'will_start_client': this._onWillStartClient(); break; - case 'load_completed': - this._onLoadCompleted(); - break; case 'new_version': this.onVersion( payload.currentVersion, payload.newVersion, @@ -892,17 +891,6 @@ module.exports = React.createClass({ }); }, - /** - * Called when the sessionloader has finished - */ - _onLoadCompleted: function() { - // if we've got this far without leaving the 'loading' view, then - // login must have failed, so start the login process - if (this.state.view === VIEWS.LOADING) { - dis.dispatch({action: "start_login"}); - } - }, - /** * Called whenever someone changes the theme *