Class star

[Ambrevar]

Unfinished, do not merge!

This is to show-case the new define-class macro.

I decided to base it on top of hu.dwim.defclass-star because it saved me hours of work and I'm a bit short on time. If we ever need to replace defclass-star in the future, it won't be a problem.
hu.dwim.defclass-star has only 1 dependency, it's a rather light addition and it's already packaged for Guix.

Features

• All the defclass-star goodies, including global/local class/slot exports, automatic initargs and automatic accessors.

• Inferred initforms. "Do what I mean" by default (i.e. use the zero value for simple types, nil otherwise). The initform can always be specified manually. I'll make the inference fully customizable later.

• Cycle detection in superclasses: We can now extend classes like this

  (define-class foo (foo)
   ((extra-slot...))

Result

I've applied the change to the window class. As a result, we save more than 20 annoying lines. For bigger classes, the gain is even bigger.

No more redundancies which means no more risks for typos.

What's left to be done
The tests are not finished and I need to polish the zero-value inference.

Feedback welcome!

[Ambrevar]

To do: Move replace-class, with-class, etc. to this new library. Remove all the defclass-export macros.
This would save even more lines of code in the Nyxt code base ;)

[Ambrevar]

More to do: Add helpers to retrieve original class.

[Ambrevar]

I've implemented all the missing features.

I still need to do some more testing, then I'll merge.

@jmercouris: What do you think of my zero-value-inference customization? See the readme.

[jmercouris]

This is one of the first times I have just been absolutely speechless! I cannot stress enough how awesome this is. This is just fantastic!

I really like how we can utilize these objects so seamlessly for configuration of the program. After all, all of our code surface is API, so this really helps in deciding how we expose it, and automating that.

As per the zero-value inference- brilliant. Why isn't this done elsewhere? It seems so obvious now!

Of course, not everything is sunshine and rainbows. You forgot the spdx license header :-P
otherwise, I am excited for this to join master!

[Ambrevar]

One question: wouldn't it be better to do the other way around, i.e. infer the type from the initform? `type-of` is not idea here because `(type-of "")` returns `(SIMPLE-ARRAY CHARACTER (0))` for instance. So we could have a function like our basic-type-zero-values here that maps the return value of `type-of` to more useful types. Thoughts?

[Ambrevar]

