search

jtbm37 / all-the-icons-dired

Adds dired support to all-the-icons
GNU General Public License v3.0
197 stars 26 forks source link

Speed up refresh and prevent line move from signaling #37

Closed wyuenho closed 4 years ago

wyuenho commented 4 years ago

This PR fixes this issue

Explanation:

dired-next-line moves character by character, and its calls to line-move-* signal errors, which stops timer functions from executing silently. This PR fixes both issues.

CC @conao3

conao3 commented 4 years ago

Have you ever measured how slow the dired-next-line is? Basically, I think we should use the API provided by dired instead of the low-level functions.

wyuenho commented 4 years ago

Benchmark of all-the-icons-dired--refresh using elp-instrument-function, running dired on my home directory (141 entries):

master:

Screen Shot 2020-03-26 at 14 13 10

This PR:

Screen Shot 2020-03-26 at 13 58 14

Also, as I said, line-move-* signal errors and you aren't handling them.

I think we should use the API provided by dired instead of the low-level functions.

No you shouldn't. This PR just get back to how it was implemented before #24. If it isn't broken, don't "fix" it. You've been going around the Emacs community introducing breakages all over the place for about a year now. I sincerely urge you to be a little more careful next time, and don't introduce changes without any good reason. You don't like it is not a good enough reason. It's not the first time someone reminded you of that.

conao3 commented 4 years ago

I used dired-k as a reference for overlay support for this package. The dired-k uses the dired-next-line, and it seemed like a better one than the forward-line, so I used it. Because dired is a huge package made up of a variety of hacks, I thought it needed to be accessed in the way dired envisions. Anyway, thanks to the benchmark, I found that dired-next-line is slow.

I'm not sure about the line-move-* signals. I don't see that error and it appears to be working fine. How does that signal adversely affect your Emacs?

jtbm37 commented 4 years ago

@wyuenho Thank you very much for your contribution and for taking the time to run those benchmarks.

I am happy with the changes and to merge it after the conflicts have been resolved.

P.S: Can we please avoid personal attacks ? Let's stay focused on the required changes and the motivation behind them. Thanks in advance.

wyuenho commented 4 years ago

@jtbm37 should be fixed now

jtbm37 commented 4 years ago

@wyuenho Thank you.