jtbm37 / all-the-icons-dired

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

Re: Use overlay for displaying icons #24

Closed conao3 closed 4 years ago

conao3 commented 4 years ago

This PR is improved version from #21. This PR adopt file deletion. How about this, @jtbm37, @seagle0128 and @buzztaiki?

jtbm37 commented 4 years ago

Thanks for your contribution.

Which emacs version are you on ?

I am on 26.1 and these changes break dired.

I am getting this error when trying to visit a folder in dired.

run-hooks: Wrong number of arguments: ((t) (fn &rest args) "Advice function for FN with ARGS." (apply fn args) (if (derived-mode-p (quote dired-mode)) (progn (all-the-icons-dired--refresh)))), 0 [3 times]

This doesn't play well with another mode I am using, dired-narrow.el. When filtering the directory, icons from non matching items are all displayed in a single line.

conao3 commented 4 years ago

I use this PR in Emacs-26.3. I reinvestigate deeply.

conao3 commented 4 years ago

I forgot to remove the hook I specified for debugging. Could you try it again?

conao3 commented 4 years ago

I may be too commit to this branch. I split PR if you want.

conao3 commented 4 years ago

ping? This patch also solve align issue, please consider to merge it.

after

Screenshot_2020-01-25_12-08-44

conao3 commented 4 years ago

I've mistakenly sent the comment unrelated to this PR... But this PR is OK for me, please review it.

conao3 commented 4 years ago

ping again?

Linuus commented 4 years ago

Works great. Left a minor suggestion.

One thing I noticed is that the overlay covers the current line highlight. Is it possible to make the hl-line overlay cover this overlay?

conao3 commented 4 years ago

Yes, I think the indent technic is also needed. But this PR only translates the current string from being inserted into an overlay. It is not a good idea to deal with many things with one PR. However, it is a different problem that the ping is not received and this PR is left long time...

Linuus commented 4 years ago

Ping @jtbm37

Perhaps you can add another maintainer to this project?

seagle0128 commented 4 years ago

I like this PR. Gently ping @jtbm37 .

jtbm37 commented 4 years ago

I have been testing for the past couple of the days. It looks good.

Thanks for your contribution @conao3

conao3 commented 4 years ago

Thanks!

It may be reasonable to add someone who is participating in this conversation as a collaborator in order to promote communication between developers.

wyuenho commented 4 years ago

This PR refreshes the entire dired buffer line by line on pretty much every operation, as if linearly searches of icons for every file isn't slow enough already. Not only is this PR insanely slow, the advice on dired-readin also interferes with window-purpose 's code1 extension on emacs 27. Anything that pops up a new buffer and moves point to a specific line now jumps back a few hundred lines.

Please revert.

conao3 commented 4 years ago

Thank you for the report. Don't just suggest moving the code back three years, but please suggest ways to improve it. Let's cooperate.

A linear search would be wasteful I think too. But I can't feel the performance getting worse. If you're stressed out, implement local (implement in this package) cache or suggest an equivalent for upstream.

the advice on dired-readin also interferes with window-purpose 's code1 extension on emacs 27.

Please show us the code and inform us concrete problem with this PR. As I use Emacs-26.3, I don't familiar Emacs HEAD information.

Anything that pops up a new buffer and moves point to a specific line now jumps back a few hundred lines.

If there are too many rows displaying icons in relation to the first problem, we may be able to reduce the performance penalty by temporarily disabling all-the-icons-dired processing. https://github.com/seagle0128/.emacs.d/blob/572fe09a4b98e31714c4dde177a661cd14e2743e/lisp/init-dired.el#L92 You don't show concrete regenerate steps too, so I can't deal with it.

Linuus commented 4 years ago

@conao3 I think there are issues here when renaming a file. When I rename a file the icon ends up on the far left.

Screenshot 2020-03-16 at 19 51 05
conao3 commented 4 years ago

I create separate issue; #34. Please move to the issue!