atlas-engineer
Improve hinting UI and its user-options
Description
Compute hints in viewport
Before:
After:
Closes #2292 Closes #2185 Closes #538 Closes #536.
Discussion
The algorithm for element-in-view-port-p is still rather primitive, and prone to errors. Find an example below of an element that is not visible, but marked as in viewport (hint marked as "D"):
These changes make our hints much slower, i.e. the overlay appears on the screen later (even though we were adding them for the whole document before!). I have some ideas on how to fix it.
Question: Should element-in-view-port-p be a parenscript macro? I don't think so. It's a procedure, and mostly likely one we'll need to specialize depending on the situation.
Question: Should we automatically (probably via hooks on input events) re-compute hints as the viewport changes? I'd say no. It's trivial from the user to cancel ans re-issue the command.
Issue: How to remove the scope of the hints when show-hint-scope-p is true?
Issue: JS relative to overlapped-p.
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
- [x] 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
- [ ] There are no merge conflicts
It's a very welcomed feature, thanks!
Why did you list #2189 as a fixed issue? Does not seem related :p Mistake?
Why did you list #2189 as a fixed issue? Does not seem related :p Mistake?
Oh, absolutely! I'll fix it!
I'm not sure what you are asking exactly. If what you want is prompt that matches only against the hints (and not the title, etc.), then I can think of at least 2 ways to do it:
- Specialize object-attributes against the source to only display the hints.
- Set the
filter-*slots to match only against the hints.
Still working on #2292. I have implemented a new way to compute the hint strings. Each has length 2, which gives 676 with an alphabet of 26 characters. That's more than enough to hint elements in viewport, even with smaller alphabets.
I still need to make plump and JS talk to each other properly.
I believe Artyom is saying that the algorithm to generate hints should support any arbitrary amount of hints. If we can support more than two characters, we could support an infinite amount of hints. This would of course be better.
Possible solution to the viewport calculation slowness: you can check&store the viewport belonging of an element when parsing it in nyxt/dom:get-document-body-json. That should mitigate most performance problems stemming from Lisp->C->JS->C->Lisp communication.
Renaming the branch on the remote closes the PR, sorry. I guess add-barebones-hinting-ui isn't a bad name for this branch.
Help me, I'm a parenscript potato.
Can someone please make the following work according to the comment? @aartaka @jmercouris
(define-parenscript overlapped-p (id)
"Whether ELEMENT is hidden by another overlapping element."
(ps:let* ((element (nyxt/ps:qs-nyxt-id document (ps:lisp id)))
(rect (ps:chain element (get-bounding-client-rect)))
(nearest-element (ps:chain document (element-from-point (ps:chain rect left)
(ps:chain rect top)))))
;; element is overlapped iff the (ps:chain element parent-node) call returns
;; the following error:
;; TypeError: null is not an object (evaluating 'element.parentNode')
(loop while (and nearest-element
(not (= nearest-element element)))
do (ps:try (setf element (ps:chain element parent-node))
(:catch (error))))))
I'm not sure that errors are the best way to find out the overlapping of elements. Maybe rather compare the rectangles there?
I'm getting a really weird warning from SBCL after commit b8cf9d222. @aartaka do you have any idea on how to fix my macro hack? If I can't come up with smth, I'll do it in the most obvious and less elegant way:
(export-always 'define-parenscript-async)
(defmacro define-parenscript-async (script-name args &body script-body)
"Define parenscript function SCRIPT-NAME.
SCRIPT-BODY must be a valid parenscript and will be wrapped in (PS:PS ...).
Any Lisp expression must be wrapped in (PS:LISP ...).
The returned function sends the compiled Javascript to the current buffer webview.
The function can be passed ARGS."
`(defun ,script-name ,args
(ffi-buffer-evaluate-javascript-async (current-buffer)
(ps:ps ,@script-body))))
(defmacro define-parenscript (script-name args &body script-body)
"Define parenscript function SCRIPT-NAME.
SCRIPT-BODY must be a valid parenscript and will be wrapped in (PS:PS ...).
Any Lisp expression must be wrapped in (PS:LISP ...).
The returned function sends the compiled Javascript to the current buffer webview.
The function can be passed ARGS."
(let* ((keyarg-index (position '&key args))
(arguments (if keyarg-index
(let ((new-args args))
(push '(async nil) (cdr (nthcdr keyarg-index args)))
new-args)
(append args '(&key (async nil))))))
`(defun ,script-name ,arguments
(funcall (if async
ffi-buffer-evaluate-javascript-async
ffi-buffer-evaluate-javascript)
(current-buffer)
(ps:ps ,@script-body)))))
failed AVER: (NULL SB-C::CURRENT) This is probably a bug in SBCL itself. (Alternatively, SBCL might have been corrupted by bad user code, e.g. by an undefined Lisp operation like (FMAKUNBOUND 'COMPILE), or by stray pointers from alien code or from unsafe Lisp code; or there might be a bug in the OS or hardware that SBCL is running on.) If it seems to be a bug in SBCL itself, the maintainers would like to know about it. Bug reports are welcome on the SBCL mailing lists, which you can find at http://sbcl.sourceforge.net/. [Condition of type SB-INT:BUG]
Help me, I'm a parenscript potato.
Can someone please make the following work according to the comment? @aartaka @jmercouris
(define-parenscript overlapped-p (id) "Whether ELEMENT is hidden by another overlapping element." (ps:let* ((element (nyxt/ps:qs-nyxt-id document (ps:lisp id))) (rect (ps:chain element (get-bounding-client-rect))) (nearest-element (ps:chain document (element-from-point (ps:chain rect left) (ps:chain rect top))))) ;; element is overlapped iff the (ps:chain element parent-node) call returns ;; the following error: ;; TypeError: null is not an object (evaluating 'element.parentNode') (loop while (and nearest-element (not (= nearest-element element))) do (ps:try (setf element (ps:chain element parent-node)) (:catch (error))))))
To answer my own question, a simply way of going around is:
(define-parenscript overlapped-p (id)
"Whether ELEMENT is hidden by another overlapping element."
(ps:let* ((element (nyxt/ps:qs-nyxt-id document (ps:lisp id)))
(rect (ps:chain element (get-bounding-client-rect)))
(nearest-element (ps:chain document (element-from-point (ps:chain rect left)
(ps:chain rect top)))))
(loop while (and nearest-element
element
(not (= nearest-element element)))
do (setf element (ps:chain element parent-node)))
(null element)))
@aadcg, see 1db1e33d180e02e783c82534a8c4614c4ea46ae0 for the :buffer and :async argument to peval.
@aartaka, I find the implementation a bit odd. Keyword arguments aren't positional, but we force them to be. It feels wrong to me.
Also, let's not forget about #2227.
@aartaka, I find the implementation a bit odd. Keyword arguments aren't positional, but we force them to be. It feels wrong to me.
Still, it's better of a pattern than any other I thought of:
- Putting async and buffer before the bindigns list in
pflet. - Adding another mandatory arglist (that's what StumpWM,
defclass, and some libraries do).
While this one resembles the pattern of method qualifiers that occupy an arbitrary space after the method name and before the args. So, it's still more or less CL-ish, just a bit special :)
Also note that there's no syntax (except addditional arglist) for this particular problem that would be supported by destructuring-bind. We have to stretch the parsing anyway, so why not make the most comfy version of the syntax we could have?
Also, let's not forget about #2227.
Ah, I should probably close that. Thanks for mentioning!
Also note that there's no syntax (except addditional arglist) for this particular problem that would be supported by destructuring-bind. We have to stretch the parsing anyway, so why not make the most comfy version of the syntax we could have?
I didn't follow you here.
We should merge this in my opinion.
Some things like prompter:filter and prompter:selection-actions might be a bit polemical, but I still think it's the best we can do now.
Why re-order the changelog? Any benefit? I prefer adding the the bottom, it creates less noise in diffs.
Re-ordering also mostly breaks the history for the whole file: is it worth it?
Why re-order the changelog? Any benefit? I prefer adding the the bottom, it creates less noise in diffs.
It seems odd that we need to go to the bottom of a file to add an entry to the changelog. Regarding diffs, I don't see the difference it makes. Example?
Notice that the changelog was broken with regards to the order:
30:(define-version "2.0.0"
67:(define-version "2.1.0"
104:(define-version "2.1.1"
116:(define-version "2.2.0"
209:(define-version "2.2.1"
239:(define-version "2.2.2"
258:(define-version "2.2.3"
291:(define-version "2.2.4"
331:(define-version "3-pre-release-1"
440:(define-version "3.0.0"
If the order of old-to-new would be respected then 3-pre-release-1 would come last.
Re-ordering also mostly breaks the history for the whole file: is it worth it?
That is not entirely true. You can still follow the history and git blame, it just takes an extra step. But it's a fair point.
Still, if I revert this commit, I'll move 3-pre-release-1 to the very bottom.
I don't understand how can I mark as resolved the changes that @aartaka requested, without dismissing his review. Can we use less GitHub paraphernalia? It gets in the way.
I feel like lost and not remembering anything about the recent changes in this PR, but one thing I'm sure: changelog is better off staying as it is, because files naturally grown downwards, not upwards (why tho?) :D
If the order of old-to-new would be respected then
3-pre-release-1would come last.
Artyom did this. In any case, the pre-release entries can disappear from the final 3.0.0 changelog I think.
If the order of old-to-new would be respected then
3-pre-release-1would come last.
I don't understand the logic of this statement. 3.0.0 comes after 3-pre-release-1 (chronologically), so why say that 3-pre-release-1 comes after 3.0.0?
Artyom did this. In any case, the pre-release entries can disappear from the final 3.0.0 changelog I think.
Yes, they won't be there in 3.0.0, but it's nice to keep track of separate releases while we're doing them!
I feel like lost and not remembering anything about the recent changes in this PR, but one thing I'm sure: changelog is better off staying as it is, because files naturally grown downwards, not upwards (why tho?) :D
Files don't naturally grow downwards. That's a psychological barrier we inherited from writing on paper.
I've never seem a changelog that keeps old entries at the top, as it makes hardly any sense. See, for instance, Emacs changelogs, and the change log major mode Emacs provides (info "(emacs) Change Log").
I don't understand the logic of this statement. 3.0.0 comes after 3-pre-release-1 (chronologically), so why say that 3-pre-release-1 comes after 3.0.0?
Correct. I should have said that it makes little sense to have a changelog section corresponding to a release tag that doesn't exist!
Are we lying to ourselves about the existence of 3-pre-release-1? I don't know what to believe anymore...
Yes, they won't be there in 3.0.0, but it's nice to keep track of separate releases while we're doing them!
Agree. Any tagged release should have a corresponding changelog section, which I think we consistently do.
Are we lying to ourselves about the existence of 3-pre-release-1? I don't know what to believe anymore...
I'm not sure what you're expressing here.
I was suggesting that commit 517689ae1 might not have been a good move, a view that @Ambrevar seems to share.
(Edit: I pointed to the wrong commit).
I mean it's not a very big deal, it's just changelog. Let's adopt a workflow and stick to it :)
I'd expand the changelog with the value of each addition, like
Add nyxt/hint-mode:fit-to-prompt-p to configure whether the prompt takes the bare minimum of the screen space or not.
Add nyxt/hint-mode:show-hint-scope-p for Konkeror-style element highlighting.
Add height so that every prompt can have a configurable height.
But then, :nxref kinda shows it to those curious enough. The question is: do we want :nxref in changelog? Or are we better off with the plain HTML changelog that can be shown in any browser?
- Staying with plain HTML means we can publish the changelog right away on atlas.engineer. But it makes us think more on what to put in the text, like in my example above.
- Using
:nxref(and other niceties from Nyxt) makes changelog nicer to use inside Nyxt, but quirky to host at atlas.engineer.
Trade-offs...
To not prolong the review of this PR, I suggest using plain :code tags for now. I'll open an issue about this.
All addressed @aartaka. I just left the word "Konkeror" out of the changelog.
Ok to merge?
Nope, tests fail because of d63d692e624ccbde92333d726a785ab8a455b79d :P
And 4c868b3990771866c7dae0fea30fd6d8c84d94aa fixes it on master ;)