Overhaul settings

[Ambrevar]

Description

This does multiple things:

• Adds a new apply-configuration helper to apply configuration to auto-config, current-instance, new instance, all instances.
• Turn Common Settings into a form-based page, as per @aartaka suggestion.
• Connect the above two!

This frees us from having to code the settings in nyxt/lisp-eval forms, which are a bit brittle and would involve complex PS. Here we are mostly in pure Lisp.

Fixes #2296

Discussion

The value we get for themes is a symbol, e.g. theme:+light-theme. So to effectively switch theme, we have to call eval on it, which is ugly!

Possible fix: Make a dark-theme subclass and call (make-instance 'dark-theme) to set the slot of the browser.

@aartaka ?

To do:

• [ ] Turn rest of Misc settings into the new HTML form.

• [ ] Test that it saves to auto-config properly.

• [ ] Test that it applies to new instances.

• [ ] Test that it applies to multiple instances.

• [ ] Reset "Common Settings" URL after form has been received.

• [ ] Update configure-slot to use the same API.

• [ ] The settings function should probably be new first- class, just like commands.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

• [ ] I have pulled from master before submitting this PR
• [ ] My code follows the style guidelines for Common Lisp code
  • See Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF) and Google Common Lisp Style Guide.
• [ ] 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)
• [ ] I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [ ] My changes generate no new warnings
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] New and existing unit tests pass locally with my changes
• [ ] There are no merge conflicts

[Ambrevar]

@jmercouris @aadcg @aartaka Please let me know if my approach seems reasonable to you.

[aartaka]

I'll be able to test in a day or two, if you don't mind :)

[aadcg]

Same here!

[Ambrevar]

@aartaka I cannot get lisp-eval callbacks to work. First, it's currently broken because chain should be prefixed with ps: since it's no logner in the Parenscript package. But even then, in

(ps:when ,callback
         (ps:chain request
                   (then (lambda (response)
                              (when (@ response ok)
                                (ps:chain response (json)))))
                   (then ,callback)))

The response.ok is always false. From the JS console I see:

Response

body: null

bodyUsed: false

headers: Headers {append: function, delete: function, get: function, has: function, set: function, …}

ok: false

redirected: false

status: 0

statusText: ""

type: "opaque"

url: "" 

Any idea? Something wrong on cl-webkit's side?

[Ambrevar]

See #2079: maybe it broke it.

[Ambrevar]

I won't have much time today and this week... @aartaka @aadcg Any chance one of you would have time to finish this? It's an easy one, just need to make the new "settings" functions with the corresponding HTML.

[aartaka]

Count me in!

[Ambrevar]

Thanks!

[Ambrevar]

Shall we wait for this for the pre-release-1 or shall we release now then?

[aartaka]

Hmmmmm...

[aartaka]

My perfectionism makes me want to bring this into the pre-release, but it's unlikely that I'll get to it today, so maybe we should not wait anymore :D

[Ambrevar]

I'm running late today, I'll do the release tomorrow morning anyways.

[aartaka]

Then let's agree this PR is not included in release tomorrow to not rush ourselves :D

[Ambrevar]

OK!

[Ambrevar]

To do: Use closer-mop:slot-definition-writers.

[Ambrevar]

@aartaka Can you summarize your latest changes?

[aartaka]

I've merely fixed some errors that I encountered while playing with this branch, and added a new interface type ("option") for configuration entries.

[aartaka]

Getting back to this PR, I don't like the direction it takes, in several aspects:

• The classes introduced for the purpose of configuration generation complicate reading and understanding the thing a lot—there are too much layers to have the whole picture in one's mind.
• The whole system seems to have been designed top-down, with all the possible use-cases that a configuration system might have, pre-baked into it.
• There's no direct correspondence between our object system and configuration, we have to conjure a whole set of new entities just to configure a class slot. Fells like downplaying the power of CLOS/MOP/user-classes here.

Given this, I want to try another course of action:

• Start with
  • slot-only,
  • restart-required,
  • MOP-backed,
  • auto-generated HTML form configuration.
• Expand it with runtime options and misc options (maybe even leave those as-is now, because there are always exceptions).
• And make it rather a Spinneret tag inspecting classes than classes generating HTML.

As an external API I suggest a Spinneret tag, say :nconfig:

(:nconfig
 (browser
  restore-session-on-startup-p
  open-external-link-in-new-window-p
  (default-new-buffer-url :choices (list "nyxt:new" "nyxt:dashboard" "https://nyxt.atlas.engineer"))
  native-dialogs
  (theme :choices (list theme:+light-theme+ theme:+dark-theme+))
  external-editor-program
  style)
 (web-buffer
  smooth-scrolling
  search-auto-complete-p
  search-always-auto-complete-p
  keep-search-hints-p
  zoom-ratio-default
  (style :type string))
 (window
  message-buffer-height
  prompt-buffer-open-height
  (message-buffer-style :type string))
 (status-buffer
  height
  glyph-mode-presentation-p
  (style :type string)))

Inferring the types of most slots via MOP, and using the :type or :choises field when unclear or overriden, and generating HTML tags for different types, like generating <input type=number> for integerslots, <input type=radio> for boolean slots, and some <select>s for member types, passing raw Lisp data (nyxt: URLs allow tht) to apply-configuration instead of some pre-set values or function processors.

This way our configuration would be:

• Extensible (much like the current one, but with less entities).
• Embeddable (even for extensions).
• Inferred/inspectable (via MOP).
• Simple (a mere Spinneret macro/widget with some MOP magic underneath, instead of a set of classes designed specifically for the purpose of configuration).

Does that sound sane? (゜▼゜＊）

[Ambrevar]

Yes for the Spinneret approach.

However I disagree that it should be "restart only": this would severely limit us (and limit the user) and the API we would thus design is almost guaranteed to not be extensible enough to accommodate for a "not just restart only" enhancement in the future.

[aartaka]

Fair ( ͡° ͜ʖ ͡°)

[Ambrevar]

Quick summary for whoever is going to resume work on this (maybe @hgluka, maybe someone else, maybe me :p):

1. Work on a new branch :)
2. Rebase on master (should be relatively easy).
3. Replace the heuristic to find writer methods with the CLOS way to do it (e.g. c2mop:slot-definition-writers).
4. Make use of slot type information to guard against bad input in apply-configuration?

[Ambrevar]

Let me clarify a bit the design behind this pull request and the discussion I had with @aartaka about leveraging spinneret tags:

The pull request has 2 parts:

• A "backend" which is centered around apply-configuration and which could be potentially extracted to a separate library and be used in any other CL project. (Including one that does not use HTML as a UI.)
• A "frontend" which is for now just the overhaul "Common Settings" page, and which this pull request defines "manually".

If I understood Artyom correctly, he suggested to leverage spinneret tags to write simplify the declaration of the "Common Settings" page. Note that this won't have an impact on the "backend". I suggest that we do this but in a later pull request. For now it's OK to have a "manual" Common Settings page, a robust backend is the current focus.

[Ambrevar]

Note about the commented (setf (gethash :keystyle *settings*) ...) and friends: I left this behind to show what I had tried to specialize some settings, but this is all rather ugly.

Instead, a CLOS approach as done in the latest commit seems much better: apply settings in the most generic fashion, only to specialize with setting subclasses whenever necessary.

[hgluka]

@Ambrevar Thanks for the write-up! I'll start working on the "back-end" first. As for rebasing (a new branch) on master, I'll just take the last commit from this branch. A lot of the previous ones have to do with the common-settings page and I think that can wait for the "front-end" PR.

[jmercouris]

Please reopen when ready.