Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

[Chong Yidong]

> Open a normal text file with the text mode, do
> (view-mode)
> (text-scale-increase) or (text-scale-decrease)
> (View-scroll-page-forward) or (View-scroll-page-backward)
> The View-scroll-page-* command does wrong.  It cannot scroll "page
> size" as expected like it does when the text-scale minor mode is off.

View-mode, written in the dawn of time, assumes that all lines have the
default height.  The following patch changes it so that
scroll-up/scroll-down are passed nil arguments where possible; then
Emacs will automatically determine how to scroll by one page, taking
variable-height lines and text-scaling into account.

It's not good to make this kind of change at this stage in the release,
but this bug would be pretty annoying if you happen to come across it,
and that's much more likely now we've introduced the text-scaling
commands.  Could someone on emacs-devel help review the patch?


*** trunk/lisp/view.el.~1.102.~	2009-05-26 10:34:04.000000000 -0400
--- trunk/lisp/view.el	2009-05-26 10:39:03.000000000 -0400
***************
*** 740,747 ****
  ;;; Some help routines.
  
  (defun view-window-size ()
!   ;; Window height excluding mode line.
!   (1- (window-height)))
  
  ;; (defun view-last-command (&optional who what)
  ;;  (setq view-last-command-entry this-command)
--- 740,754 ----
  ;;; Some help routines.
  
  (defun view-window-size ()
!   ;; Return the number of lines in the current window, excluding the
!   ;; mode line.  Using `window-line-height' accounts for
!   ;; variable-height fonts.
!   (let ((h (window-line-height -1)))
!     (if h
! 	(1+ (nth 1 h))
!       ;; This should not happen, but if `window-line-height' returns
!       ;; nil, fall back on `window-height'.
!       (1- (window-height)))))
  
  ;; (defun view-last-command (&optional who what)
  ;;  (setq view-last-command-entry this-command)
***************
*** 761,771 ****
    (recenter '(1)))
  
  (defun view-page-size-default (lines)
!   ;; Get page size.
!   (let ((default (- (view-window-size) next-screen-context-lines)))
!     (if (or (null lines) (zerop (setq lines (prefix-numeric-value lines))))
! 	default
!       (min (abs lines) default))))
  
  (defun view-set-half-page-size-default (lines)
    ;; Get and maybe set half page size.
--- 768,780 ----
    (recenter '(1)))
  
  (defun view-page-size-default (lines)
!   ;; Return nil if LINES is nil, 0, or larger than
!   ;; `view-window-size'. Otherwise, return LINES.
!   (and lines
!        (not (zerop (setq lines (prefix-numeric-value lines))))
!        (<= (abs lines)
! 	   (abs (- (view-window-size) next-screen-context-lines)))
!        lines))
  
  (defun view-set-half-page-size-default (lines)
    ;; Get and maybe set half page size.
***************
*** 827,854 ****
    ;; This function does the job for all the scrolling commands.
    ;; Scroll forward LINES lines.  If BACKWARD is true scroll backwards.
    ;; If LINES is negative scroll in the other direction.  If LINES is 0 or nil,
!   ;; scroll DEFAULT lines.  If MAXDEFAULT is true then scroll no more than a
!   ;; window full.
    (if (or (null lines) (zerop (setq lines (prefix-numeric-value lines))))
        (setq lines default))
!   (when (< lines 0)
!     (setq backward (not backward)) (setq lines (- lines)))
!   (setq default (view-page-size-default nil)) ; Max scrolled at a time.
!   (if maxdefault (setq lines (min lines default)))
!   (cond
!    (backward (scroll-down lines))
!    ((view-really-at-end)
!     (if view-scroll-auto-exit (View-quit)
!       (ding)
!       (view-end-message)))
!    (t (while (> lines default)
! 	(scroll-up default)
! 	(setq lines (- lines default))
! 	(if (view-really-at-end) (setq lines 0)))
!       (scroll-up lines)
!       (if (view-really-at-end) (view-end-message))
!       (move-to-window-line -1)
!       (beginning-of-line))))
  
  (defun view-really-at-end ()
    ;; Return true if buffer end visible.  Maybe revert buffer and test.
--- 836,857 ----
    ;; This function does the job for all the scrolling commands.
    ;; Scroll forward LINES lines.  If BACKWARD is true scroll backwards.
    ;; If LINES is negative scroll in the other direction.  If LINES is 0 or nil,
!   ;; scroll DEFAULT lines (if DEFAULT is nil, scroll by one page).  If
!   ;; MAXDEFAULT is true then scroll no more than a window full.
    (if (or (null lines) (zerop (setq lines (prefix-numeric-value lines))))
        (setq lines default))
!   (when (and lines (< lines 0))
!     (setq backward (not backward) lines (- lines)))
!   (when (and maxdefault lines (> lines (view-window-size)))
!     (setq lines nil))
!   (cond (backward (scroll-down lines))
! 	((view-really-at-end)
! 	 (if view-scroll-auto-exit
! 	     (View-quit)
! 	   (ding)
! 	   (view-end-message)))
! 	(t (scroll-up lines)
! 	   (if (view-really-at-end) (view-end-message)))))
  
  (defun view-really-at-end ()
    ;; Return true if buffer end visible.  Maybe revert buffer and test.

Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

[Stefan Monnier]

> It's not good to make this kind of change at this stage in the release,
> but this bug would be pretty annoying if you happen to come across it,
> and that's much more likely now we've introduced the text-scaling
> commands.  Could someone on emacs-devel help review the patch?

It looks fine to me.
Tho, you could change the doccomments to docstrings while you're there.


        Stefan

Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

[Juri Linkov]

> View-mode, written in the dawn of time, assumes that all lines have the
> default height.  The following patch changes it so that
> scroll-up/scroll-down are passed nil arguments where possible; then
> Emacs will automatically determine how to scroll by one page, taking
> variable-height lines and text-scaling into account.
>
> It's not good to make this kind of change at this stage in the release,
> but this bug would be pretty annoying if you happen to come across it,
> and that's much more likely now we've introduced the text-scaling
> commands.  Could someone on emacs-devel help review the patch?

I noticed that this patch causes a regression: typing RET used to put
point to the last line at the end of the window, but now typing RET keeps
point on the same line.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

[Chong Yidong]

Juri Linkov <juri@jurta.org> writes:

>> View-mode, written in the dawn of time, assumes that all lines have the
>> default height.  The following patch changes it so that
>> scroll-up/scroll-down are passed nil arguments where possible; then
>> Emacs will automatically determine how to scroll by one page, taking
>> variable-height lines and text-scaling into account.
>>
>> It's not good to make this kind of change at this stage in the release,
>> but this bug would be pretty annoying if you happen to come across it,
>> and that's much more likely now we've introduced the text-scaling
>> commands.  Could someone on emacs-devel help review the patch?
>
> I noticed that this patch causes a regression: typing RET used to put
> point to the last line at the end of the window, but now typing RET keeps
> point on the same line.

Thanks.  This regression isn't serious enough for a fix in the branch, I
think (the docstring of View-scroll-line-forward does not say that point
should go to the last line).  But I'll see what I can do for the trunk.

Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

[Juri Linkov]

>> I noticed that this patch causes a regression: typing RET used to put
>> point to the last line at the end of the window, but now typing RET keeps
>> point on the same line.
>
> Thanks.  This regression isn't serious enough for a fix in the branch, I
> think (the docstring of View-scroll-line-forward does not say that point
> should go to the last line).

Even though it is not serious, it is very noticeable for the users
of view.el, and this change in behavior is annoying.  Fixing it is just
a matter of restoring two lines:

  (move-to-window-line -1)
  (beginning-of-line)

> But I'll see what I can do for the trunk.

Thanks in advance.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

[Miles Bader]

Chong Yidong <cyd@stupidchicken.com> writes:
> View-mode, written in the dawn of time, assumes that all lines have the
> default height.

Hmm, this makes me think it's time to think again about a simpler and
cleaner replacement for (the execrable) view-mode, as discussed a while
ago...

-Miles

-- 
Friendship, n. A ship big enough to carry two in fair weather, but only one
in foul.

Re: 23.0.92; View-scroll-page-forward/backward does wrong in text mode after text-scale-increase/decrease

[Juri Linkov]

>> View-mode, written in the dawn of time, assumes that all lines have the
>> default height.
>
> Hmm, this makes me think it's time to think again about a simpler and
> cleaner replacement for (the execrable) view-mode, as discussed a while
> ago...

Yes, please.  View-mode is a mess.

-- 
Juri Linkov
http://www.jurta.org/emacs/