fixed #1218 aggressive debounce for dynamic-collection

[Inc0n]

I proposed a fix in my comment in #1218, here's the pull request, please advice!

[Inc0n]

All done! Please review

[basil-conto]

Thanks, LGTM apart from some minor comments.

[Inc0n]

Now that I have just implemented refinement on more-chars case in ivy-input-change-p, I consider it to be a closure predicate that serves more of the purpose to improve the user experience in ivy--queue-exhibit.
It would be more fitting to have it as an internal function.

But of course if there's any reasons for making this predicate public, that could also happen

Please advice! thanks

[basil-conto]

The Emacs convention (as per (info "(elisp) Documentation Tips")) is to have a complete summary sentence fit on the first line, keep the body wrapped at under 70 columns (see emacs-lisp-docstring-fill-column and M-q (fill-paragraph)), and generally refer to non-nil rather than t, unless t has a specific meaning. For example:

  "Return non-nil if length of `ivy-text' changed since last input.
Return nil if current `ivy-text' is shorter than what is required
by `ivy-more-chars-alist'."

Which leads to a follow-up question: what if the contents of ivy-text have changed, but its length is unchanged?

[Inc0n]

Hmm the case would be when the ivy-set-text has been called from functions that are not commands, the predicate would "lag" behind. It could leads to bad ui updates.

I'd sync the last-length by call ivy--input-changed-p in ivy-set-text', and allow ivy--input-changed-pto take an optionalupdate` argument to update change state when non-nil, otherwise it would return the cached change state, and set change state to nil.

Shall I commit this idea for you to review, or should we go with something different?

The doc string would look like this for the new predicate:

"Return non-nil if length of `ivy-text' changed since last input.
Return nil if current `ivy-text' is shorter than what is required
by `ivy-more-chars-alist'.  Return nil for all consequents calls
before `ivy-text' is changed. function takes an optional update
argument to update closure internal `changed' state."

[basil-conto]

Sorry, I'm not familiar with the relevant semantics of ivy-text, so I can't comment on the high-level design.

I don't understand what "all consequents calls before ivy-text is changed` means.

The fact that you're exposing an internal lexical variable via an optional argument, and have to document this in the docstring, makes it sound like we're trying too hard to use a closure here. Wouldn't a simple defvar+defun be simpler?

[Inc0n]

I would argue the two implementation is fairly similar, with the difference being the latter would expose the two internal variables.

(defalias 'ivy--input-changed-p
  (let (last-len
        changed)
    (lambda (&optioanl update)
      (if (not update)
          (prog1 changed
            ;; reset state
            (when changed
              (setq changed nil)))
        (let ((len (length ivy-text))
              (more-chars-len (or (ivy-alist-setting ivy-more-chars-alist)
                                  0)))
          (setq changed
                (and
                 ;; nil when less than more-chars
                 (>= len more-chars-len)
                 ;; changed if len is different
                 (not (eql len ivy--last-len))))
          (setq last-len len)))))
  "Return non-nil if length of `ivy-text' changed since last input.
Return nil if current `ivy-text' is shorter than what is required
by `ivy-more-chars-alist'.  Return nil for all consequents calls
before `ivy-text' is changed. function takes an optional update
argument to update closure internal `changed' state.")

(defvar ivy--last-len 0
  "stores the previous length of ivy-text")
(defvar ivy--input-changed nil
  "non-nil when input is last changed")

(defun ivy--input-changed-p ()
  (prog1 ivy--input-changed
    (when ivy-input-changed
      (setq ivy-input-changed nil))))

(defun ivy--compute-changed ()
  (let ((len (length ivy-text))
        (more-chars-len (or (ivy-alist-setting ivy-more-chars-alist)
                            0)))
    (setq ivy--input-changed
          (and
           ;; nil when less than more-chars
           (>= len more-chars-len)
           ;; changed if len is different
           (not (eql len ivy--last-len))))
    (setq ivy--last-len len)))

[Inc0n]

I don't understand what "all consequents calls before ivy-text is changed` means.

Ah if we want to introduce a new change state to counter the following issue:

what if the contents of ivy-text have changed, but its length is unchanged?

The change state after re-computation, i.e the synchronization function call placed within ivy-set-text. After accessing this state, should be set back to nil, otherwise it would be t all the time, when accessed, which would defeat the purpose of its implementation.

Therefore, I thought expose a flag to use the closure predicate as an update function when non-nil otherwise, it acts like the normal predicate function before we adding implementation to consider this issue:

Which leads to a follow-up question: what if the contents of ivy-text have changed, but its length is unchanged?

[kiennq]

Just asking but instead of debounce, wouldn't it better to allow to cancel the operation on user input instead? Something like #2567