Repository navigation
Fix: Scroll up onto a Ghostel image preview from the rows below it - #527
Conversation
A window scrolled up to the row below a local image preview could not go further. Emacs cannot always measure backwards across a preview: from that row `pixel-scroll-precision-scroll-up' signals `beginning-of-buffer', and both `pixel-scroll-precision' and `ultra-scroll' leave the window where it is. Line scrolling logged the error on every key press, the wheel did nothing, and everything above the preview stayed out of reach. With a preview wider than the window the measurement works, but the row alignment then reset the vscroll and the whole image passed in one step. Step onto the preview by hand. When an upward scroll would cross a preview's lower edge, start the window on the preview's link row and hide all but the scrolled pixels with the vscroll. That is how `pixel-scroll-precision' and `ultra-scroll' themselves leave a window inside a tall line, so both carry on from there. Line scrolling, the plain wheel and the precision wheel all take this path, and the row alignment leaves a link row alone while it hides part of its image. A window that starts right below a preview draws the preview's trailing newline as an empty row; count it as the preview's last line so the text does not jump by a row on the way in. Scrolling up pushes the last rows out of the window. Move point onto a row still shown whole, or redisplay scrolls back down to show point and undoes the scroll. Record the scroll once the preview is in view too, so that Ghostel does not anchor the window back to live output.
| `pixel-scroll-precision' can fail to cross a preview's lower edge | ||
| upwards, and then leave the window on the row below the preview." | ||
| (let* ((delta-pair (and (consp event) (nth 4 event))) | ||
| (delta (and (consp delta-pair) (cdr delta-pair)))) |
There was a problem hiding this comment.
Thanks for the fix! On emacs-mac, trackpad pixel deltas are in the event plist at (nth 3 event) as :scrolling-delta-y, rather than (nth 4 event). Could you handle that format too?
| (delta (and (consp delta-pair) (cdr delta-pair)))) | |
| (let* ((delta-pair (and (consp event) (nth 4 event))) | |
| (delta (or (and (consp delta-pair) (cdr delta-pair)) | |
| (and (listp (nth 3 event)) | |
| (plist-get (nth 3 event) :scrolling-delta-y))))) |
There was a problem hiding this comment.
Done in 4a220ee. I did not take the suggestion as is: (nth 3 event) is read outside the consp guard and signals for a non-list event, which two existing tests pass. Both reads are guarded now.
Two things this does not cover. A wheeled mouse on emacs-mac reports :delta-y in fractional lines and no :scrolling-delta-y, so it still goes to ultra-scroll-mac. And --clamp-up-event reads only (nth 4 event) too, from before this PR, so it does nothing on emacs-mac.
I have no emacs-mac here. The event shape comes from ultra-scroll-mac's source, so could you check it on your trackpad?
emacs-mac wheel events carry no pixel delta pair. They report trackpad pixels as `:scrolling-delta-y' in a property list that takes the place of the line count. The upward step onto a preview read only the pair, so on emacs-mac it never ran and the event went to `ultra-scroll-mac', which cannot cross the preview's lower edge. Read the property list when the pair is missing. Both reads stay behind the list check, as the command is also called with events that are not lists.
|
Two emacs-mac gaps are left after 4a220ee. I cannot test either one here, so I would like to know what you want for each.
For each: (a) fix it in this PR, (b) a separate PR after this one, or (c) leave it. Both are read from the |
|
Thanks a lot for the quick follow-up in 4a220ee and for the careful write-up of the two remaining emacs-mac gaps. Reading the ultra-scroll-mac source to trace them was really helpful. On my side, I ran the full test_ai-code-ghostel-image-preview.el suite on this branch with Emacs 31.1 (NS build), and all 36 tests pass. I don't have emacs-mac set up here (it is , so I couldn't check the trackpad and wheeled-mouse cases on a real device. For both gaps, I'd go with (b/c): leave them out of this PR and come back to them later. This PR already fixes the main issue, so I'm happy to keep its scope as it is. Thanks again for the thorough work and testing on this one! |
2 participants
In Ghostel sessions, a window scrolled up to the row below an image preview couldn't go further, neither by line scroll nor by wheel.
Emacs can't always measure backwards across the preview's overlay, so
pixel-scroll-precision-scroll-upsignalsbeginning-of-bufferand the window stays where it is. Now when an upward scroll would cross a preview's lower edge, the window is put on the preview's link row with the vscroll hiding all but the scrolled pixels. That's the statepixel-scroll-precisionandultra-scrollleave a window in themselves, so they carry on from there.Two leftovers: a line scroll landing exactly on the row below a preview moves 36 px in that step, and leaving a preview at its top moves up to 6 px more than asked. Also the wheel events in the tests were synthetic, not from a real device.