Settings backend

[hgluka]

Description

This is a continuation of (a part of) #2408.

Add a setting package for defining and applying configuration of Nyxt.
A setting can update a target class (by changing a slot value or by applying an arbitrary function to it) in the following places:

• the auto-config file
• a user-defined set of "live" instances
• all new instances (that will be opened in the current session)

This should provide a solid, convenient and testable back-end for configuring Nyxt.

Resolves #2296.

Discussion

Right now, I'm working on properly finding a writer for a slot. There is the closer-mop:slot-definition-writers function, but that only works for direct slots, meaning that, for example, it doesn't work very well for web-buffers, since most of its slots are inherited.

cc @Ambrevar

Checklist:

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

• Git hygiene:
  • I have pulled from master before submitting this PR
  • There are no merge conflicts.
• I've added the new dependencies as:
  • ASDF dependencies,
  • Git submodules,

    cd /path/to/nyxt/checkout
    git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser

  • and Guix dependencies.
• My code follows the style guidelines for Common Lisp code. See:
  • Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
  • 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.)
• Documentation:
  • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
  • I have updated the existing documentation to match my changes.
  • I have commented my code in hard-to-understand areas.
  • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
    • Changelog update should be a separate commit.
  • I have added a migration.lisp entry for all compatibility-breaking changes.
  • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
• Compilation and tests:
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works. (If possible.)
  • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

[jmercouris]

when you say module, are you using the Python notation to mean file?

[aadcg]

I think @hgluka meant package here. Then there's the fact that a single package is usually used in a .lisp file.

[hgluka]

That comment was left by @Ambrevar, but I assume he was talking about whether the setting file fits into the "Help" module along with help, manual, changelog, etc.

[aartaka]

That comment was left by Ambrevar, but I assume he was talking about whether the `setting` file fits into the "Help" module along with `help`, `manual`, `changelog`, etc.

Right! That's an ASDF module Pierre must've meant.

[hgluka]

The issue I was trying to solve here is that auto-configure uses customize-instance,
while extend-configuration uses customize-hook. This means that when you apply a setting
with :auto-config-p t and then restart Nyxt, applying another setting to that slot will have no effect on new instances.

Still, this seems to me like a very big change that could have unforeseen consequences.
What do you think? @Ambrevar

[aadcg]

@hgluka just a general piece of advice: write tests with regards to the customization facilities to capture all of the invariants. I can help you with some details if you get stuck.

[aartaka]

This means that when you apply a setting
with :auto-config-p t and then restart Nyxt, applying another setting to that slot will have no effect on new instances.
Still, this seems to me like a very big change that could have unforeseen consequences.
What do you think? @Ambrevar

I'm late to the party, but the change would only make the behavior more predictable and compatible with other define-configuration settings. Especially after #3163.

It's more confusing when one configures Nyxt via Common Settings, then adds some setting via define-configuration in config.lisp only to see that it's not applied whatever value they pick. So using customize-hook is a bugfix, not a breaking change.

[aartaka]

The issue I was trying to solve here is that `auto-configure` uses `customize-instance`, while `extend-configuration` uses `customize-hook`. This means that when you apply a setting with `:auto-config-p t` and then restart Nyxt, applying another setting to that slot will have no effect on new instances.

That's a perfect analysis of the issue. We should've moved to customize-hook (and/or define-configuration) a long time ago. I'm pretty sure it'll make things simpler and more predictable, with only a couple of cornercases (that are present in define-configuration anyway) staying broken. (These corner-cases are mainly the handler order and redefinition problems: if you redefine the buffer keyscheme to be Emacs, then VI, then back to Emacs—all in one file—then it might end up with either Emacs or VI. That's because customize-hook handler order is not guaranteed (but it should be pretty intuitive in most cases.) Anyway, this handler order problem happens regardless of customize-instance/customize-hook dichotomy, so let's ignoreit for now.)

[hgluka]

Hey, I've been working on this for about a month and I'm kind of getting stuck, so (as @jmercouris advised) I wanted to update everyone/ask for feedback and then maybe take a break from this PR and do something else before coming back to it.

The good part is that the apply-setting method works, it can update current instances, new instances and auto-configure. It can probably be used as is in the common-settings page and it can be tested just like define-configuration with it's own tests.

There are two big issues I haven't solved:

1. When we update current and future instances, we use objects (like theme:+dark-theme+, for example) directly, so those need to be serialized for auto-configure. I'm not sure if there's a good, general way to do this. There's also @aartaka's suggestion to rewrite it to use hooks instead of customize-instance, but that's mostly straightforward.
2. The settings work well enough for browser and buffer configuration but using them in the describe-* pages for arbitrary classes and slots is more difficult. It would work right now if we ignore live instances and just update new instances and auto-configure on those pages, but that's not an ideal solution.

If anyone has ideas for solving those two issues, let me know!

[aartaka]

