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.

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

Suggestion: Only redraw search buffer after refresh completes #293

Open
alphapapa opened this issue Sep 9, 2018 · 5 comments
Open

Suggestion: Only redraw search buffer after refresh completes #293

alphapapa opened this issue Sep 9, 2018 · 5 comments

Comments

@alphapapa
Copy link
Contributor

Hi Chris,

I've noticed that doing a full refresh of all feeds seems to complete much faster if I use a search filter that doesn't match any entries (e.g. =asdf, which matches no feeds). If I use my default filter, it is much, much slower, I'm guessing because it's redrawing the entries after each feed is updated.

Would it make sense to have an option to only redraw the search buffer after all feeds have been updated?

@skeeto
Copy link
Owner

skeeto commented Sep 9, 2018 via email

@alphapapa
Copy link
Contributor Author

I'm fine with having an option that would prevent refiltering and redraw after a database update if the fetch queue is not empty.

That sounds like a good idea to me. Two issues about that:

  1. AFAICT there is only a queue for the curl backend. I use curl, so that's fine with me, but this improvement wouldn't have any effect for users who don't use curl.

  2. Implementing this seems more complicated than I expected, but maybe that's just because I'm unfamiliar with the code. It seems like the best place to check the queue length is in elfeed-search-update. But adding the check here seems awkward, because the function is called with the force argument by the calls to the update hooks (by way of calling elfeed-search-update--force), so the queue check would have to come before the force check in the conditional, which would mean that force would no longer truly mean force.

Should we change the update hooks to call elfeed-search-update instead of elfeed-search-update--force? That would make the queue check more natural, but I'm not sure if that's the right way to do it.

Thanks.

@alphapapa
Copy link
Contributor Author

  1. I found the url-queue, so nevermind about that issue.
  2. I'm making a hacky workaround in my config that saves and restores the filter around the update, but it'd be better to avoid the filtering and redraw altogether. Let me know what you think about how to do that.

Thanks.

@alphapapa
Copy link
Contributor Author

Well, my hacky workaround doesn't work very well. In it, I save the search filter to a variable, then set it to #0, and then start the update. When the queue is empty, I restore the search filter. The problem is that elfeed-update-hooks is called every time a feed finishes updating, rather than when all feeds are finished updating. So it seems that elfeed-search-update is called once for every feed that was updated. This takes over 60 seconds for me.

The asynchronous nature of elfeed-update-feed seems to make this more difficult. I guess a counting semaphore could be used, but I can easily imagine that getting stuck somehow (e.g. if quit is called during an update). Maybe you know a better way.

Here's the code I'm using:

(defvar ap/elfeed-update-complete-hook nil
  "Functions called with no arguments when `elfeed-update' is finished.")
