Re: scroll-down with pixel transition

[Eli Zaretskii]

> Date: Sat, 15 Apr 2017 07:32:52 +0900 (JST)
> Cc: address@hidden, address@hidden, address@hidden
> From: Tak Kunihiro <address@hidden>
> 
> > I think we should have a minor mode that scrolls "smoothly" as in
> > your code.  Would you like to code that for inclusion in Emacs?
> 
> Yes, please make Emacs smoothly.
> 
> I have revised code to include your algorithm for scroll-down.  I
> attach it.

Thanks.  I'll let people try this and comment.  In the meantime, a few
comments from myself:

> The top-level functions need to be more robust.  For example, I see
> glitches during scroll-down on Info.

Probably due to Info using fonts of different sizes.

> ;;; This file is NOT part of GNU Emacs

Should be changed once this is in core.

> (defgroup pixel-scroll nil
>   "Scroll pixel-by-pixel in Emacs."
>   :group 'mouse
>   :prefix "pixel-")

I see no need for a new group, please use the existing group
'scrolling'.

> (defcustom pixel-wait 0.001
>   "Idle time on pixel scroll specified in second."
>   :group 'pixel-scroll
>   :type 'float)

The doc string should say a bit more to explain the user-visible
effect of the value.

Also, please add a ':version' tag to each defcustom.

> (defcustom pixel-amount '(1 ((shift) . 5) ((control)))
>   "Amount to scroll by when spinning the mouse wheel."
>   :group 'pixel-scroll)

Do we really need a separate defcustom for this?  Why not use
mouse-wheel-scroll-amount?

> (defcustom pixel-resolution-fine-p nil
>   "Enhance scrolling resolution to pixel-to-pixel instead of
> line-to-line."
>   :group 'pixel-scroll
>   :type 'boolean)

Not sure this should be advertised, as the functionality has issues.
At the very least those issues should be described in the doc string.

>         (vertical-motion -2))) ; FIXME: -1 gives glitch

What glitch is that?

> (defun pixel-point-at-top-p ()
>   "Return if point is at top of a window."
>   (equal (save-excursion (beginning-of-visual-line)
>                          (point-at-bol))
>          (window-start)))

I think this will fail with bidirectional text, if the first character
in the logical order is not displayed at the leftmost screen position
of the first line of the window.

I suggest to use posn-at-point instead.

> (defun pixel-point-at-bottom-p ()
>   "Return if point is at bottom of a window."
>   (<= (count-lines (save-excursion
>                      (beginning-of-visual-line)
>                      (point-at-bol))
>                    (pixel-window-end)) 1))

Likewise here, I suggest to use window-end and posn-at-point.

> (defun pixel--catch-line-up ()
>   "Flush text upward a line with pixel transition.  When `vscroll' is 
> non-zero,
> complete scrolling a line.  When `vscroll' is larger than height
> of multiple lines, for example 88, this flushes multiple lines.
> At the end, `vscroll' will be zero.  This assumes that the lines
> are with the same height.  Scope moves downward.  This function
> returns number of pixels that were scrolled."

I'm not sure what you mean by "flush" here.  And why the function's
name uses 'catch' if you mean 'flush'?

> (defun pixel--sweep-pixel-up (n)
>   "Sweep text upward to N pixels.  Scope moves downward."

Maybe I'm misunderstanding what this does, but "sweep" doesn't seem to
describe it.

> (defun pixel-window-end ()
>   "Return position of the last character of fully-visible line in
> WINDOW.  This is similar to `window-end' but see a full visible
> line."
>   (let ((pos (window-end)))
>     (if (pos-visible-in-window-p pos nil t)
>         pos
>       (save-excursion
>         (goto-char pos)
>         (vertical-motion -2)
>         (point-at-bol)))))

I don't understand the 'else' part here.  What situation does it
handle?  AFAIU, if pos-visible-in-window-p returns nil in this case,
the window-end position is completely invisible, which is something
that cannot happen, I think.

> (defun pixel-line-height (&optional pos)
>   "Measure line height of POS in pixel.  When height of all lines
> are equal, you don't need this function but `frame-char-height'.
> See Info node `(elisp) Line Height'."
>   (or pos (setq pos (window-start)))
            ^^^^^^^^^^^^^^^^^^^^^^^^
This part is not mentioned in the doc string, so the situation where
POS is unspecified is left undefined.

> (defun pixel-scroll-down-and-set-window-vscroll (vscroll)
>   "Scroll down a line and set VSCROLL in pixel.  This is written
> by Eli Zaretskii.  It is important to call `set-window-start' to
> force the display engine use that particular position as the
> window-start point.  Otherwise, redisplay will reset the window's
> vscroll."

We don't put such information in the doc strings, certainly not
authorship.  Comments are more appropriate, and you could simply cite
the emacs-devel message by its URL.