Skip to content
This repository was archived by the owner on Feb 29, 2020. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion addon/PlacesProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ Links.prototype = {
* @returns {Promise} Returns a promise with the array of links as payload.
*/
_getHistoryLinks: Task.async(function*(options = {}) {
let {limit, afterDate, beforeDate, order, ignoreBlocked} = options;
let {limit, afterDate, beforeDate, filter, order, ignoreBlocked} = options;
if (!limit || limit.options > LINKS_QUERY_LIMIT) {
limit = LINKS_QUERY_LIMIT;
}
Expand All @@ -359,6 +359,22 @@ Links.prototype = {
params.beforeDate = beforeDate;
}

// setup filter binding and sql clause
let filterClause = "";
if (filter) {
// Match each token in either the title or url
let tokens = filter.trim().toLowerCase().split(/\s+/);
tokens.forEach((token, i) => {
filterClause = `${filterClause} AND (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: how come we're using the blank string from filterClause here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other Clauses start with "AND ..." because they're chaining to the WHERE. This is the same except there's a dynamic number of chains based on how many tokens are being used, e.g., " AND token1 ... AND token2 ... AND token3 ..." The empty string is the initial value so that the subsequent token clauses can be concatenated.

moz_places.title LIKE :filter${i} OR
moz_places.url LIKE :filter${i})`;
params[`filter${i}`] = `%${token}%`;
});
}
else {
filter = "";
}

// setup order by clause
let orderbyClause = (order === "bookmarksFrecency") ?
"ORDER BY bookmarkDateCreated DESC, frecency DESC, lastVisitDate DESC, url" :
Expand All @@ -384,6 +400,7 @@ Links.prototype = {
AND moz_places.url NOT IN (${blockedURLs})
${afterDateClause}
${beforeDateClause}
${filterClause}
${orderbyClause}
LIMIT :limit`;

Expand All @@ -393,6 +410,9 @@ Links.prototype = {
params
});

// Remember how these results were filtered
links = links.map(link => Object.assign(link, {filter}));

links = this._faviconBytesToDataURI(links);
return links.filter(link => LinkChecker.checkLoadURI(link.url));
}),
Expand Down
4 changes: 2 additions & 2 deletions common/action-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ function RequestRecentLinks(options) {
return RequestExpect("RECENT_LINKS_REQUEST", "RECENT_LINKS_RESPONSE", options);
}

function RequestMoreRecentLinks(beforeDate) {
function RequestMoreRecentLinks(beforeDate, filter = "") {
return RequestRecentLinks({
data: {beforeDate},
data: {beforeDate, filter},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to make sure, this is for infinite scrolling yes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. It's to include the query for fetching more when infinitely scrolling.

append: true,
meta: {skipPreviewRequest: true}
});
Expand Down
2 changes: 1 addition & 1 deletion common/reducers/Filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module.exports = function Filter(prevState = INITIAL_STATE, action) {
const state = Object.assign({}, prevState);
switch (action.type) {
case am.type("NOTIFY_FILTER_QUERY"):
state.query = action.data;
state.query = action.data || "";
break;
}
return state;
Expand Down
4 changes: 4 additions & 0 deletions common/reducers/SetRowsOrError.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ module.exports = function setRowsOrError(requestType, responseType, querySize) {
case am.type("NOTIFY_HISTORY_DELETE"):
state.rows = prevState.rows.filter(val => val.url !== action.data);
break;
case requestType === "RECENT_LINKS_REQUEST" && am.type("NOTIFY_FILTER_QUERY"):
// Allow loading more even if we hit the end as the filter has changed
state.canLoadMore = true;
break;
default:
return prevState;
}
Expand Down
3 changes: 0 additions & 3 deletions content-src/components/Header/Header.scss
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@
background: $search-glyph-image no-repeat 8px center / $search-glyph-size;
margin-left: $header-section-spacing;
max-width: $timeline-max-width;

// GH#1305: Temporarily disable for current release
display: none;
}

.icon-dismiss {
Expand Down
7 changes: 5 additions & 2 deletions content-src/components/TimelinePage/TimelineFeed.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@ const {INFINITE_SCROLL_THRESHOLD, SCROLL_TOP_OFFSET} = require("common/constants
const debounce = require("lodash.debounce");

const TimelineFeed = React.createClass({
getFilterQuery() {
return (this.props.Filter || {}).query || "";
},
loadMore() {
const items = this.props.Feed.rows;
if (!items.length) {
return;
}
const beforeDate = items[items.length - 1][this.props.dateKey];
this.props.dispatch(this.props.loadMoreAction(beforeDate));
this.props.dispatch(this.props.loadMoreAction(beforeDate, this.getFilterQuery()));
this.props.dispatch(NotifyEvent({
event: "LOAD_MORE_SCROLL",
page: this.props.pageName,
Expand Down Expand Up @@ -75,7 +78,7 @@ const TimelineFeed = React.createClass({
},
render() {
const props = this.props;
const query = (props.Filter && props.Filter.query) || "";
const query = this.getFilterQuery();
const showSpotlight = props.Spotlight && query === "";
return (<section className="content" ref="scrollElement" onScroll={!props.Feed.isLoading && props.Feed.canLoadMore && this.loadMoreDataIfNeeded}>
<div ref="wrapper" className={classNames("wrapper", "show-on-init", {on: props.Feed.init})}>
Expand Down
5 changes: 4 additions & 1 deletion content-src/selectors/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,10 @@ module.exports.selectHistory = createSelector(
return {
Spotlight: Object.assign({}, Spotlight, {rows}),
Filter,
History
History: Object.assign({}, History, {
// Only include rows that are less filtered than the current filter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment is kind of unclear - could you explain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A query of "query" is more filtered/specific than a query of "qu" which is more specific than "". If the current query is "que" it would include rows for "" and "qu" but not "query" because "query" is more specific. This is desired because the user might be searching for "queso".

rows: History.rows.filter(val => Filter.query.indexOf(val.filter) === 0)
})
};
}
);
Expand Down
25 changes: 25 additions & 0 deletions content-test/selectors/selectors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,31 @@ describe("selectors", () => {
});
});
});
describe("selectHistory keep less filtered rows", () => {
const rows = [
{filter: ""},
{filter: "a"},
{filter: "ab"}
];
function doSelect(query) {
return selectHistory(Object.assign({}, fakeState, {
Filter: {query},
History: {rows}
}));
}
it("should keep only unfiltered for empty", () => {
let state = doSelect("");
assert.lengthOf(state.History.rows, 1);
});
it("should keep only unfiltered and partially filtered for partial filter", () => {
let state = doSelect("a");
assert.lengthOf(state.History.rows, 2);
});
it("should keep all filtered for full filter", () => {
let state = doSelect("ab");
assert.lengthOf(state.History.rows, 3);
});
});
describe("selectNewTabSites", () => {
let state;
beforeEach(() => {
Expand Down
16 changes: 15 additions & 1 deletion test/test-PlacesProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,25 @@ exports.test_Links_getRecentLinks = function*(assert) {
assert.equal(links[0].url, "https://mozilla3.com/2", "Expected 1-st link");
assert.equal(links[1].url, "https://mozilla4.com/3", "Expected 2-nd link");

// test beforeDate functionality
// test afterDate functionality
links = yield provider.getRecentLinks({afterDate: theDate});
assert.equal(links.length, 2, "should only see two links inserted after the date");
assert.equal(links[0].url, "https://mozilla2.com/1", "Expected 1-st link");
assert.equal(links[1].url, "https://mozilla1.com/0", "Expected 2-nd link");

// test filter functionality
links = yield provider.getRecentLinks({filter: ""});
assert.equal(links.length, 4, "should match all links");
assert.equal(links[0].filter, "", "should include filter");
links = yield provider.getRecentLinks({filter: "a"});
assert.equal(links.length, 4, "should match all links");
assert.equal(links[0].filter, "a", "should include filter");
links = yield provider.getRecentLinks({filter: "a1"});
assert.equal(links.length, 1, "should match just mozilla1");
assert.equal(links[0].filter, "a1", "should include filter");
links = yield provider.getRecentLinks({filter: "a 1"});
assert.equal(links.length, 2, "should match mozilla1 and /1");
assert.equal(links[0].filter, "a 1", "should include filter");
};

exports.test_Links_getFrecentLinks = function*(assert) {
Expand Down