(defun ap/elfeed-update-complete-hook (&rest ignore)
  "When update queue is empty, run `ap/elfeed-update-complete-hook' functions."
  (message "%s: Queue length: %s"
           (format-time-string "%c")
           (length (if elfeed-use-curl
                       elfeed-curl-queue
                     url-queue)))
  (unless (if elfeed-use-curl
              elfeed-curl-queue
            url-queue)
    (run-hooks 'ap/elfeed-update-complete-hook)))
(add-hook 'elfeed-update-hooks #'ap/elfeed-update-complete-hook)

;; Show message when feeds are finished updating
(defun ap/elfeed-update-message-completed (&rest _ignore)
  "Show message if update queue is empty."
  (message "Feeds updated"))
(add-hook 'ap/elfeed-update-complete-hook #'ap/elfeed-update-message-completed)

;; Unset and reset view filter when updating feeds
(defvar ap/elfeed-search-update-filter nil
  "The filter when `elfeed-update' is called.")
(defun ap/elfeed-search-update-restore-filter ()
  "Restore filter after feeds update."
  (when ap/elfeed-search-update-filter
    (elfeed-search-set-filter ap/elfeed-search-update-filter)
    (setq ap/elfeed-search-update-filter nil)))
(add-hook 'ap/elfeed-update-complete-hook #'ap/elfeed-search-update-restore-filter)

(defun ap/elfeed-search-update-save-filter ()
  "Save and change the filter while updating."
  (setq ap/elfeed-search-update-filter elfeed-search-filter)
  (setq elfeed-search-filter "#0"))
;; NOTE: It would be better if this hook were run before starting the feed updates, but in
;; `elfeed-update', it happens afterward.
(add-hook 'elfeed-update-init-hooks #'ap/elfeed-search-update-save-filter)

And here's the log output:

Thu 20 Sep 2018 03:39:31 AM CDT: Queue length: 8
Thu 20 Sep 2018 03:39:31 AM CDT: Queue length: 7
Thu 20 Sep 2018 03:39:31 AM CDT: Queue length: 6
Thu 20 Sep 2018 03:39:31 AM CDT: Queue length: 5
Thu 20 Sep 2018 03:39:32 AM CDT: Queue length: 4
Thu 20 Sep 2018 03:39:32 AM CDT: Queue length: 3
Thu 20 Sep 2018 03:39:32 AM CDT: Queue length: 2
Thu 20 Sep 2018 03:39:32 AM CDT: Queue length: 1
Thu 20 Sep 2018 03:39:32 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:38 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:41 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:43 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:46 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:49 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:52 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:55 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:39:57 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:03 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:04 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:07 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:11 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:14 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:17 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:18 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:22 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:25 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:28 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:31 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:32 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:36 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:39 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:42 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:45 AM CDT: Queue length: 0
Feeds updated
Thu 20 Sep 2018 03:40:47 AM CDT: Queue length: 0
Feeds updated

@alphapapa
Copy link
Contributor Author

alphapapa commented Sep 29, 2018

Well, I made the workaround a little hackier, but it seems to have worked. When elfeed-update-feed is called, I increment a special var, ap/elfeed-updates-in-progress. Then when elfeed-update-hooks are run, I decrement it, and if it's 0, I restore the search filter. This doesn't stop the search buffer from being refreshed, but the #0 filter prevents it from drawing any entries until the previous filter is restored. So when I update all feeds, the search buffer is only refreshed once, when all feeds have finished updating.

There's probably a potential bug in that, if the var is decremented after its value is checked, the filter wouldn't be restored, so I would need to ensure that the hooks are added in the correct order.

This is messy and not ideal, but at least it works around the issue for me. I can update all of my feeds, the search buffer is only redrawn once, so it only takes a few seconds instead of a few minutes. And it won't take longer and longer the more feeds I add. :)

Here's the complete code (similar to the above, but with the new parts):

(defvar ap/elfeed-update-complete-hook nil
  "Functions called with no arguments when `elfeed-update' is finished.")

(defvar ap/elfeed-updates-in-progress 0
  "Number of feed updates in-progress.")

(defvar ap/elfeed-search-update-filter nil
  "The filter when `elfeed-update' is called.")

(defun ap/elfeed-update-complete-hook (&rest ignore)
  "When update queue is empty, run `ap/elfeed-update-complete-hook' functions."
  (when (= 0 ap/elfeed-updates-in-progress)
    (run-hooks 'ap/elfeed-update-complete-hook)))

(add-hook 'elfeed-update-hooks #'ap/elfeed-update-complete-hook)

(defun ap/elfeed-update-message-completed (&rest _ignore)
  (message "Feeds updated"))

(add-hook 'ap/elfeed-update-complete-hook #'ap/elfeed-update-message-completed)

(defun ap/elfeed-search-update-restore-filter (&rest ignore)
  "Restore filter after feeds update."
  (when ap/elfeed-search-update-filter
    (elfeed-search-set-filter ap/elfeed-search-update-filter)
    (setq ap/elfeed-search-update-filter nil)))

(add-hook 'ap/elfeed-update-complete-hook #'ap/elfeed-search-update-restore-filter)

(defun ap/elfeed-search-update-save-filter (&rest ignore)
  "Save and change the filter while updating."
  (setq ap/elfeed-search-update-filter elfeed-search-filter)
  (setq elfeed-search-filter "#0"))

;; NOTE: It would be better if this hook were run before starting the feed updates, but in
;; `elfeed-update', it happens afterward.
(add-hook 'elfeed-update-init-hooks #'ap/elfeed-search-update-save-filter)

(defun ap/elfeed-update-counter-inc (&rest ignore)
  (cl-incf ap/elfeed-updates-in-progress))

(advice-add #'elfeed-update-feed :before #'ap/elfeed-update-counter-inc)

(defun ap/elfeed-update-counter-dec (&rest ignore)
  (cl-decf ap/elfeed-updates-in-progress)
  (when (< ap/elfeed-updates-in-progress 0)
    ;; Just in case
    (setq ap/elfeed-updates-in-progress 0)))

(add-hook 'elfeed-update-hooks #'ap/elfeed-update-counter-dec)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants