Turn the prompter function slots into methods?

[Ambrevar]

Prompters have a bunch of slots which are meant to be functions.

• prompt class:
  • constructor
  • before-destructor
  • after-destructor
• source class:
  • constructor
  • destructor
  • suggestion-maker
  • filter
  • filter-preprocessor
  • filter-postprocessor
  • resumer

There are also actions and selection-actions but, since it's a list of functions, methods would not do here.

Generic functions have many benefits over standard functions, but I can think of one drawback: Typically they are defined globally and methods must be specialized on classes, not objects.

We often have a use case like this:

(prompt
 :prompt "Reduce buffer(s)"
 :sources (make-instance 'buffer-source
                         :constructor (remove-if #'internal-url-p (buffer-list)
                                                 :key #'url)
                         :return-actions '(identity)
                         :multi-selection-p t))

Here it's convenient that we do not have to define a source only for the purpose of specializing the constructor.

Suggestion: go with generic functions but keep the constructor and *-destructor slots for local overrides.

Thoughts?

[Ambrevar]

Alternatively, we could have the source initializer behave differently for arguments like :constructor:

1. Add a method on the just-instantiated source:

   (closer-mop:ensure-method
   #'constructor
   '(lambda (source)
   (log:info "My specialization")
   `(a ,@(call-next-method) c))
   :specializers (list (closer-mop:intern-eql-specializer my-source)))

2. Remove the method in the destructor.

This would allow us to benefit from the full power of CLOS (note call-next-method above). The downside is that the prompter must then be invoked with a quoted constructor:

(prompt
 :prompt "Reduce buffer(s)"
 :sources (make-instance 'buffer-source
                         :constructor '(lambda (source)
                                        (log:info "My specialization")
                                        '(a b c))
                         :return-actions '(identity)
                         :multi-selection-p t))

[aadcg]

Your second suggestion seems quite elegant. Even though honestly I don't quite follow everything that would be happening (I need to study CLOS again).

[aadcg]

Now that I know prompter much better, I think the ideas here are interesting.

But I believe it would be risky to push such changes on the eve of a new big release.

[Ambrevar]

This would definitely be part of the Big Prompter Refactor. Whether we can make for 3.0 is another question... Let's see.

[aartaka]

Alternative solution for constructor etc. that doesn't require quoted forms and closure injection: :around accessors!

Look at this elegance. Slot definition:

(constructor #'some-lambda :accessor t)

and then an :around method for the magic:

(defmethod constructor :around ((arg prompter-class))
  (let ((result (call-next-method)))
    (etypecase result
      (function (funcall result arg))
      (list result))))

Given that constructor return data is restricted to lists, we have a pretty simple heuristic:

• If the slot value (as per the primary accessor) is a list, then return it.
• If the value is a function, then call it and return the result.

Neat, huh? This retains the slot, but preserves both method specialization per source CLOS-style (if one needs it), and the old-fashioned initarg&lambda one!

---

Not sure how this applies to other slots, but at least constructor, destructor, and filters (given some elbow grease, left as an exercise for the reader) are pretty easy to use this way.

EDIT: More clarifications and a call to implementation :)

[aadcg]

I anticipate that there will be some polishing after the Big Prompter Refactor. Such changes often require some time for the dust to settle.

I've put some effort in stabilizing the current implementation of the prompt buffer from a functional point of view and documentation-wise.

Also, I don't think that the Big Prompter Refactor will radically change the user experience. It will improve it, but it won't be a complete novel experience.

For these reasons, I think we should assign the task for post 3.0 so that there's enough time to mature the changes (in between two major releases, 3.0 and 4.0).

[aartaka]

One thing that I'd change before 3.0 is %current-suggestion. It's numbers and offsets, and it's a really inconsistent API, so making current suggestion to actually be a suggestion instead and indexing based on source contents. eq list search is cheap, mental juggling to work with indices is expensive.

[aadcg]

I'm not convinced about that design. I think @Ambrevar did it the way it is now for a good reason, namely performance. One thing that should be fixed is the fact that these offsets aren't sanity checked (for instance, whether they're within bounds).

[aartaka]

I think that this comment in our history-tree is summarizing the performance talk well enough:

;; TODO: Use fast sets for unique lookups?  Turns out hash-tables are overkill
;; with SBCL on modern hardware, for less than 10.000.000 entries.
;; Always use lists then?

In other words, only optimize it if it's actually a bottleneck :)

---

The design with list search is not perfect either, but it works well enough in REPL, for instance. I see no reason (except performance, which has to be proven) to exchange it for opaque indices instead of the nice'n'shiny CLOS objects and search for these :D

Even if there would be a performance problem, we can solve it by using vectors over lists (probably some 3x speedup on really big data) or by somehow memoizing/indexing data. So introducing C-style primitives like offsets should be the last resort :)

[Ambrevar]

While this specific part of the API can be deferred to 4.0, some other prompt buffer issues are more critical, like the freeze when you type to fast... :(

EDIT: To fix this freeze, I suspect we might have to switch to Lparallel, which would require a significant overhaul.

[aadcg]

like the freeze when you type to fast... :(

Indeed, if such critical issues are found, then they are a goal for 3.0. But I can't reproduce this bug. Is there an issue about it or a specific recipe?

[Ambrevar]

Yes: https://github.com/atlas-engineer/nyxt/issues/2134