website: improve cheat-sheet page performance #1252
Conversation
rszyma
changed the title
|
One thing though: "removed" icons are now all over the search results, and not at the end as before. However, IMHO it's nothing compared to improvements this PR brings (and it's definitely fixable, but I don't have that much time to think of the best way to do this right now). |
|
Thanks for the PR! Camping right now, so I can't check it out right now. But wanted to comment that 'removed comes last' was not intended :-D |
|
Further note: See |
|
Yeah, I didn't know it was autogenerated. Maybe we should add a disclaimer like "This file was autogenerated by . Do not edit." at the beginning of the autogenerated files so it's more obvious? Your guess about Edit: I've moved the glyphs data back to From my understanding it should be safe to update the part of the |
I've updated generate-css.sh accordingly in #1254 |
|
Still struggling to iron out bugs in 3.0.1 and prepare 3.0.2, please do not think this is forgotten. A casual glance left the impression that you pull in the (in)famous NPM? How deep is that dependency well? |
[why] Disfunctional See PR #1252
|
Started your branch locally, the search behavior is a bit strange? Is that full word search? alt
alti
altimete
altimeter
|
|
It is full word search. While it works differently than before I'd argue it's better.
If you don't like the full-word search, there are some options to tweak:
There is also an Auto Suggestion feature, you can see it in action here |
If you're talking about MiniSearch, it's downloaded from CDN. See here: |
|
I'll comment further on what is going on in this PR to make the review easier.
|
[why] Disfunctional See PR #1252
|
@allcontributors please add @SilverMira for bug |
|
I've put up a pull request to add @SilverMira! 🎉 |
|
First of all, I finally spend some time really looking into the code here, and it is excellent! Thank you. The following comments are out of date. Instead I pushed some commits. Please review them. Performance is not the main goal for me, it is still far better than before. I believe the prefix search is needed for usability and the sorting is at least a nice-to-have. I have no means to benchmark that stuff apart from gut-value ;-)
Maybe you have time for a quick look at the following patch? From ee38d0ac6869c8a28be07724675a2b7716072da4 Mon Sep 17 00:00:00 2001
From: Fini Jastrow <[email protected]>
Date: Tue, 30 May 2023 11:45:01 +0200
Subject: [PATCH] cheat_sheet: Sort removed icons last
Signed-off-by: Fini Jastrow <[email protected]>
---
cheat-sheet.js | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/cheat-sheet.js b/cheat-sheet.js
index 144987e23..6b9561104 100644
--- a/cheat-sheet.js
+++ b/cheat-sheet.js
@@ -9,7 +9,7 @@ document.addEventListener('DOMContentLoaded', function () {
// Index all glyphs/icons
let miniSearch = new MiniSearch({
- fields: ['id', 'code'], // fields to index for full-text search
+ fields: ['id', 'code', 'isNew'], // fields to index for full-text search
storeFields: ['id', 'code', 'isRemoved'], // fields to return with search results
})
miniSearch.addAll(Object.entries(glyphs).map(
@@ -17,6 +17,7 @@ document.addEventListener('DOMContentLoaded', function () {
return {
id: key,
code: value,
+ isNew: key.startsWith('nfold') ? false : key,
}
}
));
@@ -136,6 +137,7 @@ document.addEventListener('DOMContentLoaded', function () {
{
prefix: false,
combineWith: "AND",
+ boost: { isNew: 2 },
}
);
--
2.39.2
I think this is needed at least. I also can not really see any speed impact? The miniSearch.addAll(Object.entries(glyphs).map(
([key, value]) => {
return {
id: key,
code: value,
+ isRemoved: key.startsWith('nfold'),
}
}
));But then, I guess it can be removed as it is never used. |
Well, I guess Ryan set up google analytics to have some insight in usage / usage-patterns. |
| { | ||
| prefix: true, | ||
| combineWith: "AND", | ||
| boostDocument: ((documentId, term, storedFields) => { |
There was a problem hiding this comment.
The downside of the boost based sorting is ... that it follows documents text searcher rules.
Take for example this...
Because eae9 has cod in the name twice it is listed too early (if you expect a sort-by-name list).
(file_code in front of account)
There was a problem hiding this comment.
This could be resolved by boosting down 'key' field and adding another field with just the part of the id after the last - part of icon class e.g. for nf-md-bluetooth_audio it would be bluetooth_audio
There was a problem hiding this comment.
The downside of the boost based sorting is ... that it follows documents text searcher rules.
It's not a downside to boost based sorting, but the consequence of setting prefix: true
|
I also tried a lot variants with boost, it did never work out. What is wrong with just sorting? Adding all the strings also costs memory, so it is hard to tell if sticking to boost has any upside. Example with your latest changes:
No real sort on startup:
Sorting still not according to id ;-)
|
|
I changed boost for
The variant you wanted is here: 09ad62b
Probably nothing. I did the changes in 09ad62b because I found that the sorting code didn't really work. Also there's a chance that using
I've just checked how much memory the loaded page takes and it's about 20 MB, so I'd say it's completely acceptable. The additional "name" field is used so to fix the issue related to setting
resolved with 61a9c4d |
The results look correct to me. All of the below names contain prefix "cod", so they are ranked equally: coda |
Of course it can be done with just ad4c9bc but because the minisearch sort algo takes into account to number of occurrences in a document (what we not only do not care for but actually do not want), I dropped that for a regular custom search with 9d09f38 Anyhow, resorting an almost sorted array should be really cheap. I do not know which sort is used by JS, but all usual candidates do that in linear time, right? I'm not sure that adding all that "complexity" with boost and (admittedly nice |
Right, they have the same RANK, but they are not sorted for their ID within the rank. |
I see! I thought it didn't work, so you changed it. My bad.
I see now why you changed to manual sort. As I said before, I changed the code to use
I don't know much about sorting in JS, so I'd rather believe minisort than myself to sort it for me (by using
I'm not necessarily trying to avoid manual sort(), but rather prefer the build-in features minisort already offers. (also I bet it's faster that way)
TBH, sounds like a non-issue to me. Maybe that could be changed with use of some of advanced parameters in minisearch, but I think it's not worth the trouble for now. |
|
Two questions
|
I need to check that out :-D |
|
Hmm, at current commit... Why is
|
At 9d09f38 (i.e. "manual search") I get this result...
... scrolled down ...
Edit: Your image seems to be not from 'my' manual search that takes remainingSearchResults.sort((a, b) => {
if (a.isRemoved != b.isRemoved) {
return a.isRemoved > b.isRemoved
}
return a.id > b.id
}) |
I don't see any major issues with this PR right now, so yes.
I'd suggest working on #1265 after this PR is merged. Worth noting, that #1265 breaks lazy-loading implemented in this PR.
Thanks!
Not at all
Sure, go ahead.
Honestly, no clue. Maybe it has something to do with the id having double 'f'?
I ran 9d09f38 again just now and it still doesn't work. Idk. |
|
Resolving some conflicts manually, force push |
Signed-off-by: Fini Jastrow <[email protected]>
[why] Sometimes one does not know what the exact search term is. For example if you want to find a 'homefolder' but the name is 'homedirectory' it is impossible to find. [how] Allow prefix search, in this case at least 'home' will find both variants. I do believe a full substring search would be even better, but that is not supported by minisearch. Signed-off-by: Fini Jastrow <[email protected]>
[why] Often it is easier to find what one wants if the search result is sorted. [how] Do a full result sort. * Sort by id (class name) * Put removed icons last We do not need the boost function anymore, so that pre-sorting is removed. Signed-off-by: Fini Jastrow <[email protected]>
|
🙄 Manual merge via commit to get PR number into git |
|
@allcontributors add @rszyma for code |
|
I've put up a pull request to add @rszyma! 🎉 |
|
Changed/added some small things.
✔️ Works ;-) Thanks for the great contribution! |
minisort just uses ordinary sort, just on the weights [1] with a sort lambda [2] like this: const byScore = ({ score: a }: Scored, { score: b }: Scored) => b - a
...
search (query: Query, searchOptions: SearchOptions = {}): SearchResult[] {
...
results.sort(byScore)
return results
}Javascript seems to use [3]. [1] https://github.com/lucaong/minisearch/blob/93bb0bc1878a48fcf2cf64e17099ae5a7042f9bd/src/MiniSearch.ts#LL1216C9-L1216C9 |
[why] PR ryanoasis#1252 and this ran slightly out of sync. Signed-off-by: Fini Jastrow <[email protected]>
Description
Update cheat-sheet page for better performance
Requirements / Checklist
What does this Pull Request (PR) do?
Modifications to cheat-sheet page:
Other
How should this be manually tested?
manually
Any background context you can provide?
Main motivation for these changes is terrible UX/performance of https://www.nerdfonts.com/cheat-sheet
What are the relevant tickets (if any)?
There are no tickets for neither bad cheat-sheet performance, nor sidecar not working.
Tandem PR: #1254