nyxt
nyxt copied to clipboard
atlas-engineer
Lispy search-buffer
Description
This changes our search functionality to depend on Plump and Lisp-side calculations. Pros:
- More controllable and extensible (for example, for #2230 (CC @aadcg) and search-based annotations).
- Stores most data on the Lisp side, thus saves lots of JavaScript calls.
- Adds a neat set of changes to
nyxt/domthat can be used for libraries and extensions.
Cons:
- Highlighting is less exact, as it highlights the smallest element containing the search text, and this element may not be exactly small.
- May be slower than JS-based search, but, given how fast we made our hints, this is a solvable problem.
This also includes quite some changes to the existing functions that make working with Plump DOM even easier!
Discussion
- Is the element-level highlighting enough for text search? I feel like it adds more context, but I can foresee the opposing arguments too :)
- Should we reuse the same
nyxt/dom:get-unique-selectorin hint mode? This way, we don't need to alter tha page structure anymore. But then, it may be not unique enough...
Checklist:
Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.
- [X] I have pulled from master before submitting this PR
- [X] My code follows the style guidelines for Common Lisp code
- [X] I have performed a self-review of my own code
- [ ] My code has been reviewed by at least one peer (the peer review to approve a PR counts. The reviewer must download and test the code)
- [X] I have commented my code, particularly in hard-to-understand areas
- [X] I have made corresponding changes to the documentation
- [X] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [X] New and existing unit tests pass locally with my changes
- [X] There are no merge conflicts
How does it look?
I've tested it. Searching multiple times on a page led to a crash :-O. Additionally, highlighting the whole element is not particularly helpful! I also noticed that when searching large sections of the page would become blank! I can attach screenshots later if you like.
@aartaka exciting work! It's not there yet, but it's promising.
In terms of the design, I'm not convinced on the idea of highlighting elements. Say we you have multiple matches on one element.
I've tested it. Searching multiple times on a page led to a crash :-O. Additionally, highlighting the whole element is not particularly helpful! I also noticed that when searching large sections of the page would become blank! I can attach screenshots later if you like.
Huh, that's interesting, I haven't see anything like that when testing 🤔 A reason to test more then :D
In terms of the design, I'm not convinced on the idea of highlighting elements. Say we you have multiple matches on one element.
Neither am I. But I can see the benefit of highlighting the whole piece, as what we are usually looking for is the paragraph/mention/context of a certain word/phrase, not the exact phrase itself.
@aadcg 's feedback on the call: maybe highlight both the context (the element) and the exact matches (user input) in different degrees of accent color.
EDIT: Clarify. EDIT: Right nickname.
OK, I've contributed a more extensive character set to the CLSS, which should mitigate the biggest source of errors and crashes of lispy search that @jmercouris noticed (although I'd enjoy having more detailed reports). Search should work fine now. Things left to do:
- Highlighting both elements and text matches, as per @aadcg's feedback.
- Polish the element counting function to actually update the DOM when things change significantly. Right now it seems to stay outdated for long enough to invalidate the search.
Element counting is fixed on master with e9e85bf1966961f293ea64cdc8078f9c61183f29 :D
While I want this to be merged as an improvement over opaque JS-based search, I am demotivated trying to highlight both elements and exact matches inside them, as per @aadcg's suggestion. Can you try this on some familiar pages searching for something and evaluate whether the element-based highlighting is bearable? @jmercouris, @Ambrevar?
I would commit to making exact highlighting possible... but we all know that once a feature is merged, it's increasingly less probable that incomplete yet merged parts of the feature would ever be completed :P
Oh, I found one more problem: unescaped unicode characters as parts of CSS selectors break CLSS, but there are no means in CLSS to read escaped characters either :thinking:
The CSS escaping problem solved on the CLSS side now, Lispy search is fully functional. The last thing I'd add is listing the exact matching snippet in the prompt-buffer so that it's clearer from the prompt-buffer alone what the context of the word is.
Text truncation to show the match in the prompt-buffer is there. Will be glad if anyone tests and expresses their opinion on how usable the element-level highlighting is!
@Ambrevar's feedback: element-level highlighting is not enough, as one sometimes looks for the paragraph with the biggest word concentration, and not the context of word appearance -- context comes after.
My first comment is that I ran into some compilation errors while (asdf:load-system :nyxt/gtk-application) on this branch. For example:
; caught ERROR:
; READ error during COMPILE-FILE:
;
; The symbol "UI-DISPLAY" is not external in the NDEBUG package.
;
; Line: 27, Column: 28, File-Position: 868
There seem to be issues with prompter:object-attributes from search-buffer. Recipe: go to potato on wikipedia, and search for potato. @aartaka, could you take a look?
I agree with @Ambrevar that context isn't enough.
I am demotivated trying to highlight both elements and exact matches inside them
Are you're saying you're tired of working on this, and you'd like me to jump in?
There seem to be issues with
prompter:object-attributesfromsearch-buffer. Recipe: go to potato on wikipedia, and search for potato. @aartaka, could you take a look?
Fixed with ea3dfbd72391bc318bb5899fe52e7a61ea41e3a3.
I agree with @Ambrevar that context isn't enough.
I am demotivated trying to highlight both elements and exact matches inside them
Are you're saying you're tired of working on this, and you'd like me to jump in?
I'll be glad if you can try exploring it, yes!
@aartaka, could you explain commit 9935a6e?
It basically replaces the <mark> node with its first child (which is the meaningful element highlighted by <mark>). Note that this design is already supereded by adding styles to the actual elements to highlight in 54f5aabf76291ebb785382729e5800428fda6bb5.