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/5] 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 e177263d9f97fbf334073e1fd934092cbe149859 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 4 Jan 2016 16:54:27 +0000 Subject: [PATCH 2/5] 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 6c99fab3dde9ceb296281e342fbed9f8ec724eaa Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 5 Jan 2016 15:28:32 +0000 Subject: [PATCH 3/5] 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 4/5] 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 5/5] 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)
    ; }