Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handling of case transformations in search #617

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
0686ca8
Handle unicode words and punctuation
Jaifroid Apr 26, 2020
d4d7a20
Find all possible combinations of first-letter cases
Jaifroid Apr 26, 2020
fce77af
Add documentation and £ and € to regex
Jaifroid Apr 26, 2020
b2d6b99
Documentation
Jaifroid Apr 26, 2020
57e57d5
Add whole word case variations
Jaifroid Apr 27, 2020
2d374d1
Preserve search as typed, normalize spacing, start all lowercase
Jaifroid Apr 27, 2020
2295a85
Document maintenance of punctuation regex
Jaifroid Apr 27, 2020
b1ae1f2
Add UI and implement options
Jaifroid Apr 30, 2020
2762355
Rationalize case calculations and document
Jaifroid Apr 30, 2020
ae7b228
Make slider value update dynamically
Jaifroid May 1, 2020
a2417f7
Beautify
Jaifroid May 1, 2020
829f764
Use plural consistently
Jaifroid May 1, 2020
c61298c
Add dynamic return of results
Jaifroid May 2, 2020
7d29369
Optimize for fast return of results
Jaifroid May 2, 2020
c99a07f
Update Unit Tests
Jaifroid May 2, 2020
03f85a5
Correct accent to breath-mark in Modern Greek test
Jaifroid May 2, 2020
d8d960a
Provide global params object for tests
Jaifroid May 2, 2020
69e5ad9
Increase assertion count
Jaifroid May 2, 2020
676e63a
Improve messaging
Jaifroid May 2, 2020
c75c3a2
Demo of granular progressive search
Jaifroid May 2, 2020
cc794b3
Fix tests again!
Jaifroid May 2, 2020
f99a8f4
Second attempt to fix tests
Jaifroid May 2, 2020
a6f7c2c
Very granular reporting
Jaifroid May 2, 2020
7f414f4
More effective search cancelling
Jaifroid May 3, 2020
1f9611c
Fix tests
Jaifroid May 3, 2020
166a4f8
Remove demo gif
Jaifroid May 3, 2020
1b0b474
Move non-generic elements out of util
Jaifroid May 3, 2020
502e45c
Fix tests again! Stop fiddling now.
Jaifroid May 3, 2020
9e9fdbc
Prevent searching same string twice
Jaifroid May 3, 2020
2c56ed8
Cancel search earlier
Jaifroid May 3, 2020
1e0a5b0
More cancellation
Jaifroid May 3, 2020
4a455d5
Fix user being unable to search for same string after launching article
Jaifroid May 3, 2020
58dfa1c
Provide state object for search to allow for cancellation
Jaifroid May 4, 2020
2443bef
Fix tests
Jaifroid May 4, 2020
81cd6f0
Remove unneeded UI elements
Jaifroid May 4, 2020
ac37a04
Rationalize use of state search
Jaifroid May 4, 2020
ba32d81
Reverse some unnecessary changes from previous model
Jaifroid May 4, 2020
22e4c76
Remove hard-coded position from tests
Jaifroid May 5, 2020
bdd4a69
Put cap on exponential searching
Jaifroid May 5, 2020
2e1e609
More informative messaging for user
Jaifroid May 5, 2020
67dba55
Remove redundant code and better messaging
Jaifroid May 6, 2020
7453da4
Changes from self-review
Jaifroid May 9, 2020
1fa46ce
Restore 50 results max search size
Jaifroid May 10, 2020
00d102f
Simplify interim reporting of search results
Jaifroid May 10, 2020
5a7c686
Clean up after rebase
Jaifroid Jul 2, 2020
71b93b2
Add comment about searching for common patterns first
Jaifroid Jul 2, 2020
b4eedd9
Only return searches beginning with prefix (rather than containing pr…
Jaifroid Jul 2, 2020
52b554e
Remove list of searches and reduce to single object
Jaifroid Jul 3, 2020
0497822
Make search object local to tests.js
Jaifroid Jul 3, 2020
6abf8dc
Rename global state object to "global"
Jaifroid Jul 3, 2020
7f225ab
Change global.search.state to globalstate.search.status
Jaifroid Jul 8, 2020
fa177a6
Remove dummy search object
Jaifroid Jul 8, 2020
c7a82fb
Keep and test original search prefix for more efficient cancelling
Jaifroid Jul 9, 2020
d056501
Minor updates from self-review
Jaifroid Jul 9, 2020
8a7d740
Prevent relaunching the same search if it is in progress
Jaifroid Jul 9, 2020
01a75d2
Add early return and check globalstate.search instead of local
Jaifroid Jul 10, 2020
115569b
Fix tests by reinstating globalstate variable
Jaifroid Jul 10, 2020
8ff4bd3
Revert "Fix tests by reinstating globalstate variable"
Jaifroid Jul 11, 2020
46e5ac3
Revert "Add early return and check globalstate.search instead of local"
Jaifroid Jul 11, 2020
5b36e4d
Fix description of slider and regex test
Jaifroid Jul 11, 2020
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
3 changes: 3 additions & 0 deletions tests/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
*/
'use strict';

// Define global params needed for tests to run on existing app code
var params = {};

Jaifroid marked this conversation as resolved.
Show resolved Hide resolved
require.config({
baseUrl: 'www/js/lib',
paths: {
Expand Down
23 changes: 13 additions & 10 deletions tests/tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,12 @@ define(['jquery', 'zimArchive', 'zimDirEntry', 'util', 'uiUtil', 'utf8'],

var localZimArchive;


/**
* Make an HTTP request for a Blob and return a Promise
*
* @param {String} url URL to download from
* @param {String} name Name to give to the Blob instance
* @returns {Promise}
* @returns {Promise<Blob>} A Promise for the Blob
*/
function makeBlobRequest(url, name) {
return new Promise(function (resolve, reject) {
Expand Down Expand Up @@ -104,15 +103,19 @@ define(['jquery', 'zimArchive', 'zimDirEntry', 'util', 'uiUtil', 'utf8'],
var float = util.readFloatFrom4Bytes(byteArray, 0);
assert.equal(float, -118.625, "the IEEE_754 float should be converted as -118.625");
});
QUnit.test("check upper/lower case variations", function(assert) {
QUnit.test("check upper/lower case variations", function (assert) {
var testString1 = "téléphone";
var testString2 = "Paris";
var testString3 = "le Couvre-chef Est sur le porte-manteaux";
var testString4 = "épée";
assert.equal(util.ucFirstLetter(testString1), "Téléphone", "The first letter should be upper-case");
assert.equal(util.lcFirstLetter(testString2), "paris", "The first letter should be lower-case");
assert.equal(util.ucEveryFirstLetter(testString3), "Le Couvre-Chef Est Sur Le Porte-Manteaux", "The first letter of every word should be upper-case");
assert.equal(util.ucFirstLetter(testString4), "Épée", "The first letter should be upper-case (with accent)");
var testString5 = '$¥€“«xριστός» †¡Ἀνέστη!”';
var testString6 = "Καλά Νερά Μαγνησίας žižek";
assert.equal(util.allCaseFirstLetters(testString1).indexOf("Téléphone") >= 0, true, "The first letter should be uppercase");
assert.equal(util.allCaseFirstLetters(testString2).indexOf("paris") >= 0, true, "The first letter should be lowercase");
assert.equal(util.allCaseFirstLetters(testString3).indexOf("Le Couvre-Chef Est Sur Le Porte-Manteaux") >= 0, true, "The first letter of every word should be uppercase");
assert.equal(util.allCaseFirstLetters(testString4).indexOf("Épée") >= 0, true, "The first letter should be uppercase (with accent)");
assert.equal(util.allCaseFirstLetters(testString5).indexOf('$¥€“«Xριστός» †¡ἀνέστη!”') >= 0, true, "First non-punctuation/non-currency Unicode letter should be uppercase, second (with breath mark) lowercase");
assert.equal(util.allCaseFirstLetters(testString6, "full").indexOf("ΚΑΛΆ ΝΕΡΆ ΜΑΓΝΗΣΊΑΣ ŽIŽEK") >= 0, true, "All Unicode letters should be uppercase");
});
QUnit.test("check removal of parameters in URL", function(assert) {
var testUrl1 = "A/question.html";
Expand Down Expand Up @@ -174,7 +177,7 @@ define(['jquery', 'zimArchive', 'zimDirEntry', 'util', 'uiUtil', 'utf8'],
assert.equal(firstDirEntry.getTitleOrUrl() , 'A Fool for You', 'First result should be "A Fool for You"');
done();
};
localZimArchive.findDirEntriesWithPrefix('A', 5, callbackFunction);
localZimArchive.findDirEntriesWithPrefix({prefix: 'A'}, 5, callbackFunction, true);
});
QUnit.test("check findDirEntriesWithPrefix 'a'", function(assert) {
var done = assert.async();
Expand All @@ -185,7 +188,7 @@ define(['jquery', 'zimArchive', 'zimDirEntry', 'util', 'uiUtil', 'utf8'],
assert.equal(firstDirEntry.getTitleOrUrl() , 'A Fool for You', 'First result should be "A Fool for You"');
done();
};
localZimArchive.findDirEntriesWithPrefix('a', 5, callbackFunction);
localZimArchive.findDirEntriesWithPrefix({prefix: 'a'}, 5, callbackFunction, true);
});
QUnit.test("check findDirEntriesWithPrefix 'blues brothers'", function(assert) {
var done = assert.async();
Expand All @@ -196,7 +199,7 @@ define(['jquery', 'zimArchive', 'zimDirEntry', 'util', 'uiUtil', 'utf8'],
assert.equal(firstDirEntry.getTitleOrUrl() , 'Blues Brothers (film)', 'First result should be "Blues Brothers (film)"');
done();
};
localZimArchive.findDirEntriesWithPrefix('blues brothers', 5, callbackFunction);
localZimArchive.findDirEntriesWithPrefix({prefix: 'blues brothers'}, 5, callbackFunction, true);
});
QUnit.test("article '(The Night Time Is) The Right Time' correctly redirects to 'Night Time Is the Right Time'", function(assert) {
var done = assert.async();
Expand Down
16 changes: 15 additions & 1 deletion www/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ <h3>Display settings</h3>
<br />
<div class="container">
<h3>Performance settings</h3>
<div class="card card-warning" id="cacheSettingsDiv">
<div class="card card-warning" id="performanceSettingsDiv">
<div class="card-header">Speed up archive access</div>
<div class="card-body">
<div class="row">
Expand Down Expand Up @@ -305,6 +305,20 @@ <h3>Performance settings</h3>
</div>
</div>
</div>
<br />
<div class="row">
<div class="col-sm-4">
<p>Select max number of search results:</p>
Jaifroid marked this conversation as resolved.
Show resolved Hide resolved
</div>
<div class="col-sm-8">
<div class="range">
<label>
<input type="range" class="form-control-range" min="5" max="50" id="titleSearchRange" value="25">
<span>Value: <span id="titleSearchRangeVal"></span> (<i>default 25, higher values increase search time</i>)</span>
</label>
</div>
</div>
</div>
</div>
</div>
</div>
Expand Down
99 changes: 66 additions & 33 deletions www/js/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,6 @@
define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesystemAccess','q'],
function($, zimArchiveLoader, uiUtil, settingsStore, abstractFilesystemAccess, Q) {

/**
* Maximum number of articles to display in a search
* @type Integer
*/
const MAX_SEARCH_RESULT_SIZE = 50;

/**
* The delay (in milliseconds) between two "keepalive" messages sent to the ServiceWorker (so that it is not stopped
* by the browser, and keeps the MessageChannel to communicate with the application)
Expand Down Expand Up @@ -70,13 +64,25 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
params['showUIAnimations'] = settingsStore.getItem('showUIAnimations') ? settingsStore.getItem('showUIAnimations') === 'true' : true;
document.getElementById('hideActiveContentWarningCheck').checked = params.hideActiveContentWarning;
document.getElementById('showUIAnimationsCheck').checked = params.showUIAnimations;
// Maximum number of article titles to return (range is 5 - 50, default 25)
params['maxSearchResultsSize'] = settingsStore.getItem('maxSearchResultsSize') || 25;
document.getElementById('titleSearchRange').value = params.maxSearchResultsSize;
document.getElementById('titleSearchRangeVal').innerHTML = params.maxSearchResultsSize;
// A global parameter that turns caching on or off and deletes the cache (it defaults to true unless explicitly turned off in UI)
params['useCache'] = settingsStore.getItem('useCache') !== 'false';
// A parameter to set the app theme and, if necessary, the CSS theme for article content (defaults to 'light')
params['appTheme'] = settingsStore.getItem('appTheme') || 'light'; // Currently implemented: light|dark|dark_invert|dark_mwInvert
document.getElementById('appThemeSelect').value = params.appTheme;
uiUtil.applyAppTheme(params.appTheme);

// Define global state (declared in init.js)
// An object to hold the current search and its state (allows cancellation of search across modules)
globalstate['search'] = {
'prefix': '', // A field to hold the original search string
'status': '', // The status of the search: ''|'init'|'interim'|'cancelled'|'complete'
'type': '' // The type of the search: 'basic'|'full' (set automatically in search algorithm)
};

// Define globalDropZone (universal drop area) and configDropZone (highlighting area on Config page)
var globalDropZone = document.getElementById('search-article');
var configDropZone = document.getElementById('configuration');
Expand Down Expand Up @@ -111,11 +117,15 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
// Define behavior of HTML elements
var searchArticlesFocused = false;
$('#searchArticles').on('click', function() {
var prefix = document.getElementById('prefix').value;
// Do not initiate the same search if it is already in progress
if (globalstate.search.prefix === prefix && !/^(cancelled|complete)$/.test(globalstate.search.status)) return;
$("#welcomeText").hide();
$('.alert').hide();
$("#searchingArticles").show();
pushBrowserHistoryState(null, $('#prefix').val());
searchDirEntriesFromPrefix($('#prefix').val());
pushBrowserHistoryState(null, prefix);
// Initiate the search
searchDirEntriesFromPrefix(prefix);
$('.navbar-collapse').collapse('hide');
document.getElementById('prefix').focus();
// This flag is set to true in the mousedown event below
Expand Down Expand Up @@ -198,7 +208,11 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
});
// Hide the search results if user moves out of prefix field
$('#prefix').on('blur', function() {
if (!searchArticlesFocused) $('#articleListWithHeader').hide();
if (!searchArticlesFocused) {
globalstate.search.status = 'cancelled';
$("#searchingArticles").hide();
$('#articleListWithHeader').hide();
}
});
$("#btnRandomArticle").on("click", function(e) {
$('#prefix').val("");
Expand Down Expand Up @@ -349,6 +363,13 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
refreshCacheStatus();
}
});
document.getElementById('titleSearchRange').addEventListener('change', function(e) {
settingsStore.setItem('maxSearchResultsSize', e.target.value, Infinity);
params.maxSearchResultsSize = e.target.value;
});
document.getElementById('titleSearchRange').addEventListener('input', function(e) {
document.getElementById('titleSearchRangeVal').innerHTML = e.target.value;
});

/**
* Displays or refreshes the API status shown to the user
Expand Down Expand Up @@ -441,7 +462,7 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
getCacheAttributes().then(function (cache) {
document.getElementById('cacheUsed').innerHTML = cache.description;
document.getElementById('assetsCount').innerHTML = cache.count;
var cacheSettings = document.getElementById('cacheSettingsDiv');
var cacheSettings = document.getElementById('performanceSettingsDiv');
var cacheStatusPanel = document.getElementById('cacheStatusPanel');
[cacheSettings, cacheStatusPanel].forEach(function (card) {
// IE11 cannot remove more than one class from a list at a time
Expand Down Expand Up @@ -688,9 +709,13 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
if (title && !(""===title)) {
goToArticle(title);
}
else if (titleSearch && !(""===titleSearch)) {
else if (titleSearch && titleSearch !== '') {
$('#prefix').val(titleSearch);
searchDirEntriesFromPrefix($('#prefix').val());
if (titleSearch !== globalstate.search.prefix) {
searchDirEntriesFromPrefix(titleSearch);
} else {
$('#prefix').focus();
}
}
}
};
Expand Down Expand Up @@ -926,33 +951,33 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys

/**
* Handle key input in the prefix input zone
* @param {Event} evt
* @param {Event} evt The event data to handle
*/
function onKeyUpPrefix(evt) {
// Use a timeout, so that very quick typing does not cause a lot of overhead
// It is also necessary for the words suggestions to work inside Firefox OS
if(window.timeoutKeyUpPrefix) {
if (window.timeoutKeyUpPrefix) {
window.clearTimeout(window.timeoutKeyUpPrefix);
}
window.timeoutKeyUpPrefix = window.setTimeout(function() {
window.timeoutKeyUpPrefix = window.setTimeout(function () {
var prefix = $("#prefix").val();
if (prefix && prefix.length>0) {
if (prefix && prefix.length > 0 && prefix !== globalstate.search.prefix) {
$('#searchArticles').click();
}
}
,500);
}, 500);
}


/**
* Search the index for DirEntries with title that start with the given prefix (implemented
* with a binary search inside the index file)
* @param {String} prefix
* @param {String} prefix The string that must appear at the start of any title searched for
*/
function searchDirEntriesFromPrefix(prefix) {
if (selectedArchive !== null && selectedArchive.isReady()) {
// Store the new search term in the globalstate.search object and initialize
globalstate.search = {'prefix': prefix, 'status': 'init', 'type': ''};
$('#activeContent').hide();
selectedArchive.findDirEntriesWithPrefix(prefix.trim(), MAX_SEARCH_RESULT_SIZE, populateListOfArticles);
selectedArchive.findDirEntriesWithPrefix(globalstate.search, params.maxSearchResultsSize, populateListOfArticles);
} else {
$('#searchingArticles').hide();
// We have to remove the focus from the search field,
Expand All @@ -963,30 +988,34 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
}
}


/**
* Display the list of articles with the given array of DirEntry
* @param {Array} dirEntryArray The array of dirEntries returned from the binary search
* @param {Object} reportingSearchPrefix The prefix of the reporting search
*/
function populateListOfArticles(dirEntryArray) {
function populateListOfArticles(dirEntryArray, reportingSearchPrefix) {
// Do not allow cancelled or changed searches to report
if (globalstate.search.status === 'cancelled' || globalstate.search.prefix !== reportingSearchPrefix) return;
var stillSearching = globalstate.search.status === 'interim';
var articleListHeaderMessageDiv = $('#articleListHeaderMessage');
var nbDirEntry = dirEntryArray ? dirEntryArray.length : 0;

var message;
if (nbDirEntry >= MAX_SEARCH_RESULT_SIZE) {
message = 'First ' + MAX_SEARCH_RESULT_SIZE + ' articles below (refine your search).';
if (stillSearching) {
message = 'Searching [' + globalstate.search.type + ']... found: ' + nbDirEntry;
} else if (nbDirEntry >= params.maxSearchResultsSize) {
message = 'First ' + params.maxSearchResultsSize + ' articles found (refine your search).';
} else {
message = nbDirEntry + ' articles found.';
}
if (nbDirEntry === 0) {
message = 'No articles found.';
message = 'Finished. ' + (nbDirEntry ? nbDirEntry : 'No') + ' articles found' + (
globalstate.search.type === 'basic' ? ': try fewer words for full search.' : '.'
);
}

articleListHeaderMessageDiv.html(message);

var articleListDiv = $('#articleList');
var articleListDivHtml = '';
var listLength = dirEntryArray.length < MAX_SEARCH_RESULT_SIZE ? dirEntryArray.length : MAX_SEARCH_RESULT_SIZE;
var listLength = dirEntryArray.length < params.maxSearchResultsSize ? dirEntryArray.length : params.maxSearchResultsSize;
for (var i = 0; i < listLength; i++) {
var dirEntry = dirEntryArray[i];
var dirEntryStringId = uiUtil.htmlEscapeChars(dirEntry.toStringId());
Expand All @@ -997,13 +1026,15 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
// We have to use mousedown below instead of click as otherwise the prefix blur event fires first
// and prevents this event from firing; note that touch also triggers mousedown
$('#articleList a').on('mousedown', function (e) {
// Cancel search immediately
globalstate.search.status = 'cancelled';
handleTitleClick(e);
return false;
});
$('#searchingArticles').hide();
if (!stillSearching) $('#searchingArticles').hide();
$('#articleListWithHeader').show();
}

/**
* Handles the click on the title of an article in search results
* @param {Event} event
Expand Down Expand Up @@ -1058,7 +1089,9 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
* @param {DirEntry} dirEntry The directory entry of the article to read
*/
function readArticle(dirEntry) {
// Only update for expectedArticleURLToBeDisplayed.
// Reset search prefix to allow users to search the same string again if they want to
globalstate.search.prefix = '';
// Only update for expectedArticleURLToBeDisplayed.
expectedArticleURLToBeDisplayed = dirEntry.namespace + "/" + dirEntry.url;
// We must remove focus from UI elements in order to deselect whichever one was clicked (in both jQuery and SW modes),
// but we should not do this when opening the landing page (or else one of the Unit Tests fails, at least on Chrome 58)
Expand Down
7 changes: 7 additions & 0 deletions www/js/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@
*/
var params = {};

/**
* A global object for storing app state
*
* @type Object
*/
var globalstate = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable can probably be moved to app.js, but it's minor and can be done later

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @mossroy . It would feel safer to me to move this as part of #637 when I'll have time to test a unified solution to cancelling running binary searches (and possibly also cancelling fetching of assets on pages we have navigated away from). So I'll move a summary of issues to be dealt with to #637 after squashing and merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's perfect


require.config({
baseUrl: 'js/lib',
paths: {
Expand Down
Loading