Example: (cond ((subtypep (type-of ARG) 'string) 'string) ...)

[Ambrevar]

Implemented. I'm now using type inference in define-class for BUFFER. I'll update BROWSER, WINDOW and MINIBUFFER. I'll soon merge on master unless you want to keep this on hold.

[jmercouris]

Please, merge as soon as ready!

[jmercouris]

license header missing

[jmercouris]

license header missing

[jmercouris]

header

[jmercouris]

I was thinking about this, why is this a function instead of all of the invokations being at the top level? The idea being that you can set the renderer at runtime to something else?

[Ambrevar]

@aartaka: If I merge now it will create a few conflict with your current patch, is that OK?

[Ambrevar]

Initially I was using this trick for CCL. More generally, 2 reasons: - It groups all the impure, global settings in one spot. - I was planning to call (set-renderer) from (start) to avoid side-effects when loading the system. I haven't done it because it turned out that CCL worked without this trick. I still believe it's good practice to avoid ASDF-time side effects, so we should do it as some point.

[jmercouris]

OK, that makes some sense. Perhaps we'll drop it into start at some point then :-)

[Ambrevar]

Latest measures: class-star just allowed us to remove some 400 lines of code from the code base! This is not even counting the word count reduction.

[Ambrevar]

It seems to work alright so far, except for the last commit which makes the minibuffer blank, I don't know why. Could anyone test? @jmercouris?

[jmercouris]

I can test later today.

[jmercouris]

When trying to show the buffer list.

This is because of the fdeclaration:

debugger invoked on a TYPE-ERROR in thread
#<THREAD "main thread" RUNNING {10009E8083}>:
  The value
    #<NYXT:GTK-INTERNAL-BUFFER {101040C6A3}>
  is not of type
    NYXT:BUFFER
  when binding NYXT:BUFFER

Type HELP for debugger help, or (SB-EXT:EXIT) to exit from SBCL.

restarts (invokable by number or by possibly-abbreviated name):
  0: [RETURN-FROM-G-CLOSURE] Return value from closure
  1: [ABORT                ] Exit debugger, returning to top level.

(NYXT:SET-CURRENT-BUFFER #<NYXT:GTK-INTERNAL-BUFFER {101040C6A3}>) [external]
   source: (DEFUN SET-CURRENT-BUFFER (BUFFER)
             "Set the active buffer for the active window."
             (UNLESS (EQ 'MINIBUFFER (CLASS-NAME (CLASS-OF BUFFER)))
               (IF (CURRENT-WINDOW)
                   (WINDOW-SET-ACTIVE-BUFFER (CURRENT-WINDOW) BUFFER)
                   (MAKE-WINDOW BUFFER))
               BUFFER))
0] 0

[jmercouris]

I can confirm that the minibuffer is blank. I have no idea why that could be. It does function as expected...

[Ambrevar]

I've been boggling my mind around this issue for a while. It seems that
with our current model it's hard to avoid over complexity. Besides our
current class replacement within renderer-gtk already creates a mess with
class definitions, even when the user has no config.

Then I remembered a paradigm I once read: composition is often better
than inheritance because it's more versatile while it eliminates a whole
class of complexity.

Structure/Class composition is often done with mixins or equivalents.
In CLOS, we can leverage multiple inheritance.

I suggestion the following paradigm:

• For "abstract" (also known as "virtual") classes:

  • Never inherit from any other (Nyxt) class. (Currently it's the
    reason we break method specialization.)
  • Never redefine them. Currently this is our biggest induced complexity.
  • Never instantiate them.

• For "concrete" classes:

  • Don't define any slot, only specify the superclasses.
  • While I think it's possible to specialize methods against them, it
    might not be necessary.
  • These are the only classes that should be instantiated.

In practice, we would have the following classes which don't inherit
from anything:

• browser
• gtk-browser
• buffer
• gtk-buffer
• minibuffer
• internal-buffer
• web-buffer
• window
• gtk-window

And so on.

Then we would define the classes:

• (defclass user-browser (browser) ())
• (defclass user-web-buffer (web-buffer buffer) ())
• (defclass user-internal-buffer (internal-buffer buffer) ())

All calls to make-instance would use these last classe name, e.g. 'user-browser, etc.

Then in renderer-gtk:

• (defclass user-browser (gtk-browser browser) ())
• (defclass user-web-buffer (web-buffer gtk-buffer buffer) ())
• (defclass user-internal-buffer (internal-buffer gtk-buffer buffer) ())

We override the user-classes. Note that it does not modify the browser and buffer classes so it minimizes the side effects, it does not break method specialization and it does not mess up with class inheritance since gtk-buffer does not inherit from buffer.

If the user wants to change, say, the list of default modes for both web
buffers and internal buffers, they can do:

(defclass my-buffer ()
  ((default-modes ...)))

(defclass user-web-buffer (my-buffer web-buffer gtk-buffer buffer) ())
(defclass user-internal-buffer (my-buffer internal-buffer gtk-buffer buffer) ())

The define-configuration helper could generate my-buffer for us
(e.g. with (gentemp "BUFFER-CUSTOM")) and automatically redefine the
specified user-CLASSES, e.g.

(define-configuration (user-web-buffer user-internal-buffer)
  ((default-modes ...)))

which would roughly expand to the above, by adding the generated class
first in the list of superclasses (for highest precedence).

If we want to customize a class multiple times, no problem:

(defclass my-buffer2 ()
  ((default-url...)))

(defclass user-web-buffer (my-buffer2 my-buffer web-buffer gtk-buffer buffer) ()

Since define-configuration prepends the generated class to the list of
existing superclasses, this would work equivalently:

(define-configuration (user-web-buffer user-internal-buffer)
  ((default-modes ...)))
(define-configuration (user-web-buffer user-internal-buffer)
  ((default-url ...)))

@jmercouris Let me know if this approach seems sound to you and I'll
implement it.

EDIT: Added details about the user-CLASSES definitions in renderer-gtk.

[Ambrevar]

Great news! I've just updated the patch set to use class composition instead of inheritance, and it fixes everything! It also makes lots of stuff much, much simple. As you can see in the last commit, the changes were quite minimal. The main question that remains is "how shall we name the 'final' classes?" In the following: - I call "abstract" the class that contains the whole definition (all the slots); - I call "final" the class the only inherits from the abstract class + eventual specialization classes. Two strategies: 1 .For now I've prefixed the final class with "REPLACEME-". We need to find a better term of course ;) The benefit is that it induces very few changes (e.g. all the `buffer` occurrences need not change). The drawback is that the user must be aware that the instantiated object are all prefixed with REPLACEME-. 2. But we could also do the other way around and prefix the abstract classes (say with ABSTRACT-) and call the final classes by their canonical name. This would be prettier for the user, but it induces a bigger refactoring.

[jmercouris]

I need some more time to think about this. Thank you for a very deep investigation. I think that we are close to a good solution.

[jmercouris]

My first thought is that we can get a large quantity of simplification by reducing the requirements. For example, updating inheritees automatically may not be necessary.

[Ambrevar]

Another benefit of this strategy is that it's now possible to _remove_ classes from the composition. Before, if we had REPLACEME-buffer inherit from user-buffer1, which inherited from buffer, it was impossible to remove the customization induced by user-buffer1. Now we can remove user-buffer1 with something like this (untested): ```lisp (defmacro define-final-class (name &rest superclasses) `(defclass ,(intern (str:concat "REPLACEME-" (string name)) (,superclasses)))) (define-final-class buffer (delete 'user-buffer1 (mopu:direct-superclasses 'REPLACEME-buffer))) ```

[Ambrevar]

It may not, but it would trip the user many times for sure. A good example is the default-modes BUFFER slot. Enabling emacs-mode in BUFFER did mirror the change in INTERNAL-BUFFER. That said, it's OK, because it works now!

[phoe]

(define-final-class buffer (delete 'user-buffer1 (mopu:direct-superclasses 'REPLACEME-buffer)))

Calling the destructive function DELETE on the result of DIRECT-SUPERCLASSES, which returns the value of MOP:CLASS-DIRECT-SUPERCLASSES, is undefined behavior.

Use REMOVE instead for a non-destructive version, or explicitly call REMOVE-DIRECT-SUBCLASS. All in all, I'd suggest using the functional interface of ENSURE-CLASS instead of using macrology.

[Ambrevar]

@phoe: Thanks, both good points! Left to do: - Remove superclass cycle detection. - Rename REPLACEME- to user-.

[Ambrevar]

Done! This is ready to merge.

[phoe]

This decays into the integer 0. (Try it in the REPL!)

[Ambrevar]

Good catch! I've changed the logic to partition between floats and the rest of the number types

[phoe]

Do you support rationals? Perhaps you might want to form an exhaustive partition of reals into floats and rationals?

[Ambrevar]

Note that this is a zero-value inference, not a type inference. (Only exceptionally useful.)

ratio is a subtype of number, so the zero value is handled here (see the latest commits which partitions between floats and the rest).

[phoe]

The rational zero is 0, not 0.0, which is why I've actually made this comment. Sorry for being unclear.

[Ambrevar]

No problem, you were right, this didn't do the right thing for rationals. The latest commit should fix it.

[phoe]

Why is this here? The capture-and-resignal behavior seems silly, especially since 1) you explicitly discard information coming from the original error, 2) you destroy the stack coming from the original error by using handler-case instead of handler-bind.

[Ambrevar]

This trick is was not meant to expose conditions to the user, only to save me some 10 lines of code :/

It's not very useful and arguably confusing so I've replaced it with multiple return values (consistent with get-properties).

[phoe]

This is equivalent to (let ((*package* (find-package :nyxt/auto-mode))) ...)

[Ambrevar]

Good catch! Fixed.

[aartaka]

@aartaka: If I merge now it will create a few conflict with your current patch, is that OK?

It's okay, especially given that it will save lots of lines in the code of my patch at the cost of minor fixes :)

[jmercouris]

I've done some testing, it looks good to me. I like this solution. It is a good place that we can build on top of!