From cc72f7ec247adb682345fe2907efb4c0cc75d9ae Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 22 Dec 2015 18:15:32 +0000 Subject: [PATCH 1/9] Use new searchRoomEvents and backPaginateRoomEventsSearch methods MatrixClient now exposes higher-level search APIs, so use them. --- src/components/structures/RoomView.js | 165 ++++++++++------------- src/components/views/rooms/RoomHeader.js | 2 +- 2 files changed, 74 insertions(+), 93 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 127fe2fdb8..96bd71bd23 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -365,10 +365,9 @@ module.exports = React.createClass({ if (!backwards || this.state.searchInProgress) return; - if (this.nextSearchBatch) { + if (this.state.searchResults.next_batch) { if (DEBUG_SCROLL) console.log("requesting more search results"); - this._getSearchBatch(this.state.searchTerm, - this.state.searchScope); + this._backPaginateSearch(); } else { if (DEBUG_SCROLL) console.log("no more search results"); } @@ -484,10 +483,8 @@ module.exports = React.createClass({ this.setState({ searchTerm: term, searchScope: scope, - searchResults: [], + searchResults: {}, searchHighlights: [], - searchCount: null, - searchCanPaginate: null, }); // if we already have a search panel, we need to tell it to forget @@ -496,64 +493,72 @@ module.exports = React.createClass({ this.refs.searchResultsPanel.resetScrollState(); } - this.nextSearchBatch = null; - this._getSearchBatch(term, scope); + // make sure that we don't end up showing results from + // an aborted search by keeping a unique id. + // + // todo: should cancel any previous search requests. + this.searchId = new Date().getTime(); + + var filter; + if (scope === "Room") { + filter = { + // XXX: it's unintuitive that the filter for searching doesn't have the same shape as the v2 filter API :( + rooms: [ + this.props.roomId + ] + }; + } + + if (DEBUG_SCROLL) console.log("sending search request"); + + var searchPromise = MatrixClientPeg.get().searchRoomEvents( + { filter: filter, + term: term, + }); + this._handleSearchResult(searchPromise) + .done(); }, - // fire off a request for a batch of search results - _getSearchBatch: function(term, scope) { + _backPaginateSearch: function() { + if (DEBUG_SCROLL) console.log("sending search back-paginate request"); + + var searchPromise = MatrixClientPeg.get().backPaginateRoomEventsSearch( + this.state.searchResults); + this._handleSearchResult(searchPromise) + .done(); + }, + + _handleSearchResult: function(searchPromise) { + var self = this; + var searchId = this.searchId; + this.setState({ searchInProgress: true, }); - // make sure that we don't end up merging results from - // different searches by keeping a unique id. - // - // todo: should cancel any previous search requests. - var searchId = this.searchId = new Date().getTime(); - - var self = this; - - if (DEBUG_SCROLL) console.log("sending search request"); - MatrixClientPeg.get().search({ body: this._getSearchCondition(term, scope), - next_batch: this.nextSearchBatch }) - .then(function(data) { + return searchPromise.then(function(results) { if (DEBUG_SCROLL) console.log("search complete"); if (!self.state.searching || self.searchId != searchId) { console.error("Discarding stale search results"); return; } - var results = data.search_categories.room_events; - // postgres on synapse returns us precise details of the // strings which actually got matched for highlighting. - // combine the highlight list with our existing list; build an object - // to avoid O(N^2) fail - var highlights = {}; - results.highlights.forEach(function(hl) { highlights[hl] = 1; }); - self.state.searchHighlights.forEach(function(hl) { highlights[hl] = 1; }); - - // turn it back into an ordered list. For overlapping highlights, + // For overlapping highlights, // favour longer (more specific) terms first - highlights = Object.keys(highlights).sort(function(a, b) { b.length - a.length }); + var highlights = results.highlights.sort(function(a, b) { b.length - a.length }); // sqlite doesn't give us any highlights, so just try to highlight the literal search term if (highlights.length == 0) { - highlights = [ term ]; + highlights = [ self.state.searchTerm ]; } - // append the new results to our existing results - var events = self.state.searchResults.concat(results.results); - self.setState({ searchHighlights: highlights, - searchResults: events, - searchCount: results.count, - searchCanPaginate: !!(results.next_batch), + searchResults: results, }); - self.nextSearchBatch = results.next_batch; }, function(error) { var ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); Modal.createDialog(ErrorDialog, { @@ -564,51 +569,27 @@ module.exports = React.createClass({ self.setState({ searchInProgress: false }); - }).done(); + }); }, - _getSearchCondition: function(term, scope) { - var filter; - - if (scope === "Room") { - filter = { - // XXX: it's unintuitive that the filter for searching doesn't have the same shape as the v2 filter API :( - rooms: [ - this.props.roomId - ] - }; - } - - return { - search_categories: { - room_events: { - search_term: term, - filter: filter, - order_by: "recent", - event_context: { - before_limit: 1, - after_limit: 1, - include_profile: true, - } - } - } - } - }, getSearchResultTiles: function() { var DateSeparator = sdk.getComponent('messages.DateSeparator'); - var cli = MatrixClientPeg.get(); - - var ret = []; - var EventTile = sdk.getComponent('rooms.EventTile'); + var cli = MatrixClientPeg.get(); // XXX: todo: merge overlapping results somehow? // XXX: why doesn't searching on name work? + if (this.state.searchResults.results === undefined) { + // awaiting results + return []; + } - if (this.state.searchCanPaginate === false) { - if (this.state.searchResults.length == 0) { + var ret = []; + + if (!this.state.searchResults.next_batch) { + if (this.state.searchResults.results.length == 0) { ret.push(

  • No results

    • @@ -623,9 +604,10 @@ module.exports = React.createClass({ var lastRoomId; - for (var i = this.state.searchResults.length - 1; i >= 0; i--) { - var result = this.state.searchResults[i]; - var mxEv = new Matrix.MatrixEvent(result.result); + for (var i = this.state.searchResults.results.length - 1; i >= 0; i--) { + var result = this.state.searchResults.results[i]; + + var mxEv = result.context.getEvent(); if (!EventTile.haveTileForEvent(mxEv)) { // XXX: can this ever happen? It will make the result count @@ -636,29 +618,28 @@ module.exports = React.createClass({ var eventId = mxEv.getId(); if (this.state.searchScope === 'All') { - var roomId = result.result.room_id; + var roomId = mxEv.getRoomId(); if(roomId != lastRoomId) { ret.push(

  • Room: { cli.getRoom(roomId).name }

    • ); lastRoomId = roomId; } } - var ts1 = result.result.origin_server_ts; + var ts1 = mxEv.getTs(); ret.push(
    • ); // Rank: {resultList[i].rank} - if (result.context.events_before[0]) { - var mxEv2 = new Matrix.MatrixEvent(result.context.events_before[0]); - if (EventTile.haveTileForEvent(mxEv2)) { - ret.push(
    • ); + var timeline = result.context.getTimeline(); + for (var j = 0; j < timeline.length; j++) { + var ev = timeline[j]; + var highlights; + var contextual = (j != result.context.getOurEventIndex()); + if (!contextual) { + highlights = this.state.searchHighlights; } - } - - ret.push(
    • ); - - if (result.context.events_after[0]) { - var mxEv2 = new Matrix.MatrixEvent(result.context.events_after[0]); - if (EventTile.haveTileForEvent(mxEv2)) { - ret.push(
    • ); + if (EventTile.haveTileForEvent(ev)) { + ret.push(
  • + +
    • ); } } } @@ -1290,7 +1271,7 @@ module.exports = React.createClass({ searchInfo = { searchTerm : this.state.searchTerm, searchScope : this.state.searchScope, - searchCount : this.state.searchCount, + searchCount : this.state.searchResults.count, }; } diff --git a/src/components/views/rooms/RoomHeader.js b/src/components/views/rooms/RoomHeader.js index 13959a16b9..0183ce2116 100644 --- a/src/components/views/rooms/RoomHeader.js +++ b/src/components/views/rooms/RoomHeader.js @@ -110,7 +110,7 @@ module.exports = React.createClass({ var searchStatus; // don't display the search count until the search completes and // gives us a non-null searchCount. - if (this.props.searchInfo && this.props.searchInfo.searchCount !== null) { + if (this.props.searchInfo && this.props.searchInfo.searchCount != null) { searchStatus =
     (~{ this.props.searchInfo.searchCount } results)
    ; } From f2a24521dc4b7f9e1568755c8ea5ed529a41e12a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 24 Dec 2015 00:10:21 +0000 Subject: [PATCH 2/9] Make ScrollPanel keep track of when fill requests are happening The dance to avoid doing repeated fill requests on every update is common, so add it to ScrollPanel. Let onFillRequest return a promise, which prevents any updates until it completes. --- src/components/structures/RoomView.js | 27 +++++----- src/components/structures/ScrollPanel.js | 63 +++++++++++++++++++++++- 2 files changed, 72 insertions(+), 18 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 127fe2fdb8..fc909f69ab 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -330,8 +330,6 @@ module.exports = React.createClass({ this.scrollToBottom(); this.sendReadReceipt(); - - this.refs.messagePanel.checkFillState(); }, componentDidUpdate: function() { @@ -353,30 +351,25 @@ module.exports = React.createClass({ }); this.setState({paginating: false}); - - // we might not have got enough (or, indeed, any) results from the - // pagination request, so give the messagePanel a chance to set off - // another. - - this.refs.messagePanel.checkFillState(); }, onSearchResultsFillRequest: function(backwards) { - if (!backwards || this.state.searchInProgress) - return; + if (!backwards) + return false; if (this.nextSearchBatch) { if (DEBUG_SCROLL) console.log("requesting more search results"); - this._getSearchBatch(this.state.searchTerm, - this.state.searchScope); + return this._getSearchBatch(this.state.searchTerm, + this.state.searchScope).then(true); } else { if (DEBUG_SCROLL) console.log("no more search results"); + return false; } }, // set off a pagination request. onMessageListFillRequest: function(backwards) { - if (!backwards || this.state.paginating) + if (!backwards) return; // Either wind back the message cap (if there are enough events in the @@ -386,11 +379,13 @@ module.exports = React.createClass({ var cap = Math.min(this.state.messageCap + PAGINATE_SIZE, this.state.room.timeline.length); if (DEBUG_SCROLL) console.log("winding back message cap to", cap); this.setState({messageCap: cap}); + return true; } else if(this.state.room.oldState.paginationToken) { var cap = this.state.messageCap + PAGINATE_SIZE; if (DEBUG_SCROLL) console.log("starting paginate to cap", cap); this.setState({messageCap: cap, paginating: true}); - MatrixClientPeg.get().scrollback(this.state.room, PAGINATE_SIZE).finally(this._paginateCompleted).done(); + return MatrixClientPeg.get().scrollback(this.state.room, PAGINATE_SIZE). + finally(this._paginateCompleted).then(true); } }, @@ -515,8 +510,8 @@ module.exports = React.createClass({ var self = this; if (DEBUG_SCROLL) console.log("sending search request"); - MatrixClientPeg.get().search({ body: this._getSearchCondition(term, scope), - next_batch: this.nextSearchBatch }) + return MatrixClientPeg.get().search({ body: this._getSearchCondition(term, scope), + next_batch: this.nextSearchBatch }) .then(function(data) { if (DEBUG_SCROLL) console.log("search complete"); if (!self.state.searching || self.searchId != searchId) { diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 2c68562ada..fc0b630f26 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -17,6 +17,7 @@ limitations under the License. var React = require("react"); var ReactDOM = require("react-dom"); var GeminiScrollbar = require('react-gemini-scrollbar'); +var q = require("q"); var DEBUG_SCROLL = false; @@ -51,7 +52,15 @@ module.exports = React.createClass({ /* onFillRequest(backwards): a callback which is called on scroll when * the user nears the start (backwards = true) or end (backwards = - * false) of the list + * false) of the list. + * + * This should return true if the pagination was successful, 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. + * + * This can return a promise; if it does, no more calls will be made + * until the promise completes. The promise should resolve to true or + * false as above. */ onFillRequest: React.PropTypes.func, @@ -77,14 +86,22 @@ module.exports = React.createClass({ }, componentWillMount: function() { + this._pendingFillRequests = {b: null, f: null}; this.resetScrollState(); }, + componentDidMount: function() { + this.checkFillState(); + }, + 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._restoreSavedScrollState(); + + // we also re-check the fill state, in case the paginate was inadequate + this.checkFillState(); }, onScroll: function(ev) { @@ -131,10 +148,52 @@ module.exports = React.createClass({ if (sn.scrollTop < sn.clientHeight) { // there's less than a screenful of messages left - try to get some // more messages. - this.props.onFillRequest(true); + this._maybeFill(true); } }, + // check if there is already a pending fill request. If not, set one off. + _maybeFill: function(backwards) { + var dir = backwards ? 'b' : 'f'; + if (this._pendingFillRequests[dir]) { + if (DEBUG_SCROLL) { + console.log("ScrollPanel: Already a "+dir+" fill in progress - not starting another"); + } + return; + } + + if (DEBUG_SCROLL) { + console.log("ScrollPanel: starting "+dir+" fill"); + } + + // onFillRequest can end up calling us recursively (via onScroll + // events) so make sure we set this before firing off the call. That + // does present the risk that we might not ever actually fire off the + // fill request, so wrap it in a try/catch. + this._pendingFillRequests[dir] = true; + var r; + try { + r = this.props.onFillRequest(backwards); + } catch (e) { + this._pendingFillRequests[dir] = false; + throw e; + } + + q.finally(r, () => { + if (DEBUG_SCROLL) { + console.log("ScrollPanel: "+dir+" fill complete"); + } + this._pendingFillRequests[dir] = false; + }).then((res) => { + if (res) { + // further pagination requests have been disabled until now, so + // it's time to check the fill state again in case the pagination + // was insufficient. + this.checkFillState(); + } + }).done(); + }, + // get the current scroll position of the room, so that it can be // restored later getScrollState: function() { From 93e7f90ae4f286394391857175803d3ed4c73012 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 24 Dec 2015 00:08:17 +0000 Subject: [PATCH 3/9] ScrollPanel: implement forward-fill --- src/components/structures/ScrollPanel.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index fc0b630f26..80d35aea65 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -150,6 +150,9 @@ module.exports = React.createClass({ // more messages. this._maybeFill(true); } + if (sn.scrollTop > sn.scrollHeight - sn.clientHeight * 2) { + this._maybeFill(false); + } }, // check if there is already a pending fill request. If not, set one off. From b5eae891b4f148f9988d7524f97455420bd9aa77 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 4 Jan 2016 16:28:32 +0000 Subject: [PATCH 4/9] Address review comments Make onFillRequest always return a promise --- src/components/structures/RoomView.js | 33 +++++---- src/components/structures/ScrollPanel.js | 88 +++++++++++++++--------- 2 files changed, 77 insertions(+), 44 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index fc909f69ab..150c39fb70 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -43,6 +43,13 @@ var INITIAL_SIZE = 20; var DEBUG_SCROLL = false; +if (DEBUG_SCROLL) { + // using bind means that we get to keep useful line numbers in the console + var debuglog = console.log.bind(console); +} else { + var debuglog = function () {}; +} + module.exports = React.createClass({ displayName: 'RoomView', propTypes: { @@ -344,7 +351,7 @@ module.exports = React.createClass({ }, _paginateCompleted: function() { - if (DEBUG_SCROLL) console.log("paginate complete"); + debuglog("paginate complete"); this.setState({ room: MatrixClientPeg.get().getRoom(this.props.roomId) @@ -355,34 +362,34 @@ module.exports = React.createClass({ onSearchResultsFillRequest: function(backwards) { if (!backwards) - return false; + return q(false); if (this.nextSearchBatch) { - if (DEBUG_SCROLL) console.log("requesting more search results"); + debuglog("requesting more search results"); return this._getSearchBatch(this.state.searchTerm, this.state.searchScope).then(true); } else { - if (DEBUG_SCROLL) console.log("no more search results"); - return false; + debuglog("no more search results"); + return q(false); } }, // set off a pagination request. onMessageListFillRequest: function(backwards) { if (!backwards) - return; + return q(false); // Either wind back the message cap (if there are enough events in the // timeline to do so), or fire off a pagination request. if (this.state.messageCap < this.state.room.timeline.length) { var cap = Math.min(this.state.messageCap + PAGINATE_SIZE, this.state.room.timeline.length); - if (DEBUG_SCROLL) console.log("winding back message cap to", cap); + debuglog("winding back message cap to", cap); this.setState({messageCap: cap}); - return true; + return q(true); } else if(this.state.room.oldState.paginationToken) { var cap = this.state.messageCap + PAGINATE_SIZE; - if (DEBUG_SCROLL) console.log("starting paginate to cap", cap); + debuglog("starting paginate to cap", cap); this.setState({messageCap: cap, paginating: true}); return MatrixClientPeg.get().scrollback(this.state.room, PAGINATE_SIZE). finally(this._paginateCompleted).then(true); @@ -492,7 +499,7 @@ module.exports = React.createClass({ } this.nextSearchBatch = null; - this._getSearchBatch(term, scope); + this._getSearchBatch(term, scope).done(); }, // fire off a request for a batch of search results @@ -509,11 +516,11 @@ module.exports = React.createClass({ var self = this; - if (DEBUG_SCROLL) console.log("sending search request"); + debuglog("sending search request"); return MatrixClientPeg.get().search({ body: this._getSearchCondition(term, scope), next_batch: this.nextSearchBatch }) .then(function(data) { - if (DEBUG_SCROLL) console.log("search complete"); + debuglog("search complete"); if (!self.state.searching || self.searchId != searchId) { console.error("Discarding stale search results"); return; @@ -559,7 +566,7 @@ module.exports = React.createClass({ self.setState({ searchInProgress: false }); - }).done(); + }); }, _getSearchCondition: function(term, scope) { diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 80d35aea65..072d397d10 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -21,6 +21,13 @@ var q = require("q"); var DEBUG_SCROLL = false; +if (DEBUG_SCROLL) { + // using bind means that we get to keep useful line numbers in the console + var debuglog = console.log.bind(console); +} else { + var debuglog = function () {}; +} + /* This component implements an intelligent scrolling list. * * It wraps a list of
  • children; when items are added to the start or end @@ -54,13 +61,14 @@ module.exports = React.createClass({ * the user nears the start (backwards = true) or end (backwards = * false) of the list. * - * This should return true if the pagination was successful, 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. + * This should return a promise; no more calls will be made until the + * promise completes. * - * This can return a promise; if it does, no more calls will be made - * until the promise completes. The promise should resolve to true or - * false as above. + * 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, @@ -80,7 +88,7 @@ module.exports = React.createClass({ getDefaultProps: function() { return { stickyBottom: true, - onFillRequest: function(backwards) {}, + onFillRequest: function(backwards) { return q(false); }, onScroll: function() {}, }; }, @@ -106,7 +114,7 @@ module.exports = React.createClass({ onScroll: function(ev) { var sn = this._getScrollNode(); - if (DEBUG_SCROLL) console.log("Scroll event: offset now:", sn.scrollTop, "recentEventScroll:", this.recentEventScroll); + debuglog("Scroll event: offset now:", sn.scrollTop, "recentEventScroll:", this.recentEventScroll); // Sometimes we see attempts to write to scrollTop essentially being // ignored. (Or rather, it is successfully written, but on the next @@ -130,7 +138,7 @@ module.exports = React.createClass({ } this.scrollState = this._calculateScrollState(); - if (DEBUG_SCROLL) console.log("Saved scroll state", this.scrollState); + debuglog("Saved scroll state", this.scrollState); this.props.onScroll(ev); @@ -145,12 +153,36 @@ module.exports = React.createClass({ checkFillState: function() { var sn = this._getScrollNode(); + // if there is less than a screenful of messages above or below the + // viewport, try to get some more messages. + // + // scrollTop is the number of pixels between the top of the content and + // the top of the viewport. + // + // scrollHeight is the total height of the content. + // + // clientHeight is the height of the viewport (excluding borders, + // margins, and scrollbars). + // + // + // .---------. - - + // | | | scrollTop | + // .-+---------+-. - - | + // | | | | | | + // | | | | | clientHeight | scrollHeight + // | | | | | | + // `-+---------+-' - | + // | | | + // | | | + // `---------' - + // + if (sn.scrollTop < sn.clientHeight) { - // there's less than a screenful of messages left - try to get some - // more messages. + // need to back-fill this._maybeFill(true); } if (sn.scrollTop > sn.scrollHeight - sn.clientHeight * 2) { + // need to forward-fill this._maybeFill(false); } }, @@ -159,36 +191,30 @@ module.exports = React.createClass({ _maybeFill: function(backwards) { var dir = backwards ? 'b' : 'f'; if (this._pendingFillRequests[dir]) { - if (DEBUG_SCROLL) { - console.log("ScrollPanel: Already a "+dir+" fill in progress - not starting another"); - } + debuglog("ScrollPanel: Already a "+dir+" fill in progress - not starting another"); return; } - if (DEBUG_SCROLL) { - console.log("ScrollPanel: starting "+dir+" fill"); - } + debuglog("ScrollPanel: starting "+dir+" fill"); // onFillRequest can end up calling us recursively (via onScroll // events) so make sure we set this before firing off the call. That // does present the risk that we might not ever actually fire off the // fill request, so wrap it in a try/catch. this._pendingFillRequests[dir] = true; - var r; + var fillPromise; try { - r = this.props.onFillRequest(backwards); + fillPromise = this.props.onFillRequest(backwards); } catch (e) { this._pendingFillRequests[dir] = false; throw e; } - q.finally(r, () => { - if (DEBUG_SCROLL) { - console.log("ScrollPanel: "+dir+" fill complete"); - } + q.finally(fillPromise, () => { + debuglog("ScrollPanel: "+dir+" fill complete"); this._pendingFillRequests[dir] = false; - }).then((res) => { - if (res) { + }).then((hasMoreResults) => { + if (hasMoreResults) { // further pagination requests have been disabled until now, so // it's time to check the fill state again in case the pagination // was insufficient. @@ -218,13 +244,13 @@ module.exports = React.createClass({ scrollToTop: function() { this._getScrollNode().scrollTop = 0; - if (DEBUG_SCROLL) console.log("Scrolled to top"); + debuglog("Scrolled to top"); }, scrollToBottom: function() { var scrollNode = this._getScrollNode(); scrollNode.scrollTop = scrollNode.scrollHeight; - if (DEBUG_SCROLL) console.log("Scrolled to bottom; offset now", scrollNode.scrollTop); + debuglog("Scrolled to bottom; offset now", scrollNode.scrollTop); }, // scroll the message list to the node with the given scrollToken. See @@ -261,10 +287,10 @@ module.exports = React.createClass({ this.recentEventScroll = scrollNode.scrollTop; } - if (DEBUG_SCROLL) { - console.log("Scrolled to token", node.dataset.scrollToken, "+", pixelOffset+":", scrollNode.scrollTop, "(delta: "+scrollDelta+")"); - console.log("recentEventScroll now "+this.recentEventScroll); - } + debuglog("Scrolled to token", node.dataset.scrollToken, "+", + pixelOffset+":", scrollNode.scrollTop, + "(delta: "+scrollDelta+")"); + debuglog("recentEventScroll now "+this.recentEventScroll); }, _calculateScrollState: function() { From e177263d9f97fbf334073e1fd934092cbe149859 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 4 Jan 2016 16:54:27 +0000 Subject: [PATCH 5/9] Address review comments Minor fixes post-review --- src/components/structures/RoomView.js | 21 +++++++++++---------- src/components/views/rooms/RoomHeader.js | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 96bd71bd23..334fc02e18 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -511,12 +511,11 @@ module.exports = React.createClass({ if (DEBUG_SCROLL) console.log("sending search request"); - var searchPromise = MatrixClientPeg.get().searchRoomEvents( - { filter: filter, - term: term, - }); - this._handleSearchResult(searchPromise) - .done(); + var searchPromise = MatrixClientPeg.get().searchRoomEvents({ + filter: filter, + term: term, + }); + this._handleSearchResult(searchPromise).done(); }, _backPaginateSearch: function() { @@ -524,13 +523,15 @@ module.exports = React.createClass({ var searchPromise = MatrixClientPeg.get().backPaginateRoomEventsSearch( this.state.searchResults); - this._handleSearchResult(searchPromise) - .done(); + this._handleSearchResult(searchPromise).done(); }, _handleSearchResult: function(searchPromise) { var self = this; - var searchId = this.searchId; + + // keep a record of the current search id, so that if the search terms + // change before we get a response, we can ignore the results. + var localSearchId = this.searchId; this.setState({ searchInProgress: true, @@ -538,7 +539,7 @@ module.exports = React.createClass({ return searchPromise.then(function(results) { if (DEBUG_SCROLL) console.log("search complete"); - if (!self.state.searching || self.searchId != searchId) { + if (!self.state.searching || self.searchId != localSearchId) { console.error("Discarding stale search results"); return; } diff --git a/src/components/views/rooms/RoomHeader.js b/src/components/views/rooms/RoomHeader.js index 0183ce2116..f1cfb67fe3 100644 --- a/src/components/views/rooms/RoomHeader.js +++ b/src/components/views/rooms/RoomHeader.js @@ -109,7 +109,7 @@ module.exports = React.createClass({ var searchStatus; // don't display the search count until the search completes and - // gives us a non-null searchCount. + // gives us a valid (possibly zero) searchCount. if (this.props.searchInfo && this.props.searchInfo.searchCount != null) { searchStatus =
     (~{ this.props.searchInfo.searchCount } results)
    ; } From a9f7bf63ffe94df4be5c113da8782b1d8fb96a34 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 21 Dec 2015 09:56:50 +0000 Subject: [PATCH 6/9] spell out we're doing 3PID invites --- src/components/views/rooms/MemberList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/rooms/MemberList.js b/src/components/views/rooms/MemberList.js index 8ca24f8992..719e4af9b3 100644 --- a/src/components/views/rooms/MemberList.js +++ b/src/components/views/rooms/MemberList.js @@ -254,7 +254,7 @@ module.exports = React.createClass({ } else { return (
    - +
    ); } From 6c99fab3dde9ceb296281e342fbed9f8ec724eaa Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 5 Jan 2016 15:28:32 +0000 Subject: [PATCH 7/9] Highlight the search term in search results Sometimes we don't get the search term back in the highlights list, so make sure we add it. --- src/components/structures/RoomView.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 334fc02e18..b9bc2c76c9 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -544,17 +544,20 @@ module.exports = React.createClass({ return; } - // postgres on synapse returns us precise details of the - // strings which actually got matched for highlighting. + // postgres on synapse returns us precise details of the strings + // which actually got matched for highlighting. + // + // In either case, we want to highlight the literal search term + // whether it was used by the search engine or not. + + var highlights = results.highlights; + if (highlights.indexOf(self.state.searchTerm) < 0) { + highlights = highlights.concat(self.state.searchTerm); + } // For overlapping highlights, // favour longer (more specific) terms first - var highlights = results.highlights.sort(function(a, b) { b.length - a.length }); - - // sqlite doesn't give us any highlights, so just try to highlight the literal search term - if (highlights.length == 0) { - highlights = [ self.state.searchTerm ]; - } + highlights = highlights.sort(function(a, b) { b.length - a.length }); self.setState({ searchHighlights: highlights, From 4730179c26c18592ab38457e108047094234a787 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 5 Jan 2016 15:51:16 +0000 Subject: [PATCH 8/9] Fix slight mis-merge We need to return 'true' from our promise of search result pagination. Also inline _backPaginateSearch which mostly served to confuse, and use debuglog instead of checking DEBUG_SCROLL --- src/components/structures/RoomView.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index abefbdd9bc..ad30f25b22 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -366,7 +366,9 @@ module.exports = React.createClass({ if (this.state.searchResults.next_batch) { debuglog("requesting more search results"); - return this._backPaginateSearch(); + var searchPromise = MatrixClientPeg.get().backPaginateRoomEventsSearch( + this.state.searchResults); + return this._handleSearchResult(searchPromise); } else { debuglog("no more search results"); return q(false); @@ -511,7 +513,7 @@ module.exports = React.createClass({ }; } - if (DEBUG_SCROLL) console.log("sending search request"); + debuglog("sending search request"); var searchPromise = MatrixClientPeg.get().searchRoomEvents({ filter: filter, @@ -520,14 +522,6 @@ module.exports = React.createClass({ this._handleSearchResult(searchPromise).done(); }, - _backPaginateSearch: function() { - if (DEBUG_SCROLL) console.log("sending search back-paginate request"); - - var searchPromise = MatrixClientPeg.get().backPaginateRoomEventsSearch( - this.state.searchResults); - return this._handleSearchResult(searchPromise); - }, - _handleSearchResult: function(searchPromise) { var self = this; From a2b7c9ba966c4f419ebe53aa1dce19d5b2e522eb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 5 Jan 2016 15:57:41 +0000 Subject: [PATCH 9/9] RoomHeader: Make 'undefined' check more explicit --- src/components/views/rooms/RoomHeader.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/rooms/RoomHeader.js b/src/components/views/rooms/RoomHeader.js index f1cfb67fe3..513b4bf192 100644 --- a/src/components/views/rooms/RoomHeader.js +++ b/src/components/views/rooms/RoomHeader.js @@ -110,7 +110,7 @@ module.exports = React.createClass({ var searchStatus; // don't display the search count until the search completes and // gives us a valid (possibly zero) searchCount. - if (this.props.searchInfo && this.props.searchInfo.searchCount != null) { + if (this.props.searchInfo && this.props.searchInfo.searchCount !== undefined && this.props.searchInfo.searchCount !== null) { searchStatus =
     (~{ this.props.searchInfo.searchCount } results)
    ; }