From aa0b94847b088c83bc3a4ea242420c3ef98c6598 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 1 Sep 2017 16:17:22 +0100 Subject: [PATCH 01/26] Prepare changelog for v0.10.3-rc.1 --- CHANGELOG.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5e596144e..f4e9ae9a78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,36 @@ +Changes in [0.10.3-rc.1](https://github.com/matrix-org/matrix-react-sdk/releases/tag/v0.10.3-rc.1) (2017-09-01) +=============================================================================================================== +[Full Changelog](https://github.com/matrix-org/matrix-react-sdk/compare/v0.10.2...v0.10.3-rc.1) + + * Fix room change sometimes being very slow + [\#1354](https://github.com/matrix-org/matrix-react-sdk/pull/1354) + * apply shouldHideEvent fn to onRoomTimeline for RoomStatusBar + [\#1346](https://github.com/matrix-org/matrix-react-sdk/pull/1346) + * text4event widget modified, used to show widget added each time. + [\#1345](https://github.com/matrix-org/matrix-react-sdk/pull/1345) + * separate concepts of showing and managing RRs to fix regression + [\#1352](https://github.com/matrix-org/matrix-react-sdk/pull/1352) + * Make staging widgets work with live and vice versa. + [\#1350](https://github.com/matrix-org/matrix-react-sdk/pull/1350) + * Avoid breaking /sync with uncaught exceptions + [\#1349](https://github.com/matrix-org/matrix-react-sdk/pull/1349) + * we need to pass whether it is an invite RoomSubList explicitly (i18n) + [\#1343](https://github.com/matrix-org/matrix-react-sdk/pull/1343) + * Percent encoding isn't a valid thing within _t + [\#1348](https://github.com/matrix-org/matrix-react-sdk/pull/1348) + * Fix spurious notifications + [\#1339](https://github.com/matrix-org/matrix-react-sdk/pull/1339) + * Unbreak password reset with a non-default HS + [\#1347](https://github.com/matrix-org/matrix-react-sdk/pull/1347) + * Remove unnecessary 'load' on notif audio element + [\#1341](https://github.com/matrix-org/matrix-react-sdk/pull/1341) + * _tJsx returns a React Object, the sub fn must return a React Object + [\#1340](https://github.com/matrix-org/matrix-react-sdk/pull/1340) + * Fix deprecation warning about promise.defer() + [\#1292](https://github.com/matrix-org/matrix-react-sdk/pull/1292) + * Fix click to insert completion + [\#1331](https://github.com/matrix-org/matrix-react-sdk/pull/1331) + Changes in [0.10.2](https://github.com/matrix-org/matrix-react-sdk/releases/tag/v0.10.2) (2017-08-24) ===================================================================================================== [Full Changelog](https://github.com/matrix-org/matrix-react-sdk/compare/v0.10.1...v0.10.2) From c07362d7c159c83f1ea0568765e13baa78b4cb29 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 1 Sep 2017 16:17:22 +0100 Subject: [PATCH 02/26] v0.10.3-rc.1 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index cc352cb8db..0610d42c0d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "matrix-react-sdk", - "version": "0.10.2", + "version": "0.10.3-rc.1", "description": "SDK for matrix.org using React", "author": "matrix.org", "repository": { From c67a6e9d2ec71a6bbb743e928d8a8994ee93137c Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 5 Sep 2017 13:13:02 +0100 Subject: [PATCH 03/26] Prepare changelog for v0.10.3-rc.2 --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4e9ae9a78..af82ef2e1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +Changes in [0.10.3-rc.2](https://github.com/matrix-org/matrix-react-sdk/releases/tag/v0.10.3-rc.2) (2017-09-05) +=============================================================================================================== +[Full Changelog](https://github.com/matrix-org/matrix-react-sdk/compare/v0.10.3-rc.1...v0.10.3-rc.2) + + * Fix plurals in translations + [\#1358](https://github.com/matrix-org/matrix-react-sdk/pull/1358) + * Fix typo + [\#1357](https://github.com/matrix-org/matrix-react-sdk/pull/1357) + * Update from Weblate. + [\#1356](https://github.com/matrix-org/matrix-react-sdk/pull/1356) + Changes in [0.10.3-rc.1](https://github.com/matrix-org/matrix-react-sdk/releases/tag/v0.10.3-rc.1) (2017-09-01) =============================================================================================================== [Full Changelog](https://github.com/matrix-org/matrix-react-sdk/compare/v0.10.2...v0.10.3-rc.1) From c75bc42585c8aa5e8e193ca7f6096d83734f921a Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 5 Sep 2017 13:13:02 +0100 Subject: [PATCH 04/26] v0.10.3-rc.2 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 0610d42c0d..e1fbfb58d7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "matrix-react-sdk", - "version": "0.10.3-rc.1", + "version": "0.10.3-rc.2", "description": "SDK for matrix.org using React", "author": "matrix.org", "repository": { From 9dd98d1085dbd807742613934e4d722890884d63 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Sep 2017 13:25:57 +0100 Subject: [PATCH 05/26] Prepare changelog for v0.10.3 --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index af82ef2e1f..090f5a49da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +Changes in [0.10.3](https://github.com/matrix-org/matrix-react-sdk/releases/tag/v0.10.3) (2017-09-06) +===================================================================================================== +[Full Changelog](https://github.com/matrix-org/matrix-react-sdk/compare/v0.10.3-rc.2...v0.10.3) + + * No changes + Changes in [0.10.3-rc.2](https://github.com/matrix-org/matrix-react-sdk/releases/tag/v0.10.3-rc.2) (2017-09-05) =============================================================================================================== [Full Changelog](https://github.com/matrix-org/matrix-react-sdk/compare/v0.10.3-rc.1...v0.10.3-rc.2) From f5cf2aece2ac39a172b2b08adc6b66c006c4790c Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Sep 2017 13:25:58 +0100 Subject: [PATCH 06/26] v0.10.3 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e1fbfb58d7..38a647ff67 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "matrix-react-sdk", - "version": "0.10.3-rc.2", + "version": "0.10.3", "description": "SDK for matrix.org using React", "author": "matrix.org", "repository": { From 609d61d53c55eae0660fb01724bb51518ec5f46c Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Sep 2017 17:40:58 +0100 Subject: [PATCH 07/26] Revert "Implement sticky date separators" --- package.json | 1 - src/DateUtils.js | 44 --- src/components/structures/MessagePanel.js | 9 +- src/components/structures/ScrollPanel.js | 278 +++++++++--------- src/components/structures/TimelinePanel.js | 1 + .../structures/TimelinePanel-test.js | 3 + 6 files changed, 143 insertions(+), 193 deletions(-) diff --git a/package.json b/package.json index bb7f33492d..38a647ff67 100644 --- a/package.json +++ b/package.json @@ -73,7 +73,6 @@ "react-addons-css-transition-group": "15.3.2", "react-dom": "^15.4.0", "react-gemini-scrollbar": "matrix-org/react-gemini-scrollbar#5e97aef", - "react-sticky": "^6.0.1", "sanitize-html": "^1.14.1", "text-encoding-utf-8": "^1.0.1", "url": "^0.11.0", diff --git a/src/DateUtils.js b/src/DateUtils.js index e7be394c17..78eef57eae 100644 --- a/src/DateUtils.js +++ b/src/DateUtils.js @@ -30,18 +30,6 @@ function getDaysArray() { ]; } -function getLongDaysArray() { - return [ - _t('Sunday'), - _t('Monday'), - _t('Tuesday'), - _t('Wednesday'), - _t('Thursday'), - _t('Friday'), - _t('Saturday'), - ]; -} - function getMonthsArray() { return [ _t('Jan'), @@ -108,38 +96,6 @@ module.exports = { }); }, - formatDateSeparator: function(date) { - const days = getDaysArray(); - const longDays = getLongDaysArray(); - const months = getMonthsArray(); - - const today = new Date(); - const yesterday = new Date(); - yesterday.setDate(today.getDate() - 1); - - if (date.toDateString() === today.toDateString()) { - return _t('Today'); - } else if (date.toDateString() === yesterday.toDateString()) { - return _t('Yesterday'); - } else if (today.getTime() - date.getTime() < 6 * 24 * 60 * 60 * 1000) { - return longDays[date.getDay()]; - } else if (today.getTime() - date.getTime() < 365 * 24 * 60 * 60 * 1000) { - return _t('%(weekDayName)s, %(monthName)s %(day)s', { - weekDayName: days[date.getDay()], - monthName: months[date.getMonth()], - day: date.getDate(), - fullYear: date.getFullYear(), - }); - } else { - return _t('%(weekDayName)s, %(monthName)s %(day)s %(fullYear)s', { - weekDayName: days[date.getDay()], - monthName: months[date.getMonth()], - day: date.getDate(), - fullYear: date.getFullYear(), - }); - } - }, - formatTime: function(date, showTwelveHour=false) { if (showTwelveHour) { return twelveHourTime(date); diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index aad4e1957a..e5884973c6 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -61,6 +61,9 @@ module.exports = React.createClass({ // for pending messages. ourUserId: React.PropTypes.string, + // true to suppress the date at the start of the timeline + suppressFirstDateSeparator: React.PropTypes.bool, + // whether to show read receipts showReadReceipts: React.PropTypes.bool, @@ -514,10 +517,10 @@ module.exports = React.createClass({ _wantsDateSeparator: function(prevEvent, nextEventDate) { if (prevEvent == null) { - // First event in the panel always wants a DateSeparator - return true; + // first event in the panel: depends if we could back-paginate from + // here. + return !this.props.suppressFirstDateSeparator; } - const prevEventDate = prevEvent.getDate(); if (!nextEventDate || !prevEventDate) { return false; diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 3ea699798e..a8a2ec181b 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -17,7 +17,6 @@ limitations under the License. var React = require("react"); var ReactDOM = require("react-dom"); var GeminiScrollbar = require('react-gemini-scrollbar'); -import { StickyContainer } from 'react-sticky'; import Promise from 'bluebird'; var KeyCode = require('../../KeyCode'); @@ -78,52 +77,111 @@ if (DEBUG_SCROLL) { * scroll down further. If stickyBottom is disabled, we just save the scroll * offset as normal. */ -export default class ScrollPanel extends StickyContainer { +module.exports = React.createClass({ + displayName: 'ScrollPanel', - constructor() { - super(); - this.onResize = this.onResize.bind(this); - this.onScroll = this.onScroll.bind(this); - } + propTypes: { + /* stickyBottom: if set to true, then once the user hits the bottom of + * the list, any new children added to the list will cause the list to + * scroll down to show the new element, rather than preserving the + * existing view. + */ + stickyBottom: React.PropTypes.bool, - componentWillMount() { + /* startAtBottom: if set to true, the view is assumed to start + * scrolled to the bottom. + * XXX: It's likley this is unecessary and can be derived from + * stickyBottom, but I'm adding an extra parameter to ensure + * behaviour stays the same for other uses of ScrollPanel. + * If so, let's remove this parameter down the line. + */ + startAtBottom: React.PropTypes.bool, + + /* onFillRequest(backwards): a callback which is called on scroll when + * the user nears the start (backwards = true) or end (backwards = + * false) of the list. + * + * This should return a promise; no more calls will be made until the + * promise completes. + * + * The promise should resolve to true if there is more data to be + * retrieved in this direction (in which case onFillRequest may be + * called again immediately), or false if there is no more data in this + * directon (at this time) - which will stop the pagination cycle until + * the user scrolls again. + */ + onFillRequest: React.PropTypes.func, + + /* onUnfillRequest(backwards): a callback which is called on scroll when + * there are children elements that are far out of view and could be removed + * without causing pagination to occur. + * + * This function should accept a boolean, which is true to indicate the back/top + * of the panel and false otherwise, and a scroll token, which refers to the + * first element to remove if removing from the front/bottom, and last element + * to remove if removing from the back/top. + */ + onUnfillRequest: React.PropTypes.func, + + /* onScroll: a callback which is called whenever any scroll happens. + */ + onScroll: React.PropTypes.func, + + /* onResize: a callback which is called whenever the Gemini scroll + * panel is resized + */ + onResize: React.PropTypes.func, + + /* className: classnames to add to the top-level div + */ + className: React.PropTypes.string, + + /* style: styles to add to the top-level div + */ + style: React.PropTypes.object, + }, + + getDefaultProps: function() { + return { + stickyBottom: true, + startAtBottom: true, + onFillRequest: function(backwards) { return Promise.resolve(false); }, + onUnfillRequest: function(backwards, scrollToken) {}, + onScroll: function() {}, + }; + }, + + componentWillMount: function() { this._pendingFillRequests = {b: null, f: null}; this.resetScrollState(); - } + }, - componentDidMount() { + componentDidMount: function() { this.checkFillState(); - } + }, - componentDidUpdate() { + componentDidUpdate: function() { // after adding event tiles, we may need to tweak the scroll (either to // keep at the bottom of the timeline, or to maintain the view after // adding events to the top). // // This will also re-check the fill state, in case the paginate was inadequate this.checkScroll(); - } + }, - componentWillUnmount() { + componentWillUnmount: function() { // set a boolean to say we've been unmounted, which any pending // promises can use to throw away their results. // // (We could use isMounted(), but facebook have deprecated that.) this.unmounted = true; - } + }, - onScroll(ev) { + onScroll: function(ev) { var sn = this._getScrollNode(); debuglog("Scroll event: offset now:", sn.scrollTop, "_lastSetScroll:", this._lastSetScroll); - // Set the node and notify subscribers of the StickyContainer - // By extending StickyContainer, we can set the scroll node to be that of the - // ScrolPanel to allow any `` children to be sticky, namely DateSeparators. - this.node = sn; - // Update subscribers - arbitrarily nested `` children - this.notifySubscribers(ev); - // Sometimes we see attempts to write to scrollTop essentially being // ignored. (Or rather, it is successfully written, but on the next // scroll event, it's been reset again). @@ -159,27 +217,27 @@ export default class ScrollPanel extends StickyContainer { this.props.onScroll(ev); this.checkFillState(); - } + }, - onResize() { + onResize: function() { this.props.onResize(); this.checkScroll(); this.refs.geminiPanel.forceUpdate(); - } + }, // after an update to the contents of the panel, check that the scroll is // where it ought to be, and set off pagination requests if necessary. - checkScroll() { + checkScroll: function() { this._restoreSavedScrollState(); this.checkFillState(); - } + }, // return true if the content is fully scrolled down right now; else false. // // note that this is independent of the 'stuckAtBottom' state - it is simply // about whether the the content is scrolled down right now, irrespective of // whether it will stay that way when the children update. - isAtBottom() { + isAtBottom: function() { var sn = this._getScrollNode(); // there seems to be some bug with flexbox/gemini/chrome/richvdh's @@ -189,7 +247,7 @@ export default class ScrollPanel extends StickyContainer { // that we're at the bottom when we're still a few pixels off. return sn.scrollHeight - Math.ceil(sn.scrollTop) <= sn.clientHeight + 3; - } + }, // returns the vertical height in the given direction that can be removed from // the content box (which has a height of scrollHeight, see checkFillState) without @@ -222,17 +280,17 @@ export default class ScrollPanel extends StickyContainer { // |#########| - | // |#########| | // `---------' - - _getExcessHeight(backwards) { + _getExcessHeight: function(backwards) { var sn = this._getScrollNode(); if (backwards) { return sn.scrollTop - sn.clientHeight - UNPAGINATION_PADDING; } else { return sn.scrollHeight - (sn.scrollTop + 2*sn.clientHeight) - UNPAGINATION_PADDING; } - } + }, // check the scroll state and send out backfill requests if necessary. - checkFillState() { + checkFillState: function() { if (this.unmounted) { return; } @@ -271,10 +329,10 @@ export default class ScrollPanel extends StickyContainer { // need to forward-fill this._maybeFill(false); } - } + }, // check if unfilling is possible and send an unfill request if necessary - _checkUnfillState(backwards) { + _checkUnfillState: function(backwards) { let excessHeight = this._getExcessHeight(backwards); if (excessHeight <= 0) { return; @@ -315,10 +373,10 @@ export default class ScrollPanel extends StickyContainer { this.props.onUnfillRequest(backwards, markerScrollToken); }, UNFILL_REQUEST_DEBOUNCE_MS); } - } + }, // check if there is already a pending fill request. If not, set one off. - _maybeFill(backwards) { + _maybeFill: function(backwards) { var dir = backwards ? 'b' : 'f'; if (this._pendingFillRequests[dir]) { debuglog("ScrollPanel: Already a "+dir+" fill in progress - not starting another"); @@ -350,7 +408,7 @@ export default class ScrollPanel extends StickyContainer { this.checkFillState(); } }).done(); - } + }, /* get the current scroll state. This returns an object with the following * properties: @@ -366,9 +424,9 @@ export default class ScrollPanel extends StickyContainer { * the number of pixels the bottom of the tracked child is above the * bottom of the scroll panel. */ - getScrollState() { + getScrollState: function() { return this.scrollState; - } + }, /* reset the saved scroll state. * @@ -382,46 +440,46 @@ export default class ScrollPanel extends StickyContainer { * no use if no children exist yet, or if you are about to replace the * child list.) */ - resetScrollState() { + resetScrollState: function() { this.scrollState = {stuckAtBottom: this.props.startAtBottom}; - } + }, /** * jump to the top of the content. */ - scrollToTop() { + scrollToTop: function() { this._setScrollTop(0); this._saveScrollState(); - } + }, /** * jump to the bottom of the content. */ - scrollToBottom() { + scrollToBottom: function() { // the easiest way to make sure that the scroll state is correctly // saved is to do the scroll, then save the updated state. (Calculating // it ourselves is hard, and we can't rely on an onScroll callback // happening, since there may be no user-visible change here). this._setScrollTop(Number.MAX_VALUE); this._saveScrollState(); - } + }, /** * Page up/down. * * mult: -1 to page up, +1 to page down */ - scrollRelative(mult) { + scrollRelative: function(mult) { var scrollNode = this._getScrollNode(); var delta = mult * scrollNode.clientHeight * 0.5; this._setScrollTop(scrollNode.scrollTop + delta); this._saveScrollState(); - } + }, /** * Scroll up/down in response to a scroll key */ - handleScrollKey(ev) { + handleScrollKey: function(ev) { switch (ev.keyCode) { case KeyCode.PAGE_UP: if (!ev.ctrlKey && !ev.shiftKey && !ev.altKey && !ev.metaKey) { @@ -447,7 +505,7 @@ export default class ScrollPanel extends StickyContainer { } break; } - } + }, /* Scroll the panel to bring the DOM node with the scroll token * `scrollToken` into view. @@ -460,7 +518,7 @@ export default class ScrollPanel extends StickyContainer { * node (specifically, the bottom of it) will be positioned. If omitted, it * defaults to 0. */ - scrollToToken(scrollToken, pixelOffset, offsetBase) { + scrollToToken: function(scrollToken, pixelOffset, offsetBase) { pixelOffset = pixelOffset || 0; offsetBase = offsetBase || 0; @@ -482,11 +540,11 @@ export default class ScrollPanel extends StickyContainer { // ... then make it so. this._restoreSavedScrollState(); - } + }, // set the scrollTop attribute appropriately to position the given child at the // given offset in the window. A helper for _restoreSavedScrollState. - _scrollToToken(scrollToken, pixelOffset) { + _scrollToToken: function(scrollToken, pixelOffset) { /* find the dom node with the right scrolltoken */ var node; var messages = this.refs.itemlist.children; @@ -518,9 +576,9 @@ export default class ScrollPanel extends StickyContainer { this._setScrollTop(scrollNode.scrollTop + scrollDelta); } - } + }, - _saveScrollState() { + _saveScrollState: function() { if (this.props.stickyBottom && this.isAtBottom()) { this.scrollState = { stuckAtBottom: true }; debuglog("ScrollPanel: Saved scroll state", this.scrollState); @@ -558,9 +616,9 @@ export default class ScrollPanel extends StickyContainer { } else { debuglog("ScrollPanel: unable to save scroll state: found no children in the viewport"); } - } + }, - _restoreSavedScrollState() { + _restoreSavedScrollState: function() { var scrollState = this.scrollState; var scrollNode = this._getScrollNode(); @@ -570,9 +628,9 @@ export default class ScrollPanel extends StickyContainer { this._scrollToToken(scrollState.trackedScrollToken, scrollState.pixelOffset); } - } + }, - _setScrollTop(scrollTop) { + _setScrollTop: function(scrollTop) { var scrollNode = this._getScrollNode(); var prevScroll = scrollNode.scrollTop; @@ -594,12 +652,12 @@ export default class ScrollPanel extends StickyContainer { debuglog("ScrollPanel: set scrollTop:", scrollNode.scrollTop, "requested:", scrollTop, "_lastSetScroll:", this._lastSetScroll); - } + }, /* get the DOM node which has the scrollTop property we care about for our * message panel. */ - _getScrollNode() { + _getScrollNode: function() { if (this.unmounted) { // this shouldn't happen, but when it does, turn the NPE into // something more meaningful. @@ -607,91 +665,21 @@ export default class ScrollPanel extends StickyContainer { } return this.refs.geminiPanel.scrollbar.getViewElement(); - } + }, - render() { + render: function() { // TODO: the classnames on the div and ol could do with being updated to // reflect the fact that we don't necessarily contain a list of messages. // it's not obvious why we have a separate div and ol anyway. - return ( - -
-
    - {this.props.children} -
-
- - ); - } -} - -ScrollPanel.propTypes = { - /* stickyBottom: if set to true, then once the user hits the bottom of - * the list, any new children added to the list will cause the list to - * scroll down to show the new element, rather than preserving the - * existing view. - */ - stickyBottom: React.PropTypes.bool, - - /* startAtBottom: if set to true, the view is assumed to start - * scrolled to the bottom. - * XXX: It's likley this is unecessary and can be derived from - * stickyBottom, but I'm adding an extra parameter to ensure - * behaviour stays the same for other uses of ScrollPanel. - * If so, let's remove this parameter down the line. - */ - startAtBottom: React.PropTypes.bool, - - /* onFillRequest(backwards): a callback which is called on scroll when - * the user nears the start (backwards = true) or end (backwards = - * false) of the list. - * - * This should return a promise; no more calls will be made until the - * promise completes. - * - * The promise should resolve to true if there is more data to be - * retrieved in this direction (in which case onFillRequest may be - * called again immediately), or false if there is no more data in this - * directon (at this time) - which will stop the pagination cycle until - * the user scrolls again. - */ - onFillRequest: React.PropTypes.func, - - /* onUnfillRequest(backwards): a callback which is called on scroll when - * there are children elements that are far out of view and could be removed - * without causing pagination to occur. - * - * This function should accept a boolean, which is true to indicate the back/top - * of the panel and false otherwise, and a scroll token, which refers to the - * first element to remove if removing from the front/bottom, and last element - * to remove if removing from the back/top. - */ - onUnfillRequest: React.PropTypes.func, - - /* onScroll: a callback which is called whenever any scroll happens. - */ - onScroll: React.PropTypes.func, - - /* onResize: a callback which is called whenever the Gemini scroll - * panel is resized - */ - onResize: React.PropTypes.func, - - /* className: classnames to add to the top-level div - */ - className: React.PropTypes.string, - - /* style: styles to add to the top-level div - */ - style: React.PropTypes.object, -}; - -ScrollPanel.defaultProps = { - stickyBottom: true, - startAtBottom: true, - onFillRequest: function(backwards) { return Promise.resolve(false); }, - onUnfillRequest: function(backwards, scrollToken) {}, - onScroll: function() {}, -}; + return ( +
+
    + {this.props.children} +
+
+ + ); + }, +}); diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index ebd9784b6f..862c3f46d0 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -1147,6 +1147,7 @@ var TimelinePanel = React.createClass({ highlightedEventId={ this.props.highlightedEventId } readMarkerEventId={ this.state.readMarkerEventId } readMarkerVisible={ this.state.readMarkerVisible } + suppressFirstDateSeparator={ this.state.canBackPaginate } showUrlPreview={ this.props.showUrlPreview } showReadReceipts={ this.props.showReadReceipts } ourUserId={ MatrixClientPeg.get().credentials.userId } diff --git a/test/components/structures/TimelinePanel-test.js b/test/components/structures/TimelinePanel-test.js index c13d149ed0..98ec65b8e8 100644 --- a/test/components/structures/TimelinePanel-test.js +++ b/test/components/structures/TimelinePanel-test.js @@ -234,6 +234,7 @@ describe('TimelinePanel', function() { // 5 times, and we should have given up paginating expect(client.paginateEventTimeline.callCount).toEqual(5); expect(messagePanel.props.backPaginating).toBe(false); + expect(messagePanel.props.suppressFirstDateSeparator).toBe(false); // now, if we update the events, there shouldn't be any // more requests. @@ -338,6 +339,7 @@ describe('TimelinePanel', function() { awaitScroll().then(() => { // we should now have loaded the first few events expect(messagePanel.props.backPaginating).toBe(false); + expect(messagePanel.props.suppressFirstDateSeparator).toBe(true); // back-paginate until we hit the start return backPaginate(); @@ -345,6 +347,7 @@ describe('TimelinePanel', function() { // hopefully, we got to the start of the timeline expect(messagePanel.props.backPaginating).toBe(false); + expect(messagePanel.props.suppressFirstDateSeparator).toBe(false); var events = scryEventTiles(panel); expect(events[0].props.mxEvent).toBe(timeline.getEvents()[0]); From d71f15adf4d65faf0ba06b9ec72cccdde2ed785c Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Sep 2017 22:51:10 +0100 Subject: [PATCH 08/26] Remove unused scrollStateMap from LoggedinView --- src/components/structures/LoggedInView.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/components/structures/LoggedInView.js b/src/components/structures/LoggedInView.js index 0790a5766e..147707b6fc 100644 --- a/src/components/structures/LoggedInView.js +++ b/src/components/structures/LoggedInView.js @@ -81,10 +81,6 @@ export default React.createClass({ // stash the MatrixClient in case we log out before we are unmounted this._matrixClient = this.props.matrixClient; - // _scrollStateMap is a map from room id to the scroll state returned by - // RoomView.getScrollState() - this._scrollStateMap = {}; - CallMediaHandler.loadDevices(); document.addEventListener('keydown', this._onKeyDown); @@ -116,10 +112,6 @@ export default React.createClass({ return Boolean(MatrixClientPeg.get()); }, - getScrollStateForRoom: function(roomId) { - return this._scrollStateMap[roomId]; - }, - canResetTimelineInRoom: function(roomId) { if (!this.refs.roomView) { return true; @@ -248,7 +240,6 @@ export default React.createClass({ opacity={this.props.middleOpacity} collapsedRhs={this.props.collapse_rhs} ConferenceHandler={this.props.ConferenceHandler} - scrollStateMap={this._scrollStateMap} />; if (!this.props.collapse_rhs) right_panel = ; break; From 408b8c18ea89d0c2fa714f4e34ec82e455273d42 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 7 Sep 2017 17:08:36 +0100 Subject: [PATCH 09/26] Introduce a RoomScrollStateStore to keep the place we're scrolled to in rooms. This mainly eleimates the extra, superfluous onRoomViewStoreUpdate callback that happened when the previous room saved back its scroll state. Moving the scroll state to a separate store means we can have this not emit events because nothing needs to know when the scroll state changes. --- src/components/structures/RoomView.js | 42 ++++++++++++++------------- src/stores/RoomScrollStateStore.js | 37 +++++++++++++++++++++++ src/stores/RoomViewStore.js | 30 ------------------- 3 files changed, 59 insertions(+), 50 deletions(-) create mode 100644 src/stores/RoomScrollStateStore.js diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 87bed1ed08..02a333ee82 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -47,6 +47,7 @@ import KeyCode from '../../KeyCode'; import UserProvider from '../../autocomplete/UserProvider'; import RoomViewStore from '../../stores/RoomViewStore'; +import RoomScrollStateStore from '../../stores/RoomScrollStateStore'; let DEBUG = false; let debuglog = function() {}; @@ -163,7 +164,6 @@ module.exports = React.createClass({ roomLoadError: RoomViewStore.getRoomLoadError(), joining: RoomViewStore.isJoining(), initialEventId: RoomViewStore.getInitialEventId(), - initialEventPixelOffset: RoomViewStore.getInitialEventPixelOffset(), isInitialEventHighlighted: RoomViewStore.isInitialEventHighlighted(), forwardingEvent: RoomViewStore.getForwardingEvent(), shouldPeek: RoomViewStore.shouldPeek(), @@ -191,18 +191,32 @@ module.exports = React.createClass({ newState.room = MatrixClientPeg.get().getRoom(newState.roomId); } + if (this.state.roomId !== newState.roomId) { + // Store the scroll state for the previous room so that we can return to this + // position when viewing this room in future. + if (this.state.roomId) { + RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); + } + + // ...and get the scroll state for the new room + + // If an event ID wasn't specified, default to the one saved for this room + // via update_scroll_state. Assume initialEventPixelOffset should be set. + if (!newState.initialEventId) { + const roomScrollState = RoomScrollStateStore.getScrollState(newState.roomId); + if (roomScrollState) { + newState.initialEventId = roomScrollState.focussedEvent; + newState.initialEventPixelOffset = roomScrollState.pixelOffset; + } + } + } + // Clear the search results when clicking a search result (which changes the // currently scrolled to event, this.state.initialEventId). if (this.state.initialEventId !== newState.initialEventId) { newState.searchResults = null; } - // Store the scroll state for the previous room so that we can return to this - // position when viewing this room in future. - if (this.state.roomId !== newState.roomId) { - this._updateScrollMap(this.state.roomId); - } - this.setState(newState, () => { // At this point, this.state.roomId could be null (e.g. the alias might not // have been resolved yet) so anything called here must handle this case. @@ -340,7 +354,7 @@ module.exports = React.createClass({ this.unmounted = true; // update the scroll map before we get unmounted - this._updateScrollMap(this.state.roomId); + RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); if (this.refs.roomView) { // disconnect the D&D event listeners from the room view. This @@ -617,18 +631,6 @@ module.exports = React.createClass({ }); }, - _updateScrollMap(roomId) { - // No point updating scroll state if the room ID hasn't been resolved yet - if (!roomId) { - return; - } - dis.dispatch({ - action: 'update_scroll_state', - room_id: roomId, - scroll_state: this._getScrollState(), - }); - }, - onRoom: function(room) { if (!room || room.roomId !== this.state.roomId) { return; diff --git a/src/stores/RoomScrollStateStore.js b/src/stores/RoomScrollStateStore.js new file mode 100644 index 0000000000..a1d46a29b8 --- /dev/null +++ b/src/stores/RoomScrollStateStore.js @@ -0,0 +1,37 @@ +/* +Copyright 2017 New Vector Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/** + * Stores where the user has scrolled to in each room + */ +class RoomScrollStateStore { + constructor() { + this._scrollStateMap = {}; + } + + getScrollState(roomId) { + return this._scrollStateMap[roomId]; + } + + setScrollState(roomId, scrollState) { + this._scrollStateMap[roomId] = scrollState; + } +} + +if (global.mx_RoomScrollStateStore === undefined) { + global.mx_RoomScrollStateStore = new RoomScrollStateStore(); +} +export default global.mx_RoomScrollStateStore; diff --git a/src/stores/RoomViewStore.js b/src/stores/RoomViewStore.js index bd9d3ea0fa..bd1fff600d 100644 --- a/src/stores/RoomViewStore.js +++ b/src/stores/RoomViewStore.js @@ -30,8 +30,6 @@ const INITIAL_STATE = { // The event to scroll to when the room is first viewed initialEventId: null, - // The offset to display the initial event at (see scrollStateMap) - initialEventPixelOffset: null, // Whether to highlight the initial event isInitialEventHighlighted: false, @@ -41,20 +39,6 @@ const INITIAL_STATE = { roomLoading: false, // Any error that has occurred during loading roomLoadError: null, - // A map from room id to scroll state. - // - // If there is no special scroll state (ie, we are following the live - // timeline), the scroll state is null. Otherwise, it is an object with - // the following properties: - // - // focussedEvent: the ID of the 'focussed' event. Typically this is - // the last event fully visible in the viewport, though if we - // have done an explicit scroll to an explicit event, it will be - // that event. - // - // pixelOffset: the number of pixels the window is scrolled down - // from the focussedEvent. - scrollStateMap: {}, forwardingEvent: null, }; @@ -115,9 +99,6 @@ class RoomViewStore extends Store { case 'on_logged_out': this.reset(); break; - case 'update_scroll_state': - this._updateScrollState(payload); - break; case 'forward_event': this._setState({ forwardingEvent: payload.event, @@ -132,7 +113,6 @@ class RoomViewStore extends Store { roomId: payload.room_id, roomAlias: payload.room_alias, initialEventId: payload.event_id, - initialEventPixelOffset: undefined, isInitialEventHighlighted: payload.highlighted, forwardingEvent: null, roomLoading: false, @@ -145,16 +125,6 @@ class RoomViewStore extends Store { newState.joining = false; } - // If an event ID wasn't specified, default to the one saved for this room - // via update_scroll_state. Assume initialEventPixelOffset should be set. - if (!newState.initialEventId) { - const roomScrollState = this._state.scrollStateMap[payload.room_id]; - if (roomScrollState) { - newState.initialEventId = roomScrollState.focussedEvent; - newState.initialEventPixelOffset = roomScrollState.pixelOffset; - } - } - if (this._state.forwardingEvent) { dis.dispatch({ action: 'send_event', From 7f44ac740346b06934850834214e6506b4a4e62a Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 7 Sep 2017 17:15:19 +0100 Subject: [PATCH 10/26] Remove now unused stuff from RoomViewStore --- src/stores/RoomViewStore.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/stores/RoomViewStore.js b/src/stores/RoomViewStore.js index bd1fff600d..17fcc97160 100644 --- a/src/stores/RoomViewStore.js +++ b/src/stores/RoomViewStore.js @@ -211,15 +211,6 @@ class RoomViewStore extends Store { }); } - _updateScrollState(payload) { - // Clobber existing scroll state for the given room ID - const newScrollStateMap = this._state.scrollStateMap; - newScrollStateMap[payload.room_id] = payload.scroll_state; - this._setState({ - scrollStateMap: newScrollStateMap, - }); - } - reset() { this._state = Object.assign({}, INITIAL_STATE); } @@ -234,11 +225,6 @@ class RoomViewStore extends Store { return this._state.initialEventId; } - // The offset to display the initial event at (see scrollStateMap) - getInitialEventPixelOffset() { - return this._state.initialEventPixelOffset; - } - // Whether to highlight the initial event isInitialEventHighlighted() { return this._state.isInitialEventHighlighted; From 82d1afcc47e7515ba093c5be56ef68f357411540 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 7 Sep 2017 17:16:32 +0100 Subject: [PATCH 11/26] Correct comment --- src/components/structures/RoomView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 02a333ee82..69760499a1 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -201,7 +201,7 @@ module.exports = React.createClass({ // ...and get the scroll state for the new room // If an event ID wasn't specified, default to the one saved for this room - // via update_scroll_state. Assume initialEventPixelOffset should be set. + // in the scroll state store. Assume initialEventPixelOffset should be set. if (!newState.initialEventId) { const roomScrollState = RoomScrollStateStore.getScrollState(newState.roomId); if (roomScrollState) { From d714291aa139b5a7b65edd3d1615f19a8027ac0f Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 13:27:14 +0100 Subject: [PATCH 12/26] Re-add doc on scroll state map structure --- src/stores/RoomScrollStateStore.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/stores/RoomScrollStateStore.js b/src/stores/RoomScrollStateStore.js index a1d46a29b8..07848283d1 100644 --- a/src/stores/RoomScrollStateStore.js +++ b/src/stores/RoomScrollStateStore.js @@ -19,6 +19,19 @@ limitations under the License. */ class RoomScrollStateStore { constructor() { + // A map from room id to scroll state. + // + // If there is no special scroll state (ie, we are following the live + // timeline), the scroll state is null. Otherwise, it is an object with + // the following properties: + // + // focussedEvent: the ID of the 'focussed' event. Typically this is + // the last event fully visible in the viewport, though if we + // have done an explicit scroll to an explicit event, it will be + // that event. + // + // pixelOffset: the number of pixels the window is scrolled down + // from the focussedEvent. this._scrollStateMap = {}; } From 59c54d3756153e3378ba2198115e946eee599159 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 13:39:22 +0100 Subject: [PATCH 13/26] Remove redundant code --- src/components/structures/RoomView.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 69760499a1..2de8496b50 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -191,14 +191,8 @@ module.exports = React.createClass({ newState.room = MatrixClientPeg.get().getRoom(newState.roomId); } - if (this.state.roomId !== newState.roomId) { - // Store the scroll state for the previous room so that we can return to this - // position when viewing this room in future. - if (this.state.roomId) { - RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); - } - - // ...and get the scroll state for the new room + if (this.state.roomId === null && newState.roomId !== null) { + // Get the scroll state for the new room // If an event ID wasn't specified, default to the one saved for this room // in the scroll state store. Assume initialEventPixelOffset should be set. @@ -354,7 +348,9 @@ module.exports = React.createClass({ this.unmounted = true; // update the scroll map before we get unmounted - RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); + if (this.state.roomId) { + RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); + } if (this.refs.roomView) { // disconnect the D&D event listeners from the room view. This From 78a2e497053df599240fcadb7922f92df3498639 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 15:41:19 +0100 Subject: [PATCH 14/26] Don't always paginate when mounting a ScrollPanel Calling just checkFill on DidMount did not initially set the scrollTop which meant that one back pagination request is always performed regardless. This meant we would end up rending the first batch of events, then paginating and re-rendering again after the pagination got another batch, causing unnecessary render churn. --- src/components/structures/ScrollPanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index a8a2ec181b..ae3ffe66e3 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -157,7 +157,7 @@ module.exports = React.createClass({ }, componentDidMount: function() { - this.checkFillState(); + this.checkScroll(); }, componentDidUpdate: function() { From 1be35a77ec996a3409e544d3e7a11ecde6652137 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 17:06:46 +0100 Subject: [PATCH 15/26] Don't wait for setState to run onHaveRoom onHaveRoom sets some more state (among other things) so putting it in the setState callback so it could observe the new state caused us to have to re-render again unnecessarily. Just give it the new state as a parameter. --- src/components/structures/RoomView.js | 29 +++++++++++++++------------ 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 2de8496b50..037cb736cd 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -211,16 +211,19 @@ module.exports = React.createClass({ newState.searchResults = null; } - this.setState(newState, () => { - // At this point, this.state.roomId could be null (e.g. the alias might not - // have been resolved yet) so anything called here must handle this case. - if (initial) { - this._onHaveRoom(); - } - }); + this.setState(newState); + // At this point, newState.roomId could be null (e.g. the alias might not + // have been resolved yet) so anything called here must handle this case. + // We pass the new state into this function for it to read: it needs to + // observe the new state but we don't want to put it in the setState + // callback because this would prevent the setStates from being batched, + // ie. cause it to render RoomView twice rather than the once that is necessary. + if (initial) { + this._onHaveRoom(newState); + } }, - _onHaveRoom: function() { + _onHaveRoom: function(state) { // if this is an unknown room then we're in one of three states: // - This is a room we can peek into (search engine) (we can /peek) // - This is a room we can publicly join or were invited to. (we can /join) @@ -236,7 +239,7 @@ module.exports = React.createClass({ // about it). We don't peek in the historical case where we were joined but are // now not joined because the js-sdk peeking API will clobber our historical room, // making it impossible to indicate a newly joined room. - const room = this.state.room; + const room = state.room; if (room) { this.setState({ unsentMessageError: this._getUnsentMessageError(room), @@ -244,15 +247,15 @@ module.exports = React.createClass({ }); this._onRoomLoaded(room); } - if (!this.state.joining && this.state.roomId) { + if (!state.joining && state.roomId) { if (this.props.autoJoin) { this.onJoinButtonClicked(); - } else if (!room && this.state.shouldPeek) { - console.log("Attempting to peek into room %s", this.state.roomId); + } else if (!room && state.shouldPeek) { + console.log("Attempting to peek into room %s", state.roomId); this.setState({ peekLoading: true, }); - MatrixClientPeg.get().peekInRoom(this.state.roomId).then((room) => { + MatrixClientPeg.get().peekInRoom(state.roomId).then((room) => { this.setState({ room: room, peekLoading: false, From 03dcded72f98108a9f59fe8bb5e01b42299112b4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 17:39:10 +0100 Subject: [PATCH 16/26] Blank line to make comment clearer --- src/components/structures/RoomView.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 037cb736cd..6b76ad4ae1 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -214,6 +214,7 @@ module.exports = React.createClass({ this.setState(newState); // At this point, newState.roomId could be null (e.g. the alias might not // have been resolved yet) so anything called here must handle this case. + // We pass the new state into this function for it to read: it needs to // observe the new state but we don't want to put it in the setState // callback because this would prevent the setStates from being batched, From 81871c50bed120eb94d217e485473f428b055052 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 17:43:41 +0100 Subject: [PATCH 17/26] Add more doc on why module level variables do not work as singletons --- src/Skinner.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Skinner.js b/src/Skinner.js index f47572ba01..1fe12f85ab 100644 --- a/src/Skinner.js +++ b/src/Skinner.js @@ -84,6 +84,9 @@ class Skinner { // behaviour with multiple copies of files etc. is erratic at best. // XXX: We can still end up with the same file twice in the resulting // JS bundle which is nonideal. +// See https://derickbailey.com/2016/03/09/creating-a-true-singleton-in-node-js-with-es6-symbols/ +// or https://nodejs.org/api/modules.html#modules_module_caching_caveats +// ("Modules are cached based on their resolved filename") if (global.mxSkinner === undefined) { global.mxSkinner = new Skinner(); } From bf982004f6e06934df18ddc0fdcd51dbd863e7c2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 17:56:53 +0100 Subject: [PATCH 18/26] Give onHaveRoom the info it needs explicitly Rather than giving it a state object which is not actually the whole state but happens to be everything it actually wants (currently) --- src/components/structures/RoomView.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 6b76ad4ae1..56c0d1d281 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -220,11 +220,11 @@ module.exports = React.createClass({ // callback because this would prevent the setStates from being batched, // ie. cause it to render RoomView twice rather than the once that is necessary. if (initial) { - this._onHaveRoom(newState); + this._onHaveRoom(newState.room, newState.roomId, newState.joining, newState.shouldPeek); } }, - _onHaveRoom: function(state) { + _onHaveRoom: function(room, roomId, joining, shouldPeek) { // if this is an unknown room then we're in one of three states: // - This is a room we can peek into (search engine) (we can /peek) // - This is a room we can publicly join or were invited to. (we can /join) @@ -240,7 +240,6 @@ module.exports = React.createClass({ // about it). We don't peek in the historical case where we were joined but are // now not joined because the js-sdk peeking API will clobber our historical room, // making it impossible to indicate a newly joined room. - const room = state.room; if (room) { this.setState({ unsentMessageError: this._getUnsentMessageError(room), @@ -248,15 +247,15 @@ module.exports = React.createClass({ }); this._onRoomLoaded(room); } - if (!state.joining && state.roomId) { + if (!joining && roomId) { if (this.props.autoJoin) { this.onJoinButtonClicked(); - } else if (!room && state.shouldPeek) { - console.log("Attempting to peek into room %s", state.roomId); + } else if (!room && shouldPeek) { + console.log("Attempting to peek into room %s", roomId); this.setState({ peekLoading: true, }); - MatrixClientPeg.get().peekInRoom(state.roomId).then((room) => { + MatrixClientPeg.get().peekInRoom(roomId).then((room) => { this.setState({ room: room, peekLoading: false, From aee2f3cdefaf4a787f7bbdfedf226f3927a10301 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 18:11:13 +0100 Subject: [PATCH 19/26] Rename onHaveRoom And move some code out of it which didn't really have any reason to be hanging out there rather than just be where we set the room a few lines above. --- src/components/structures/RoomView.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 56c0d1d281..8a0eeb50b9 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -189,6 +189,11 @@ module.exports = React.createClass({ // the RoomView instance if (initial) { newState.room = MatrixClientPeg.get().getRoom(newState.roomId); + if (newState.room) { + newState.unsentMessageError = this._getUnsentMessageError(newState.room); + newState.showApps = this._shouldShowApps(newState.room); + this._onRoomLoaded(newState.room); + } } if (this.state.roomId === null && newState.roomId !== null) { @@ -220,11 +225,11 @@ module.exports = React.createClass({ // callback because this would prevent the setStates from being batched, // ie. cause it to render RoomView twice rather than the once that is necessary. if (initial) { - this._onHaveRoom(newState.room, newState.roomId, newState.joining, newState.shouldPeek); + this._setupRoom(newState.room, newState.roomId, newState.joining, newState.shouldPeek); } }, - _onHaveRoom: function(room, roomId, joining, shouldPeek) { + _setupRoom: function(room, roomId, joining, shouldPeek) { // if this is an unknown room then we're in one of three states: // - This is a room we can peek into (search engine) (we can /peek) // - This is a room we can publicly join or were invited to. (we can /join) @@ -240,13 +245,6 @@ module.exports = React.createClass({ // about it). We don't peek in the historical case where we were joined but are // now not joined because the js-sdk peeking API will clobber our historical room, // making it impossible to indicate a newly joined room. - if (room) { - this.setState({ - unsentMessageError: this._getUnsentMessageError(room), - showApps: this._shouldShowApps(room), - }); - this._onRoomLoaded(room); - } if (!joining && roomId) { if (this.props.autoJoin) { this.onJoinButtonClicked(); From 663dc3e5131bb2e38df2ba8eba8fefb6909f87f5 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 18:56:57 +0100 Subject: [PATCH 20/26] Don't re-render matrixchat unnecessarily ...on room switch. We were setting most of the state in viewRoom, but getting the current room ID from the RoomViewStore, but this meant we did one setState from the RoomViewStore updating, re-rendered and then setState again in viewRoom causing another render. This just sets the room ID in viewRoom. --- src/components/structures/MatrixChat.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index bbe345933e..c3fae0f1b3 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -38,7 +38,6 @@ import linkifyMatrix from "../../linkify-matrix"; import * as Lifecycle from '../../Lifecycle'; // LifecycleStore is not used but does listen to and dispatch actions require('../../stores/LifecycleStore'); -import RoomViewStore from '../../stores/RoomViewStore'; import PageTypes from '../../PageTypes'; import createRoom from "../../createRoom"; @@ -214,9 +213,6 @@ module.exports = React.createClass({ componentWillMount: function() { SdkConfig.put(this.props.config); - this._roomViewStoreToken = RoomViewStore.addListener(this._onRoomViewStoreUpdated); - this._onRoomViewStoreUpdated(); - if (!UserSettingsStore.getLocalSetting('analyticsOptOut', false)) Analytics.enable(); // Used by _viewRoom before getting state from sync @@ -587,10 +583,6 @@ module.exports = React.createClass({ } }, - _onRoomViewStoreUpdated: function() { - this.setState({ currentRoomId: RoomViewStore.getRoomId() }); - }, - _setPage: function(pageType) { this.setState({ page_type: pageType, @@ -677,6 +669,7 @@ module.exports = React.createClass({ this.focusComposer = true; const newState = { + currentRoomId: roomInfo.room_id || null, page_type: PageTypes.RoomView, thirdPartyInvite: roomInfo.third_party_invite, roomOobData: roomInfo.oob_data, From 0e8bd856bc3527b46397b2173004f5cd0f9c24a6 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Fri, 8 Sep 2017 20:14:27 +0200 Subject: [PATCH 21/26] remove obsolete this._roomViewStoreToken.remove(); --- src/components/structures/MatrixChat.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index c3fae0f1b3..c142d6958c 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -349,7 +349,6 @@ module.exports = React.createClass({ UDEHandler.stopListening(); window.removeEventListener("focus", this.onFocus); window.removeEventListener('resize', this.handleResize); - this._roomViewStoreToken.remove(); }, componentDidUpdate: function() { From ec3ff529e7ce4985f1b553c48d7175c7d1097b5a Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 23:05:27 +0100 Subject: [PATCH 22/26] Fast path for emojifying strings Emojione's regex for detecting emoji is *enourmous* and we were running it on every display name, room name, message etc every time those components mounted. Add a much simpler regex to rule out the majority of strings that contain no emoji and fast-path them. Makes room switching about 10% faster (in my tests with all the profiling turned on). --- src/HtmlUtils.js | 15 +++++++++++++++ src/components/views/elements/EmojiText.js | 13 ++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index 87e714083b..a046a94363 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -31,13 +31,28 @@ emojione.imagePathPNG = 'emojione/png/'; // Use SVGs for emojis emojione.imageType = 'svg'; +const SIMPLE_EMOJI_PATTERN = /([\ud800-\udbff])([\udc00-\udfff])/; const EMOJI_REGEX = new RegExp(emojione.unicodeRegexp+"+", "gi"); const COLOR_REGEX = /^#[0-9a-fA-F]{6}$/; +/* + * Return true if the given string contains emoji + * Uses a much, much simpler regex than emojione's so will give false + * positives, but useful for fast-path testing strings to see if they + * need emojification. + * unicodeToImage uses this function. + */ +export function containsEmoji(str) { + return SIMPLE_EMOJI_PATTERN.test(str); +} + /* modified from https://github.com/Ranks/emojione/blob/master/lib/js/emojione.js * because we want to include emoji shortnames in title text */ export function unicodeToImage(str) { + // fast path + if (!containsEmoji(str)) return str; + let replaceWith, unicode, alt, short, fname; const mappedUnicode = emojione.mapUnicodeToShort(); diff --git a/src/components/views/elements/EmojiText.js b/src/components/views/elements/EmojiText.js index cb6cd2ef5e..a48e51f44f 100644 --- a/src/components/views/elements/EmojiText.js +++ b/src/components/views/elements/EmojiText.js @@ -15,12 +15,19 @@ */ import React from 'react'; -import {emojifyText} from '../../../HtmlUtils'; +import {emojifyText, containsEmoji} from '../../../HtmlUtils'; export default function EmojiText(props) { const {element, children, ...restProps} = props; - restProps.dangerouslySetInnerHTML = emojifyText(children); - return React.createElement(element, restProps); + + // fast path: simple regex to detect strings that don't contain + // emoji and just return them + if (containsEmoji(children)) { + restProps.dangerouslySetInnerHTML = emojifyText(children); + return React.createElement(element, restProps); + } else { + return React.createElement(element, restProps, children); + } } EmojiText.propTypes = { From ea5726aa4e101afa65d2e8a87813d1ef80fd418d Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 23:14:06 +0100 Subject: [PATCH 23/26] Copyright --- src/HtmlUtils.js | 1 + src/components/views/elements/EmojiText.js | 1 + 2 files changed, 2 insertions(+) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index a046a94363..05be98f5fd 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -1,5 +1,6 @@ /* Copyright 2015, 2016 OpenMarket Ltd +Copyright 2017 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/src/components/views/elements/EmojiText.js b/src/components/views/elements/EmojiText.js index a48e51f44f..faab0241ae 100644 --- a/src/components/views/elements/EmojiText.js +++ b/src/components/views/elements/EmojiText.js @@ -1,5 +1,6 @@ /* Copyright 2016 Aviral Dasgupta + Copyright 2017 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. From fe79010e4e32604ff3685fa6f42289d0c83abd03 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 23:36:22 +0100 Subject: [PATCH 24/26] Only add the code copy button for HTML messages Trivial fast-path optimisation: plain text messages cannot possibly contain pre blocks so there's no point in trying to parse them in order to add code copy buttons. --- src/HtmlUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index 87e714083b..ccc7c1a630 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -393,7 +393,7 @@ export function bodyToHtml(content, highlights, opts) { } safeBody = sanitizeHtml(body, sanitizeHtmlParams); safeBody = unicodeToImage(safeBody); - safeBody = addCodeCopyButton(safeBody); + if (isHtml) safeBody = addCodeCopyButton(safeBody); } finally { delete sanitizeHtmlParams.textFilter; From 876257f4e219acc06bd5097dd4bd71eb5403ebd2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Sun, 10 Sep 2017 14:23:33 +0100 Subject: [PATCH 25/26] Consolidate the code copy button Adding the code code button was done by manipulating the HTML of the event body to add a span tag, then adding the onclick handler after the thing was mounted. Apart from splitting the code between two places, adding the span tag was, according to Chrome's profiler, taking up quite a lot of CPU cycles (apparently as soon as you set the innerHTML on a div). Instead, just build the whole lot together after the component mounts. --- src/HtmlUtils.js | 18 ------------------ src/components/views/messages/TextualBody.js | 15 +++++++++------ 2 files changed, 9 insertions(+), 24 deletions(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index ccc7c1a630..2b0fd686c0 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -393,7 +393,6 @@ export function bodyToHtml(content, highlights, opts) { } safeBody = sanitizeHtml(body, sanitizeHtmlParams); safeBody = unicodeToImage(safeBody); - if (isHtml) safeBody = addCodeCopyButton(safeBody); } finally { delete sanitizeHtmlParams.textFilter; @@ -412,23 +411,6 @@ export function bodyToHtml(content, highlights, opts) { return ; } -function addCodeCopyButton(safeBody) { - // Adds 'copy' buttons to pre blocks - // Note that this only manipulates the markup to add the buttons: - // we need to add the event handlers once the nodes are in the DOM - // since we can't save functions in the markup. - // This is done in TextualBody - const el = document.createElement("div"); - el.innerHTML = safeBody; - const codeBlocks = Array.from(el.getElementsByTagName("pre")); - codeBlocks.forEach(p => { - const button = document.createElement("span"); - button.className = "mx_EventTile_copyButton"; - p.appendChild(button); - }); - return el.innerHTML; -} - export function emojifyText(text) { return { __html: unicodeToImage(escape(text)), diff --git a/src/components/views/messages/TextualBody.js b/src/components/views/messages/TextualBody.js index 58273bee67..3937780c2a 100644 --- a/src/components/views/messages/TextualBody.js +++ b/src/components/views/messages/TextualBody.js @@ -113,14 +113,17 @@ module.exports = React.createClass({ } }, 10); } - // add event handlers to the 'copy code' buttons - const buttons = ReactDOM.findDOMNode(this).getElementsByClassName("mx_EventTile_copyButton"); - for (let i = 0; i < buttons.length; i++) { - buttons[i].onclick = (e) => { - const copyCode = buttons[i].parentNode.getElementsByTagName("code")[0]; + + // Add 'copy' buttons to pre blocks + ReactDOM.findDOMNode(this).querySelectorAll('.mx_EventTile_body pre').forEach((p) => { + const button = document.createElement("span"); + button.className = "mx_EventTile_copyButton"; + button.onclick = (e) => { + const copyCode = button.parentNode.getElementsByTagName("code")[0]; this.copyToClipboard(copyCode.textContent); }; - } + p.appendChild(button); + }); } }, From 05a986334d482ecd37af40c4769700fe54fdcb46 Mon Sep 17 00:00:00 2001 From: David Baker Date: Sun, 10 Sep 2017 15:58:17 +0100 Subject: [PATCH 26/26] Separate function to add code copy button For neatness and also so it can show up separately in the profiler. --- src/components/views/messages/TextualBody.js | 24 ++++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/components/views/messages/TextualBody.js b/src/components/views/messages/TextualBody.js index 3937780c2a..a58422e840 100644 --- a/src/components/views/messages/TextualBody.js +++ b/src/components/views/messages/TextualBody.js @@ -114,16 +114,7 @@ module.exports = React.createClass({ }, 10); } - // Add 'copy' buttons to pre blocks - ReactDOM.findDOMNode(this).querySelectorAll('.mx_EventTile_body pre').forEach((p) => { - const button = document.createElement("span"); - button.className = "mx_EventTile_copyButton"; - button.onclick = (e) => { - const copyCode = button.parentNode.getElementsByTagName("code")[0]; - this.copyToClipboard(copyCode.textContent); - }; - p.appendChild(button); - }); + this._addCodeCopyButton(); } }, @@ -260,6 +251,19 @@ module.exports = React.createClass({ } }, + _addCodeCopyButton() { + // Add 'copy' buttons to pre blocks + ReactDOM.findDOMNode(this).querySelectorAll('.mx_EventTile_body pre').forEach((p) => { + const button = document.createElement("span"); + button.className = "mx_EventTile_copyButton"; + button.onclick = (e) => { + const copyCode = button.parentNode.getElementsByTagName("code")[0]; + this.copyToClipboard(copyCode.textContent); + }; + p.appendChild(button); + }); + }, + onCancelClick: function(event) { this.setState({ widgetHidden: true }); // FIXME: persist this somewhere smarter than local storage