~protesilaos/general-issues

7 3

dired-preview: Hook-based (reactive) instead of rebinding

Details
Message ID
<m1zg4noej2.fsf@christiantietze.de>
DKIM signature
missing
Download raw message
Hi Prot,

Great to hear to iterate on the peep-dired package! (I'm a happy user.)

I found that rebinding the dired keys composed worse than hooks; with
hooks, you can preview whichever file point is on easily. With rebinding
keys, you would need to account for a lot of special cases.

I documented the issue here in 2020:
https://github.com/asok/Peep-dired/issues/18

So my suggestion would be to make similar adjustments to:
https://git.sr.ht/~protesilaos/dired-preview/tree/main/item/dired-preview.el#L283-288

Cheers,
Christian

-- 
Sent from Bielefeld, Germany <3
https://christiantietze.de -- Programming + Personal
https://zettelkasten.de    -- Creative Knowledge Work
Details
Message ID
<b3f71eb9-590e-6106-e3db-802db1eb7c2e@gmail.com>
In-Reply-To
<m1zg4noej2.fsf@christiantietze.de> (view parent)
DKIM signature
missing
Download raw message
Hi,

Very nice package and I expect it to be very useful.

I'm Just an end-user so I don't understand Christian's explanation, but 
that is the first thing I noticed - that it doesn't work with the up and 
down arrow keys.

Also, if you land in a directory by doing p, p, etc, then <enter> on the 
folder name, the point lands on the first file listed. Preview does not 
work there - maybe that is intended.

Ed Hamilton

edhamilton9@gmail.com
Details
Message ID
<87y1k6oigp.fsf@protesilaos.com>
In-Reply-To
<b3f71eb9-590e-6106-e3db-802db1eb7c2e@gmail.com> (view parent)
DKIM signature
missing
Download raw message
> From: Ed Hamilton <edhamilton9@gmail.com>
> Date: Sun, 25 Jun 2023 16:12:50 -0400
>
> Hi,

Hello Ed, Christian,

> Very nice package and I expect it to be very useful.

You are welcome!

> I'm Just an end-user so I don't understand Christian's explanation, but 
> that is the first thing I noticed - that it doesn't work with the up and 
> down arrow keys.

What Christian suggests is that instead of triggering the effect via a
command bound to a key, we do it automatically after the 'dired-mark' is
set.

Christian, since you already have the proposal ready, do you want to
prepare a patch for it?

> Also, if you land in a directory by doing p, p, etc, then <enter> on the 
> folder name, the point lands on the first file listed. Preview does not 
> work there - maybe that is intended.

Good to know!  Maybe Christian's approach will work in this case.

All the best,
Protesilaos (or simply "Prot")

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<m1ttuuo7fe.fsf@christiantietze.de>
In-Reply-To
<87y1k6oigp.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Hi Prot & Ed!

>> I'm Just an end-user so I don't understand Christian's explanation, but 
>> that is the first thing I noticed - that it doesn't work with the up and 
>> down arrow keys.
>
> What Christian suggests is that instead of triggering the effect via a
> command bound to a key, we do it automatically after the 'dired-mark' is
> set.

This would be the ideal, but dired.el doesn't notify "interested parties"
when the selected line changes. There's no hook, for some reason. This
could be a useful addition upstream in the Emacs source code, but at the
moment, we're stuck with key rebindings.

> Christian, since you already have the proposal ready, do you want to
> prepare a patch for it?

With these limitations in mind, please find a patch attached that shows
how I used to "fix" updating the preview for marking filed with keys
like `d' or `m'.

Code review questions:

- Should we use a lambda instead of a named function?

I'll think about key rebindings a bit more in the meantime :)

Cheers,
Christian

-- 
Sent from Bielefeld, Germany <3
https://christiantietze.de -- Programming + Personal
https://zettelkasten.de    -- Creative Knowledge Work
Details
Message ID
<m1r0pyo711.fsf@christiantietze.de>
In-Reply-To
<m1ttuuo7fe.fsf@christiantietze.de> (view parent)
DKIM signature
missing
Download raw message
> This would be the ideal, but dired.el doesn't notify "interested parties"
> when the selected line changes. There's no hook, for some reason. This
> could be a useful addition upstream in the Emacs source code, but at the
> moment, we're stuck with key rebindings.

One more thing:

`dired-move-to-filename' is apparently called after most (all?)
selection and movement commands, so maybe we could figure out to advise
this function instead. Doing it right now will result in a stack
overflow of sorts because the preview calls `dired-move-to-filename',
too.

Another suggestion would be to grep for all callers of this function in
dired.el, assemble a list of these functions, and advise each and every
one of them.

That's quite brittle, though. And similar to key rebinding, it will
not recognize user functions that modify the selected line.

Maybe key rebinding *is* the best way at the moment?

-- Christian

-- 
Sent from Bielefeld, Germany <3
https://christiantietze.de -- Programming + Personal
https://zettelkasten.de    -- Creative Knowledge Work
Details
Message ID
<87zg4lk1i3.fsf@protesilaos.com>
In-Reply-To
<m1r0pyo711.fsf@christiantietze.de> (view parent)
DKIM signature
missing
Download raw message
> From: Christian Tietze <me@christiantietze.de>
> Date: Mon, 26 Jun 2023 12:24:42 +0200
>
>> This would be the ideal, but dired.el doesn't notify "interested parties"
>> when the selected line changes. There's no hook, for some reason. This
>> could be a useful addition upstream in the Emacs source code, but at the
>> moment, we're stuck with key rebindings.
>
> One more thing:
>
> [... 13 lines elided]
>
> Maybe key rebinding *is* the best way at the moment?

Thank you, Christian!  I installed your patch.  I think the advice
mechanism is better suited for this purpose.  With the ":after", we are
using it in practically the same way as a hook.  I thus generalised what
you did in the following commit:

    commit 3304286c175387602032a15a7d1888473ca054b0
    Author: Protesilaos Stavrou <info@protesilaos.com>
    Date:   Tue Jun 27 06:42:04 2023 +0300

        Use an advice instead of defining new commands; delete our keymap

        This is the same ideas as in commit ae93720, except more generalised.
        Instead of defining our own commands, we advise Dired to display a
        preview after one of its standard commands is called.  This way we
        avoid the problem where the key remap mechanism does not actually
        remap all keys associated with a given command.[1][2]

        [1] Thanks to Peter Prevos for reporting this in issue 1 on the GitHub
            mirror: <https://github.com/protesilaos/dired-preview/issues/1>.

        [2] Thanks to Christian Tietze and Ed Hamilton for discussing this
            with me on the mailing list:
            <https://lists.sr.ht/~protesilaos/general-issues/%3Cm1zg4noej2.fsf%40christiantietze.de%3E>.
            Commit ae93720 by Christian Tietze is based on this discussion.

     dired-preview.el | 50 +++++++++++++-------------------------------------
     1 file changed, 13 insertions(+), 37 deletions(-)

Please give it a try.

We may still need to tweak how this behaves, based on what Ed reported
in this thread.

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<878rc5xnpv.fsf@bookworm.mail-host-address-is-not-set>
In-Reply-To
<87zg4lk1i3.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
With the patch, dired-preview works with both arrow keys. Also with prefixes like C-u 11 n, or C-11 p, or c-11 up or down arrow key, etc.

It still does not find the first file in a directory if you land in that directory in the way I described earlier. It leaves the most recently displayed file still displayed. If you move down a file or more and then back up, it will display the first file. 

Also, I just noticed that if
- you have dired-preview-mode active but are not in dired, and 
- you are editing a file and
- then do C-x d to open dired in the file's directory, and
- you land on a file, then dired-preview does not display the file. 

Please note that I am mentioning these things because I feel you are interested in getting the package to work well. For myself, I could easily work around the small problem of not finding the first file in a directory.  

-- Ed H

Protesilaos Stavrou <info@protesilaos.com> writes:

>> From: Christian Tietze <me@christiantietze.de>
>> Date: Mon, 26 Jun 2023 12:24:42 +0200
>>
>>> This would be the ideal, but dired.el doesn't notify "interested parties"
>>> when the selected line changes. There's no hook, for some reason. This
>>> could be a useful addition upstream in the Emacs source code, but at the
>>> moment, we're stuck with key rebindings.
>>
>> One more thing:
>>
>> [... 13 lines elided]
>>
>> Maybe key rebinding *is* the best way at the moment?
>
> Thank you, Christian!  I installed your patch.  I think the advice
> mechanism is better suited for this purpose.  With the ":after", we are
> using it in practically the same way as a hook.  I thus generalised what
> you did in the following commit:
>
>     commit 3304286c175387602032a15a7d1888473ca054b0
>     Author: Protesilaos Stavrou <info@protesilaos.com>
>     Date:   Tue Jun 27 06:42:04 2023 +0300
>
>         Use an advice instead of defining new commands; delete our keymap
>
>         This is the same ideas as in commit ae93720, except more generalised.
>         Instead of defining our own commands, we advise Dired to display a
>         preview after one of its standard commands is called.  This way we
>         avoid the problem where the key remap mechanism does not actually
>         remap all keys associated with a given command.[1][2]
>
>         [1] Thanks to Peter Prevos for reporting this in issue 1 on the GitHub
>             mirror: <https://github.com/protesilaos/dired-preview/issues/1>.
>
>         [2] Thanks to Christian Tietze and Ed Hamilton for discussing this
>             with me on the mailing list:
>             <https://lists.sr.ht/~protesilaos/general-issues/%3Cm1zg4noej2.fsf%40christiantietze.de%3E>.
>             Commit ae93720 by Christian Tietze is based on this discussion.
>
>      dired-preview.el | 50 +++++++++++++-------------------------------------
>      1 file changed, 13 insertions(+), 37 deletions(-)
>
> Please give it a try.
>
> We may still need to tweak how this behaves, based on what Ed reported
> in this thread.


-- 
Ed Hamilton
Details
Message ID
<87jzvphr0z.fsf@protesilaos.com>
In-Reply-To
<878rc5xnpv.fsf@bookworm.mail-host-address-is-not-set> (view parent)
DKIM signature
missing
Download raw message
> From: Ed Hamilton <edhamilton9@gmail.com>
> Date: Tue, 27 Jun 2023 05:06:08 -0400
>
> With the patch, dired-preview works with both arrow keys. Also with
> prefixes like C-u 11 n, or C-11 p, or c-11 up or down arrow key, etc.

Good to know.  Thank you!

> It still does not find the first file in a directory if you land in
> that directory in the way I described earlier. It leaves the most
> recently displayed file still displayed. If you move down a file or
> more and then back up, it will display the first file.

Indeed.  I have noticed those issues.  I am not sure how to handle the
"landing" phase because there is no hook that runs when a new window is
focused.  It must be doable, but will need more work.

> Also, I just noticed that if
> - you have dired-preview-mode active but are not in dired, and 
> - you are editing a file and
> - then do C-x d to open dired in the file's directory, and
> - you land on a file, then dired-preview does not display the file. 

I tried this right now but could not reproduce it.  Maybe I fixed it
by accident in one of the subsequent commits I made?

> Please note that I am mentioning these things because I feel you are
> interested in getting the package to work well. For myself, I could
> easily work around the small problem of not finding the first file in
> a directory.

You are doing well and I appreciate your feedback.  Yes, I want to make
this work as reliably and intuitively as possible.  Thank you!

-- 
Protesilaos Stavrou
https://protesilaos.com
Reply to thread Export thread (mbox)