nyxt icon indicating copy to clipboard operation
nyxt copied to clipboard

Improve and extend `common-settings`

Open Ambrevar opened this issue 2 years ago • 16 comments

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):

  • On the existing buffers.
  • On the new buffers (customize-instance and customize-hook).
  • For the next session (auto-config).

Some very useful settings:

  • [ ] Ensure that user-defined (or extension-defined) keyschemes are listed.
  • [ ] Dynamically preview theme styles.
  • [ ] Place "Edit user files" at the top.
  • [ ] Restore session: always, ask, never.
  • [ ] Make default browser, see #2439.
  • [ ] Spell check language (it's a mode, should we have per-mode settings?)
  • [ ] Download directory.
  • [ ] Default modes.
  • [ ] Proxy (also a mode)
  • [ ] "set default new buffer URL" should have suggestions like "nyxt:new", "nyxt:dashboard".
  • [ ] Customize search engines.
  • [ ] Cookie settings.
  • [ ] Intelligent Tracking Protection (it's part of reduce-tracking-mode, but maybe we should factor it out).
  • [ ] Text editing keybindings.
  • [ ] Font families
  • [ ] Removal of the data files (clear history, as one use-case for that).
  • [ ] WebKit cache clearing per category — cookies, history, cache, and a dozen others.
  • [ ] WebKitSettings, like buffer background and default font, lots of tasty stuff there. We may need to somehow propagate those to all buffers, maybe even maintaining a general set of WwebKitSettings to apply by default.

Ambrevar avatar May 25 '22 08:05 Ambrevar

See https://github.com/atlas-engineer/nyxt/issues/2278 how it bit us recently.

Ambrevar avatar May 25 '22 08:05 Ambrevar

I believe you convinced me in the past that updating live instances is a bad idea, but I couldn't find that discussion.

aadcg avatar May 25 '22 11:05 aadcg

What is a "live instance"? Can you explain the problem?

Ambrevar avatar May 25 '22 12:05 Ambrevar

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.

aadcg avatar May 25 '22 14:05 aadcg

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.

Ambrevar avatar May 25 '22 17:05 Ambrevar

See also #2351.

Ambrevar avatar Jun 09 '22 11:06 Ambrevar

We actually need many actions for each setting:

  • save to auto-config
  • apply for new instances
  • apply to all instances
  • apply to all instances + new instances
  • apply to current instance.
  • apply to current instance + new instances

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?

Ambrevar avatar Jun 22 '22 12:06 Ambrevar

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.

Ambrevar avatar Jun 22 '22 12:06 Ambrevar

Maybe make our settings screen one huge HTML form? It's should be possible now that nyxt:// URLs imply strings by default.

aartaka avatar Jun 22 '22 12:06 aartaka

Hmm, what do you mean? Can you clarify?

Ambrevar avatar Jun 22 '22 13:06 Ambrevar

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:

  • If the URI has query parameters, apply the settings, then load the page.
  • If it doesn't, just load the page.

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!

Ambrevar avatar Jun 22 '22 14:06 Ambrevar

Yes, that's what I meant!

aartaka avatar Jun 22 '22 16:06 aartaka

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?

Ambrevar avatar Jun 23 '22 10:06 Ambrevar

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.

aartaka avatar Jun 23 '22 10:06 aartaka

@Ambrevar will these new features be part of 3.0?

aadcg avatar Apr 23 '23 19:04 aadcg

Most of it should be enabled by #2408.

Ambrevar avatar Apr 24 '23 09:04 Ambrevar