bug#68215: Bug in what-page

bug#68215: Bug in what-page

[Lars Brinkhoff]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

Hello,

The Emacs manual says:
  "M-x what-page
   Display the page number of point, and the line number within that page."

I would argue that this is the intended function, since that is what
GNU Emacs did before 2010, and it is the same as "What Page" in the
original TECO Emacs.

Bug https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6825 noted that the
line number was wrong if the cursor was not at the beginning of a
line.  There was a fix for this, but the fix only works for the first
page.  In subsequent pages, the line number is no longer relative to
the current page.

I'm attaching a patch which restores the original function and also
fixes the bug.  The updated test checks this.  I also updated the
what-page doc string to more clearly explain what line numbers are
reported.

[-- Attachment #2: what-line.patch --]
[-- Type: text/x-patch, Size: 2028 bytes --]

commit e678a23ee1b0864f76c2d91d4f354866dcd1199f
Author: Lars Brinkhoff <lars@nocrew.org>
Date:   Tue Jan 2 09:06:13 2024 +0100

    Fix what-page.

diff --git a/lisp/textmodes/page.el b/lisp/textmodes/page.el
index e8621ee0383..afbc614770e 100644
--- a/lisp/textmodes/page.el
+++ b/lisp/textmodes/page.el
@@ -159,21 +159,25 @@ count-lines-page
              total before after)))
 
 (defun page--what-page ()
-  "Return a list of the page and line number of point."
+  "Return a list of the page number of point, and the line number
+within that page."
   (save-restriction
     (widen)
     (save-excursion
       (let ((count 1)
-            (opoint (point)))
+            (opoint (point))
+            (oline (line-number-at-pos (point))))
         (goto-char (point-min))
         (while (re-search-forward page-delimiter opoint t)
           (when (= (match-beginning 0) (match-end 0))
             (forward-char))
           (setq count (1+ count)))
-        (list count (line-number-at-pos opoint))))))
+        (list count
+              (1+ (- oline (line-number-at-pos (point)))))))))
 
 (defun what-page ()
-  "Print page and line number of point."
+  "Display the page number of point, and the line number within
+that page."
   (interactive)
   (apply #'message (cons "Page %d, line %d" (page--what-page))))
 
diff --git a/test/lisp/textmodes/page-tests.el b/test/lisp/textmodes/page-tests.el
index f3a2c5fbe00..617b59a54fb 100644
--- a/test/lisp/textmodes/page-tests.el
+++ b/test/lisp/textmodes/page-tests.el
@@ -106,10 +106,14 @@ page-tests-what-page
     (insert "foo\n\f\nbar\n\f\nbaz")
     (goto-char (point-min))
     (should (equal (page--what-page) '(1 1)))
+    (forward-char)
+    (should (equal (page--what-page) '(1 1)))
     (forward-page)
+    (should (equal (page--what-page) '(2 1)))
+    (next-line)
     (should (equal (page--what-page) '(2 2)))
     (forward-page)
-    (should (equal (page--what-page) '(3 4)))))
+    (should (equal (page--what-page) '(3 1)))))
 
 
 ;;; page-tests.el ends here

bug#68215: Bug in what-page

[Eli Zaretskii]

> From: Lars Brinkhoff <lars.brinkhoff@gmail.com>
> Date: Tue, 2 Jan 2024 11:39:22 +0100
> 
> The Emacs manual says:
>   "M-x what-page
>    Display the page number of point, and the line number within that page."
> 
> I would argue that this is the intended function, since that is what
> GNU Emacs did before 2010, and it is the same as "What Page" in the
> original TECO Emacs.
> 
> Bug https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6825 noted that the
> line number was wrong if the cursor was not at the beginning of a
> line.  There was a fix for this, but the fix only works for the first
> page.  In subsequent pages, the line number is no longer relative to
> the current page.

Right, thanks for finding this bug.

> I'm attaching a patch which restores the original function and also
> fixes the bug.  The updated test checks this.  I also updated the
> what-page doc string to more clearly explain what line numbers are
> reported.

Using line-number-at-pos in the fix for bug#6825 had one more adverse
consequence: the line counts returned by what-page no longer honor
invisible lines and selective-display, which count-lines did.  So I'd
prefer to fix both bugs at once, by going back to count-lines, but in
a way that would avoid the off-by-one error fixed in bug#6825.  AFAIU,
the off-by-one error is due to the fact that count-lines treats buffer
position at BOL specially, e.g.:

	    (t
             (goto-char (point-max))
             (if (bolp)
                 (1- (line-number-at-pos))
               (line-number-at-pos)))))))

It should be a simple matter to countermand this special-casing in
what-page, while still using count-lines.  Or am I missing something?

>  (defun page--what-page ()
> -  "Return a list of the page and line number of point."
> +  "Return a list of the page number of point, and the line number
> +within that page."

Our convention is to have the first line of a doc string be a single
complete sentence.

>  (defun what-page ()
> -  "Print page and line number of point."
> +  "Display the page number of point, and the line number within
> +that page."

Likewise here.

Thanks.

bug#68215: Bug in what-page

[Lars Brinkhoff]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

Eli Zaretskii <eliz@gnu.org> wrote:
> It should be a simple matter to countermand this special-casing in
> what-page, while still using count-lines.  Or am I missing something?

That's probably better.  I'm attaching an updated patch which uses
count-lines instead.  It countermands the special case by checking
(bolp) and then doing the opposite adjustment.

However, there's one more case to consider: when point starts out
right after a page delimiter (i.e. the very first positon on a page),
the page--what-page while loop will leave the final point there too,
and then count-lines will return 0.  The line number should be 1, so
an adjustment is needed there too.

The updated test is the same as the previous round, and they still
pass with the new patch.

> > +  "Return a list of the page number of point, and the line number
> > +within that page."
>
> Our convention is to have the first line of a doc string be a single
> complete sentence.

Fixed by moving info to a second line, or rephrasing to one line.

[-- Attachment #2: what-line-2.patch --]
[-- Type: text/x-patch, Size: 2016 bytes --]

commit 746013e179735673b2ad535ea46d54bae3f38d50
Author: Lars Brinkhoff <lars@nocrew.org>
Date:   Tue Jan 2 09:06:13 2024 +0100

    Fix what-page.

diff --git a/lisp/textmodes/page.el b/lisp/textmodes/page.el
index e8621ee0383..1c7561d71c6 100644
--- a/lisp/textmodes/page.el
+++ b/lisp/textmodes/page.el
@@ -159,21 +159,23 @@ count-lines-page
              total before after)))
 
 (defun page--what-page ()
-  "Return a list of the page and line number of point."
+  "Return a list of the page and line number of point.
+The line number is relative to the start of the page."
   (save-restriction
     (widen)
     (save-excursion
       (let ((count 1)
+            (adjust (if (or (bolp) (looking-back page-delimiter)) 1 0))
             (opoint (point)))
         (goto-char (point-min))
         (while (re-search-forward page-delimiter opoint t)
           (when (= (match-beginning 0) (match-end 0))
             (forward-char))
           (setq count (1+ count)))
-        (list count (line-number-at-pos opoint))))))
+        (list count (+ adjust (count-lines (point) opoint)))))))
 
 (defun what-page ()
-  "Print page and line number of point."
+  "Display the page number, and the line number within that page."
   (interactive)
   (apply #'message (cons "Page %d, line %d" (page--what-page))))
 
diff --git a/test/lisp/textmodes/page-tests.el b/test/lisp/textmodes/page-tests.el
index f3a2c5fbe00..617b59a54fb 100644
--- a/test/lisp/textmodes/page-tests.el
+++ b/test/lisp/textmodes/page-tests.el
@@ -106,10 +106,14 @@ page-tests-what-page
     (insert "foo\n\f\nbar\n\f\nbaz")
     (goto-char (point-min))
     (should (equal (page--what-page) '(1 1)))
+    (forward-char)
+    (should (equal (page--what-page) '(1 1)))
     (forward-page)
+    (should (equal (page--what-page) '(2 1)))
+    (next-line)
     (should (equal (page--what-page) '(2 2)))
     (forward-page)
-    (should (equal (page--what-page) '(3 4)))))
+    (should (equal (page--what-page) '(3 1)))))
 
 
 ;;; page-tests.el ends here

bug#68215: Bug in what-page

[Eli Zaretskii]

> From: Lars Brinkhoff <lars.brinkhoff@gmail.com>
> Date: Thu, 11 Jan 2024 14:49:58 +0100
> Cc: 68215@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> wrote:
> > It should be a simple matter to countermand this special-casing in
> > what-page, while still using count-lines.  Or am I missing something?
> 
> That's probably better.  I'm attaching an updated patch which uses
> count-lines instead.  It countermands the special case by checking
> (bolp) and then doing the opposite adjustment.
> 
> However, there's one more case to consider: when point starts out
> right after a page delimiter (i.e. the very first positon on a page),
> the page--what-page while loop will leave the final point there too,
> and then count-lines will return 0.  The line number should be 1, so
> an adjustment is needed there too.
> 
> The updated test is the same as the previous round, and they still
> pass with the new patch.
> 
> > > +  "Return a list of the page number of point, and the line number
> > > +within that page."
> >
> > Our convention is to have the first line of a doc string be a single
> > complete sentence.
> 
> Fixed by moving info to a second line, or rephrasing to one line.

Thanks, installed on the master branch, and closing the bug.