The good part is that the apply-setting method works, it can update current instances, new instances and auto-configure. It can probably be used as is in the common-settings page and it can be tested just like define-configuration with it's own tests.

That's a huge progress!

1. When we update current and future instances, we use objects (like theme:+dark-theme+, for example) directly, so those need to be serialized for auto-configure. I'm not sure if there's a good, general way to do this. There's also aartaka's suggestion to rewrite it to use hooks instead of customize-instance, but that's mostly straightforward.

So rewriting it with hooks solves the problem? I'm not sure I get what the thing is about, can you expand on it?

2. The settings work well enough for browser and buffer configuration but using them in the describe-* pages for arbitrary classes and slots is more difficult. It would work right now if we ignore live instances and just update new instances and auto-configure on those pages, but that's not an ideal solution.

This problem is equivalent to "list all the instances of all classes" problem. Which, as far as I digged into the problem (I did, for quite some time) is hard and non-portable. SBCL has sb-vm:map-allocated-objects and that's as close to to properly searching instances as it can be. But other implementations don't have exported APIs for that. I'm pretty sure that CCL has something, but my search had no result yet. Allegro likely has some functionality for that, but it's proprietary and really obscure... Even though we are mostly building on SBCL, using its non-portable APIs is not a cool move.

We can hack up around this problem by keeping count on all the user-class-typed objects. We already hook into make-instance for these, so it shouldn't be hard. This solves the problem for all the Nyxt-related classes/instances at least. We can disable the configuration buttons for other classes—that'll be more honest anyway, because we actually can't guarantee any control over these.

This approach would be somewhat similar to what *inspected-values* do. Maybe it's a good opportunity to finally switch to implementation-provided IDs (#2991) and simply track all Nyxt objects by their IDs and their instances in some global table.

[hgluka]

1. When we update current and future instances, we use objects (like theme:+dark-theme+, for example) directly, so those need to be serialized for auto-configure. I'm not sure if there's a good, general way to do this. There's also aartaka's suggestion to rewrite it to use hooks instead of customize-instance, but that's mostly straightforward.

So rewriting it with hooks solves the problem? I'm not sure I get what the thing is about, can you expand on it?

No, sorry for the confusion, those two things are only related because they have to do with auto-configure.
The issue I'm having is with serialization. When we try to configure a slot like default-new-buffer-url, the value is a string. We can set the slot value directly and we can write it to the auto-config file and get this:

(defmethod customize-instance ((browser browser) &key)
  (setf (slot-value browser 'default-new-buffer-url) "https://example.com"))

That's good, everything works.
The problem comes up when we try to set the slot value to a more complex object, theme is a good example, but I assume there are others.
This is the auto-config file when we try to configure the theme slot for the browser to theme:+dark-theme+:

(defmethod customize-instance ((browser browser) &key)
  (setf (slot-value browser 'theme) #<theme:theme {1008530413}>))

We need the object there to be serialized, so that the file can be loaded on startup.

[aartaka]

I see now! A crude solution that I can come up with is comparing which slots in the theme:theme object (for the sake of example) had changed and construct a make-instance form with these. So, if the background-color and on-background-color swapped places, then the theme object might get serialized as:

(make-instance 'theme:theme :background-color "black" :on-background-color "white")

That's an incomplete solution, because current slot values for objects might be a result of some complex computation based on absolutely different initargs, but, I guess, that's the problem of the class with such complex computations 😄

I have a vague recollection of something like this instance diffing in our codebase, but that could well be an erroneous intuition.

[aartaka]

FYI: You can store raw Lisp objects inside compiled files, and you can recover them while loading compiled files. And it's an ANSI standard feature!

[hgluka]

In case someone else picks this PR up, I'll do a write up of what I've figured out so far:

In talking with @aadcg last Thursday, he seems to think this is basically ready, but there are some issues left to fix and work to be done.

The biggest issue is with writing non-serializable slots to the auto-config file. This one can be solved by somehow modifying auto-configure to include serialization (like what @aartaka proposed in the above comments) or by circumventing the problem entirely.

I'm not sure how easy or difficult the first option is, but the second one should be easier.
In the theme example, it should be possible to pass the name of the object along with the value. The name can then be written to auto-configure and the value can be applied to live instances and added as a hook to future instances.
The only problem I see with that approach is generalizing it.

Outside of that, there is some junk to clean up, rebase on master (which has the new common-settings page), etc.
Because the describe pages and common-settings both use it, modifying configure-slot to use settings is the easiest way to incorporate these changes there.

[aadcg]

@jmercouris if you have time, please give this branch a try and provide a report.

With regards to the theme, it's trivial to fix it. The slot value theme should set to a symbol, not an object.

[jmercouris]

There is too much wrong with this, and the system in general. I suggest /not/ merging this pull request. I need more time to think about how to do things properly. Possibly from the ground up.

[aadcg]

Let's keep the branch then. I'd suggest opening/reviewing issues on this topic and report what this iteration got right and wrong.