Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed #1218 aggressive debounce for dynamic-collection #2678

Closed
wants to merge 3 commits into from
Closed

fixed #1218 aggressive debounce for dynamic-collection #2678

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
143266a
fixed #1218 aggressive debounce for dynamic-collection
Sep 18, 2020
e0f57dd
additional style and optimization fixes for pull request #2678
Sep 19, 2020
7889102
style-change and refine more-chars case in ivy-input-change-p
Sep 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion ivy.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3227,12 +3227,29 @@ Otherwise, ~/ will move home."

(defvar ivy--exhibit-timer nil)

(defalias 'ivy-input-changed-p
Inc0n marked this conversation as resolved.
Show resolved Hide resolved
(let (last-len)
(lambda ()
(let ((len (length ivy-text))
(more-chars-len (ivy-alist-setting ivy-more-chars-alist)))
(prog1 (cond ((< len more-chars-len)
;; force ui update, changed
nil)
(t ;; changed if len is different
(not (eql len last-len))))
Inc0n marked this conversation as resolved.
Show resolved Hide resolved
(setq last-len len)))))
"Return t when lenght of `ivy-text' is changed,
But force return nil when current `ivy-text' len is smaller than `(ivy-alist-setting ivy-more-chars-alist)'.
This closure function stores a previous `ivy-text' length.")
Comment on lines +3241 to +3243
Copy link
Collaborator

@basil-conto basil-conto Sep 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@Inc0n Inc0n Sep 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@Inc0n Inc0n Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?



Inc0n marked this conversation as resolved.
Show resolved Hide resolved
(defun ivy--queue-exhibit ()
"Insert Ivy completions display, possibly after a timeout for
dynamic collections.
Should be run via minibuffer `post-command-hook'."
(if (and (> ivy-dynamic-exhibit-delay-ms 0)
(ivy-state-dynamic-collection ivy-last))
(ivy-state-dynamic-collection ivy-last)
(ivy-input-changed-p))
(progn
(when ivy--exhibit-timer (cancel-timer ivy--exhibit-timer))
(setq ivy--exhibit-timer
Expand Down