Open Ambrevar opened 2 years ago
See https://github.com/atlas-engineer/nyxt/issues/2278 how it bit us recently.
I believe you convinced me in the past that updating live instances is a bad idea, but I couldn't find that discussion.
What is a "live instance"? Can you explain the problem?
As you've mentioned, writing to auto-config.lisp
doesn't update the live instances of the classes (or the live objects, if you prefer). If my memory doesn't fail me, there were reasons not to do this.
No particular reason beside that it was difficult to do it well and in particular to revert.
But now it should hopefully be easier and the configuration is fully functional, so everything should be reversible.
See also #2351.
We actually need many actions for each setting:
Having that many buttons for each setting is going to be very impractical. Instead, we ought to do it like other familiar UIs and add buttons at the top (and add keyboard shortcuts): when clicked, collect all the elements of the document that match some HTML class (e.g. with document get-elements-by-class
), collect the necessary details from there, then apply the configuration.
For this to work, we first need to fix https://github.com/atlas-engineer/nyxt/issues/2397.
Then we need something like the following:
;; TODO: Make class-name a required argument?
(defun apply-configuration (&key lambda slot (slot-value nil slot-value-p)
instances class-name auto-config-p new-instances-p)
(unless (and (or lambda slot)
(not (and lambda slot)))
(error "Either LAMBDA or SLOT must be provided."))
(let ((instances (delete-duplicates
(sera:filter (sera:eqs class-name) instances :key #'sera:class-name-of))))
;; For new instances:
(when new-instances-p
(apply #'extend-configuration class-name lambda slot
(when slot-value-p `(:slot-value ,slot-value))))
(when auto-config-p
(apply #'auto-configure
:class-name class-name
:slot slot
;; TODO: Rename this key arg to `:lambda' and have it funcall'ed.
:form lambda
(when slot-value-p
`(:slot-value ,slot-value))))
;; For existing instances:
(mapc (or lambda
(lambda (instance)
;; TODO: use accessor?
(setf (slot-value instance slot)
(if slot-value-p
slot-value
(slot-default instance)))))
instances)))
;; Example calls:
(apply-configuration
:lambda (lambda (input-buffer) (disable-modes '(nyxt/emacs-mode:emacs-mode) input-buffer))
:instances (buffer-list)
:class-name 'input-buffer
:auto-config-p nil
:new-instances-p t)
(apply-configuration
:slot 'theme
:slot-value 'my-theme
:instances nil ; Only for new future instances.
:class-name 'browser
:auto-config-p t
:new-instances-p t)
What's interesting here is that we are no longer quoting or generting any code, we are only dealing with proper lambdas.
extend-configuration
is a new function that adds the lambda / slot config to the nyxt::customize-hook
like define-configuration
. There is some code factoring to be done here.
Thoughts?
Also, apply-configuration
could be tested in its own suite, so that would make it reliable.
A better test suite would be one that fills the Common Settings HTML then calls the command to apply the configuration. That would test the collection of settings done over the HTML.
To do this, I suppose that we could give an HTML ID to each element that can be set (drop down menus, text fields, etc.), then the test suite can set the various elements using some JS.
Maybe make our settings screen one huge HTML form? It's should be possible now that nyxt://
URLs imply strings by default.
Hmm, what do you mean? Can you clarify?
Oh, I see, I think.
You mean we would add a submit button, then upon clicking it would load something like nyxt://custom-settings/?scheme=cua&zoom-level=1.5
, so the custom-settings
internal page must change it logic:
To submit the form proagrammatically (for instance so that we can create a key binding for it), we could use the following JS:
document.forms["myform"].submit();
Is this what you meant? I think that would work, and that might be less code than the manual collection I suggested. Good one!
Yes, that's what I meant!
So i tried the put the following in common-settings
:
(:form (:label :for "name"
"Name")
(:input :type "text"
:id "name"
:name "name")
(:input :type "submit"
:value "Submit"))
Now if I press submit, it fails because the method signature is wrong.
If I add &key name
to the method signature, it works!
But of course it's not very convenient to keep the form ID in sync with the function keywords arguments.
So instead I could use the &rest
argument. But internal pages do not allow them. Should we enable them then?
The keyword thing is intended to keep strict 1-1 relation betweenquery args and keyword args.
But &rest
can work too, I suppose. Note that we'd be parsing it anyway, so not much difference there. If you have some smarter way to process keywords, them I'm okay with &rest
. Otherwise I'm on the side of explicit (even if reader-macro-generated) argument list.
@Ambrevar will these new features be part of 3.0?
Most of it should be enabled by #2408.
common-settings
is for now a bit barren but we can easily see how it can be expanded to be a full-fledged configuration panel.Now the problem is that it's very fragile and it has broken so many times in the past. Indeed, since it's mostly made of arbitrary forms, there are barely any checks done, ever.
Besides, for now we just write to
auto-config.lisp
and we don't update the live instances.I believe we could hit 2 birds with one stone: by updating the live instances, we might be able to perform more checking.
To sum up, each settings should be applied at 3 levels (optionally letting the user choose where):
customize-instance
andcustomize-hook
).auto-config
).Some very useful settings:
reduce-tracking-mode
, but maybe we should factor it out).