From 49a950b9e0dc262fd20c28e21ee4815ea8efe758 Mon Sep 17 00:00:00 2001 From: Sebastian Keller Date: Tue, 16 Nov 2021 18:57:26 +0100 Subject: [PATCH 1/3] search: Split out the description highlighter into its own class No functional change yet, only preparation to allow adding a unit test later on. Part-of: --- js/misc/util.js | 38 +++++++++++++++++++++++++++++++++++++- js/ui/search.js | 12 +++++------- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/js/misc/util.js b/js/misc/util.js index 8139d3f47..d1a702960 100644 --- a/js/misc/util.js +++ b/js/misc/util.js @@ -1,7 +1,8 @@ // -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- /* exported findUrls, spawn, spawnCommandLine, spawnApp, trySpawnCommandLine, formatTime, formatTimeSpan, createTimeLabel, insertSorted, - ensureActorVisibleInScrollView, wiggle, lerp, GNOMEversionCompare */ + ensureActorVisibleInScrollView, wiggle, lerp, GNOMEversionCompare, + Highlighter */ const { Clutter, Gio, GLib, Shell, St, GnomeDesktop } = imports.gi; const Gettext = imports.gettext; @@ -477,3 +478,38 @@ function GNOMEversionCompare(version1, version2) { return 0; } + +/* @class Highlighter Highlight given terms in text using markup. */ +var Highlighter = class { + /** + * @param {?string[]} terms - list of terms to highlight + */ + constructor(terms) { + if (!terms) + return; + + const escapedTerms = terms + .map(term => Shell.util_regex_escape(term)) + .filter(term => term.length > 0); + + if (escapedTerms.length === 0) + return; + + this._highlightRegex = new RegExp('(%s)'.format( + escapedTerms.join('|')), 'gi'); + } + + /** + * Highlight all occurences of the terms defined for this + * highlighter in the provided text using markup. + * + * @param {string} text - text to highlight the defined terms in + * @returns {string} + */ + highlight(text) { + if (!this._highlightRegex) + return text; + + return text.replace(this._highlightRegex, '$1'); + } +}; diff --git a/js/ui/search.js b/js/ui/search.js index 7300b053e..b1e76c46d 100644 --- a/js/ui/search.js +++ b/js/ui/search.js @@ -10,6 +10,8 @@ const ParentalControlsManager = imports.misc.parentalControlsManager; const RemoteSearch = imports.ui.remoteSearch; const Util = imports.misc.util; +const { Highlighter } = imports.misc.util; + const SEARCH_PROVIDERS_SCHEMA = 'org.gnome.desktop.search-providers'; var MAX_LIST_SEARCH_RESULTS_ROWS = 5; @@ -596,7 +598,7 @@ var SearchResultsView = GObject.registerClass({ this._providers = []; - this._highlightRegex = null; + this._highlighter = new Highlighter(); this._searchSettings = new Gio.Settings({ schema_id: SEARCH_PROVIDERS_SCHEMA }); this._searchSettings.connect('changed::disabled', this._reloadRemoteProviders.bind(this)); @@ -739,8 +741,7 @@ var SearchResultsView = GObject.registerClass({ if (this._searchTimeoutId == 0) this._searchTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 150, this._onSearchTimeout.bind(this)); - let escapedTerms = this._terms.map(term => Shell.util_regex_escape(term)); - this._highlightRegex = new RegExp('(%s)'.format(escapedTerms.join('|')), 'gi'); + this._highlighter = new Highlighter(this._terms); this.emit('terms-changed'); } @@ -894,10 +895,7 @@ var SearchResultsView = GObject.registerClass({ if (!description) return ''; - if (!this._highlightRegex) - return description; - - return description.replace(this._highlightRegex, '$1'); + return this._highlighter.highlight(description); } }); -- 2.35.1 From 7c1abe1bd91ecf274d81e122035cbeeef6fd58d4 Mon Sep 17 00:00:00 2001 From: Sebastian Keller Date: Wed, 17 Nov 2021 02:50:39 +0100 Subject: [PATCH 2/3] util: Properly handle markup in highlighter The code to highlight matches did not properly escape the passed in text as for markup before adding its highlighting markup. This lead to some search result descriptions not showing up, because their descriptions contained characters, such as "<", that would have to be escaped when used in markup or otherwise lead to invalid markup. To work around this some search providers wrongly started escaping the description on their end before sending them to gnome-shell. This lead to another issue. Now if the highlighter was trying to highlight the term "a", and the escaped description contained "'", the "a" in that would be considered a match and surrounded by "". This however would also generate invalid markup, again leading to an error and the description not being shown. Fix this by always escaping the passed in string before applying the highlights in such a way that there are no matches within entities. This also means that search providers that escaped their description strings will now show up with the markup syntax. This will have to be fixed separately in the affected search providers. Fixes: https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/4791 Part-of: --- js/misc/util.js | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/js/misc/util.js b/js/misc/util.js index d1a702960..802398d18 100644 --- a/js/misc/util.js +++ b/js/misc/util.js @@ -508,8 +508,25 @@ var Highlighter = class { */ highlight(text) { if (!this._highlightRegex) - return text; + return GLib.markup_escape_text(text, -1); + + let escaped = []; + let lastMatchEnd = 0; + let match; + while ((match = this._highlightRegex.exec(text))) { + if (match.index > lastMatchEnd) { + let unmatched = GLib.markup_escape_text( + text.slice(lastMatchEnd, match.index), -1); + escaped.push(unmatched); + } + let matched = GLib.markup_escape_text(match[0], -1); + escaped.push('%s'.format(matched)); + lastMatchEnd = match.index + match[0].length; + } + let unmatched = GLib.markup_escape_text( + text.slice(lastMatchEnd), -1); + escaped.push(unmatched); - return text.replace(this._highlightRegex, '$1'); + return escaped.join(''); } }; -- 2.35.1 From 82e2a6dcfabc2f82efbf468175d16c303f0c73da Mon Sep 17 00:00:00 2001 From: Sebastian Keller Date: Wed, 17 Nov 2021 03:05:05 +0100 Subject: [PATCH 3/3] tests: Add unit test for highlighter Part-of: --- tests/meson.build | 12 ++++- tests/unit/highlighter.js | 106 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 tests/unit/highlighter.js diff --git a/tests/meson.build b/tests/meson.build index c0431631f..50fb601e9 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -10,7 +10,17 @@ run_test = configure_file( testenv = environment() testenv.set('GSETTINGS_SCHEMA_DIR', join_paths(meson.build_root(), 'data')) -foreach test : ['insertSorted', 'jsParse', 'markup', 'params', 'url', 'versionCompare'] +tests = [ + 'highlighter', + 'insertSorted', + 'jsParse', + 'markup', + 'params', + 'url', + 'versionCompare', +] + +foreach test : tests test(test, run_test, args: 'unit/@0@.js'.format(test), env: testenv, diff --git a/tests/unit/highlighter.js b/tests/unit/highlighter.js new file mode 100644 index 000000000..d582d38e3 --- /dev/null +++ b/tests/unit/highlighter.js @@ -0,0 +1,106 @@ +// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- + +// Test cases for SearchResult description match highlighter + +const JsUnit = imports.jsUnit; +const Pango = imports.gi.Pango; + +const Environment = imports.ui.environment; +Environment.init(); + +const Util = imports.misc.util; + +const tests = [ + { input: 'abc cba', + terms: null, + output: 'abc cba' }, + { input: 'abc cba', + terms: [], + output: 'abc cba' }, + { input: 'abc cba', + terms: [''], + output: 'abc cba' }, + { input: 'abc cba', + terms: ['a'], + output: 'abc cba' }, + { input: 'abc cba', + terms: ['a', 'a'], + output: 'abc cba' }, + { input: 'CaSe InSenSiTiVe', + terms: ['cas', 'sens'], + output: 'CaSe InSenSiTiVe' }, + { input: 'This contains the < character', + terms: null, + output: 'This contains the < character' }, + { input: 'Don\'t', + terms: ['t'], + output: 'Don't' }, + { input: 'Don\'t', + terms: ['n\'t'], + output: 'Don't' }, + { input: 'Don\'t', + terms: ['o', 't'], + output: 'Don't' }, + { input: 'salt&pepper', + terms: ['salt'], + output: 'salt&pepper' }, + { input: 'salt&pepper', + terms: ['salt', 'alt'], + output: 'salt&pepper' }, + { input: 'salt&pepper', + terms: ['pepper'], + output: 'salt&pepper' }, + { input: 'salt&pepper', + terms: ['salt', 'pepper'], + output: 'salt&pepper' }, + { input: 'salt&pepper', + terms: ['t', 'p'], + output: 'salt&pepper' }, + { input: 'salt&pepper', + terms: ['t', '&', 'p'], + output: 'salt&pepper' }, + { input: 'salt&pepper', + terms: ['e'], + output: 'salt&pepper' }, + { input: 'salt&pepper', + terms: ['&a', '&am', '&', '&'], + output: 'salt&pepper' }, + { input: '&&&&&', + terms: ['a'], + output: '&&&&&' }, + { input: '&;&;&;&;&;', + terms: ['a'], + output: '&;&;&;&;&;' }, + { input: '&;&;&;&;&;', + terms: [';'], + output: '&;&;&;&;&;' }, + { input: '&', + terms: ['a'], + output: '&amp;' } +]; + +try { + for (let i = 0; i < tests.length; i++) { + let highlighter = new Util.Highlighter(tests[i].terms); + let output = highlighter.highlight(tests[i].input); + + JsUnit.assertEquals(`Test ${i + 1} highlight ` + + `"${tests[i].terms}" in "${tests[i].input}"`, + output, tests[i].output); + + let parsed = false; + try { + Pango.parse_markup(output, -1, ''); + parsed = true; + } catch (e) {} + JsUnit.assertEquals(`Test ${i + 1} is valid markup`, true, parsed); + } +} catch (e) { + if (typeof(e.isJsUnitException) != 'undefined' + && e.isJsUnitException) + { + if (e.comment) + log(`Error in: ${e.comment}`); + } + throw e; +} -- 2.35.1