unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#20663: page.el (forward-page): Avoid skipping pages
@ 2015-05-26 17:14 Pierre Neidhardt
  2016-04-09 10:13 ` Marcin Borkowski
  2022-04-22 12:05 ` Lars Ingebrigtsen
  0 siblings, 2 replies; 21+ messages in thread
From: Pierre Neidhardt @ 2015-05-26 17:14 UTC (permalink / raw)
  To: 20663

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

When `page-delimiter` starts at the beginning of the line and the position is
also at the beginning of the line, calling `forward-page` will skip one page.

Running `emacs -Q example.txt`:

	M-<
	C-x n p
	M->
	M-1 C-x n p

This should bring us from page 1 to page 2, but page 3 gets displayed instead.

The attached patch fixes it by changing the code to actually match its
surrounding comments.


In GNU Emacs 24.5.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.16.2)
 of 2015-04-20 on bitzer.hoetzel.info
Windowing system distributor `The X.Org Foundation', version 11.0.11701000
System Description:	Arch Linux

Configured using:
 `configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong
 --param=ssp-buffer-size=4' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro'

Important settings:
  value of $LC_MESSAGES: sv_SE.UTF-8
  value of $LANG: sv_SE.UTF-8
  locale-coding-system: utf-8-unix

-- 
Pierre Neidhardt

Virginia law forbids bathtubs in the house; tubs must be kept in the yard.

[-- Attachment #2: ChangeLog --]
[-- Type: text/plain, Size: 48 bytes --]

	* page.el (forward-page): Avoid skipping pages

[-- Attachment #3: example.txt --]
[-- Type: text/plain, Size: 25 bytes --]

Page 1
\f
Page 2
\f
Page 3

[-- Attachment #4: page.patch --]
[-- Type: text/x-diff, Size: 551 bytes --]

diff --git a/lisp/textmodes/page.el b/lisp/textmodes/page.el
index 39db5bb..c875a49 100644
--- a/lisp/textmodes/page.el
+++ b/lisp/textmodes/page.el
@@ -37,7 +37,7 @@ A page boundary is any line whose beginning matches the regexp
   (while (and (> count 0) (not (eobp)))
     ;; In case the page-delimiter matches the null string,
     ;; don't find a match without moving.
-    (if (bolp) (forward-char 1))
+    (if (string= page-delimiter "") (forward-char 1))
     (if (re-search-forward page-delimiter nil t)
 	nil
       (goto-char (point-max)))

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2015-05-26 17:14 bug#20663: page.el (forward-page): Avoid skipping pages Pierre Neidhardt
@ 2016-04-09 10:13 ` Marcin Borkowski
  2016-04-09 12:00   ` Eli Zaretskii
  2022-04-22 12:05 ` Lars Ingebrigtsen
  1 sibling, 1 reply; 21+ messages in thread
From: Marcin Borkowski @ 2016-04-09 10:13 UTC (permalink / raw)
  To: Pierre Neidhardt; +Cc: 20663

On 2015-05-26, at 19:14, Pierre Neidhardt <ambrevar@gmail.com> wrote:

> When `page-delimiter` starts at the beginning of the line and the position is
> also at the beginning of the line, calling `forward-page` will skip one page.
>
> Running `emacs -Q example.txt`:
>
> 	M-<
> 	C-x n p
> 	M->
> 	M-1 C-x n p
>
> This should bring us from page 1 to page 2, but page 3 gets displayed instead.
>
> The attached patch fixes it by changing the code to actually match its
> surrounding comments.

Hi Emacs devs,

could someone take a look at the proposed patch?  It is not installed as
of GNU Emacs 25.1.50.8 (commit 1e8cd05), and I don't feel competent
enough to be sure it does not have any adverse side effects.

Best,

-- 
Marcin





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-09 10:13 ` Marcin Borkowski
@ 2016-04-09 12:00   ` Eli Zaretskii
  2016-04-09 18:16     ` Marcin Borkowski
  0 siblings, 1 reply; 21+ messages in thread
From: Eli Zaretskii @ 2016-04-09 12:00 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663, ambrevar

> From: Marcin Borkowski <mbork@mbork.pl>
> Date: Sat, 09 Apr 2016 12:13:11 +0200
> Cc: 20663@debbugs.gnu.org
> 
> On 2015-05-26, at 19:14, Pierre Neidhardt <ambrevar@gmail.com> wrote:
> 
> > When `page-delimiter` starts at the beginning of the line and the position is
> > also at the beginning of the line, calling `forward-page` will skip one page.
> >
> > Running `emacs -Q example.txt`:
> >
> > 	M-<
> > 	C-x n p
> > 	M->
> > 	M-1 C-x n p
> >
> > This should bring us from page 1 to page 2, but page 3 gets displayed instead.
> >
> > The attached patch fixes it by changing the code to actually match its
> > surrounding comments.
> 
> Hi Emacs devs,
> 
> could someone take a look at the proposed patch?  It is not installed as
> of GNU Emacs 25.1.50.8 (commit 1e8cd05), and I don't feel competent
> enough to be sure it does not have any adverse side effects.

Maybe I'm missing something, but I don't see the connection between
the description of the bug, the recipe, and the patch.

The description talks about calling forward-page, but the recipe
doesn't call it.  The patch compares page-delimiter with an empty
string, but the default value of page-delimiter is not empty.

So I'm mightily confused by this.  Hopefully, someone will show me
what I'm missing.





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-09 12:00   ` Eli Zaretskii
@ 2016-04-09 18:16     ` Marcin Borkowski
  2016-04-09 19:34       ` Eli Zaretskii
  0 siblings, 1 reply; 21+ messages in thread
From: Marcin Borkowski @ 2016-04-09 18:16 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 20663, ambrevar


On 2016-04-09, at 12:00, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Marcin Borkowski <mbork@mbork.pl>
>> Date: Sat, 09 Apr 2016 12:13:11 +0200
>> Cc: 20663@debbugs.gnu.org
>> 
>> On 2015-05-26, at 19:14, Pierre Neidhardt <ambrevar@gmail.com> wrote:
>> 
>> > When `page-delimiter` starts at the beginning of the line and the position is
>> > also at the beginning of the line, calling `forward-page` will skip one page.
>> >
>> > Running `emacs -Q example.txt`:
>> >
>> > 	M-<
>> > 	C-x n p
>> > 	M->
>> > 	M-1 C-x n p
>> >
>> > This should bring us from page 1 to page 2, but page 3 gets displayed instead.
>> >
>> > The attached patch fixes it by changing the code to actually match its
>> > surrounding comments.
>> 
>> Hi Emacs devs,
>> 
>> could someone take a look at the proposed patch?  It is not installed as
>> of GNU Emacs 25.1.50.8 (commit 1e8cd05), and I don't feel competent
>> enough to be sure it does not have any adverse side effects.
>
> Maybe I'm missing something, but I don't see the connection between
> the description of the bug, the recipe, and the patch.
>
> The description talks about calling forward-page, but the recipe
> doesn't call it.  The patch compares page-delimiter with an empty
> string, but the default value of page-delimiter is not empty.
>
> So I'm mightily confused by this.  Hopefully, someone will show me
> what I'm missing.

Well, I quickly glanced over the patch and decided that even if it
worked for me, I wouldn't be sure whether it doesn't break something
else.  Since you claim it rather won't work, I'm now tempted to look at
this issue more closely.  Hopefully I'll be able to come up with
a better patch in a few days.

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-09 18:16     ` Marcin Borkowski
@ 2016-04-09 19:34       ` Eli Zaretskii
  2016-04-10  1:29         ` Pierre Neidhardt
  0 siblings, 1 reply; 21+ messages in thread
From: Eli Zaretskii @ 2016-04-09 19:34 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663, ambrevar

> From: Marcin Borkowski <mbork@mbork.pl>
> Cc: ambrevar@gmail.com, 20663@debbugs.gnu.org
> Date: Sat, 09 Apr 2016 20:16:47 +0200
> 
> Well, I quickly glanced over the patch and decided that even if it
> worked for me, I wouldn't be sure whether it doesn't break something
> else.  Since you claim it rather won't work

I don't claim it doesn't work, I just don't have a clear understanding
of the problem, and consequently don't understand the solution, and
cannot judge whether it's TRT.

> I'm now tempted to look at this issue more closely.  Hopefully I'll
> be able to come up with a better patch in a few days.

Thanks.





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-09 19:34       ` Eli Zaretskii
@ 2016-04-10  1:29         ` Pierre Neidhardt
  2016-04-11 10:20           ` Marcin Borkowski
  0 siblings, 1 reply; 21+ messages in thread
From: Pierre Neidhardt @ 2016-04-10  1:29 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 20663, Marcin Borkowski

I did not expect this patch to be so confusing, but let me clarify the above questions:

- `forward-page' is called by `narrow-to-page', which is bound to 'C-x n p' by
  default.

- All the patch does it make the code consistent with its comments, that is:

	;; In case the page-delimiter matches the null string,
	;; don't find a match without moving.

- If you try the recipe (I just did on Emacs 24.5.1, don't have time to check
now on upstream), you'll see that a page gets skipped, which is not the desired
behaviour.

- As for side effects, there might be some, althought I haven't noticed anything
in a year of use. If there is, then it is a bug in the caller.

-- 
Pierre Neidhardt





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-10  1:29         ` Pierre Neidhardt
@ 2016-04-11 10:20           ` Marcin Borkowski
  2016-04-11 15:35             ` Eli Zaretskii
  0 siblings, 1 reply; 21+ messages in thread
From: Marcin Borkowski @ 2016-04-11 10:20 UTC (permalink / raw)
  To: Pierre Neidhardt; +Cc: 20663

Hi,

I spent a few minutes on this with Edebug and Git today.

On 2016-04-10, at 08:29, Pierre Neidhardt <ambrevar@gmail.com> wrote:

> I did not expect this patch to be so confusing, but let me clarify the above questions:
>
> - `forward-page' is called by `narrow-to-page', which is bound to 'C-x n p' by
>   default.

True.

> - All the patch does it make the code consistent with its comments, that is:
>
> 	;; In case the page-delimiter matches the null string,
> 	;; don't find a match without moving.

Seems ok.

> - If you try the recipe (I just did on Emacs 24.5.1, don't have time to check
> now on upstream), you'll see that a page gets skipped, which is not the desired
> behaviour.

Condirmed.  And it doesn't happen with the patch installed.

> - As for side effects, there might be some, althought I haven't noticed anything
> in a year of use. If there is, then it is a bug in the caller.

That I still don't know.

I also checked when the discussed fragment was introduced.  It seems it
was commit 07f4ea7, and clearly the commiter did not adhere to the rules
concerning writing clear and informative commit messages. (SCNR;-P)
I still have a very vague idea about the patch.  I think it is crucial
to define clearly where the page boundary is.  For instance, when the
point is -!-, is the point on the first or second page here?  Emacs with
and without the patch has different opinions on that.

Page 1
-!-^L
Page 2
^L
Page 3

-- 
Marcin





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-11 10:20           ` Marcin Borkowski
@ 2016-04-11 15:35             ` Eli Zaretskii
  2016-04-13 17:53               ` Marcin Borkowski
  0 siblings, 1 reply; 21+ messages in thread
From: Eli Zaretskii @ 2016-04-11 15:35 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663, ambrevar

> From: Marcin Borkowski <mbork@mbork.pl>
> Cc: Eli Zaretskii <eliz@gnu.org>,  20663@debbugs.gnu.org
> Date: Mon, 11 Apr 2016 12:20:07 +0200
> 
> I spent a few minutes on this with Edebug and Git today.

Thanks.

> > - All the patch does it make the code consistent with its comments, that is:
> >
> > 	;; In case the page-delimiter matches the null string,
> > 	;; don't find a match without moving.
> 
> Seems ok.
> 
> > - If you try the recipe (I just did on Emacs 24.5.1, don't have time to check
> > now on upstream), you'll see that a page gets skipped, which is not the desired
> > behaviour.
> 
> Condirmed.  And it doesn't happen with the patch installed.

Can one of you please explain why the original code misbehaves?

> For instance, when the point is -!-, is the point on the first or
> second page here?  Emacs with and without the patch has different
> opinions on that.

Good point.





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-11 15:35             ` Eli Zaretskii
@ 2016-04-13 17:53               ` Marcin Borkowski
  2016-04-13 20:14                 ` John Mastro
                                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Marcin Borkowski @ 2016-04-13 17:53 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 20663, ambrevar


On 2016-04-11, at 15:35, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Marcin Borkowski <mbork@mbork.pl>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  20663@debbugs.gnu.org
>> Date: Mon, 11 Apr 2016 12:20:07 +0200
>> 
>> I spent a few minutes on this with Edebug and Git today.
>
> Thanks.
>
>> > - All the patch does it make the code consistent with its comments, that is:
>> >
>> > 	;; In case the page-delimiter matches the null string,
>> > 	;; don't find a match without moving.
>> 
>> Seems ok.
>> 
>> > - If you try the recipe (I just did on Emacs 24.5.1, don't have time to check
>> > now on upstream), you'll see that a page gets skipped, which is not the desired
>> > behaviour.
>> 
>> Condirmed.  And it doesn't happen with the patch installed.
>
> Can one of you please explain why the original code misbehaves?

Quoting from the original report:

--8<---------------cut here---------------start------------->8---
> Running `emacs -Q example.txt`:
>
> 	M-<
> 	C-x n p
> 	M->
> 	M-1 C-x n p
>
> This should bring us from page 1 to page 2, but page 3 gets displayed instead.
--8<---------------cut here---------------end--------------->8---

>> For instance, when the point is -!-, is the point on the first or
>> second page here?  Emacs with and without the patch has different
>> opinions on that.
>
> Good point.

Here's the relevant excerpt from the manual:

--8<---------------cut here---------------start------------->8---
   The variable ‘page-delimiter’ controls where pages begin.  Its value
is a regular expression that matches the beginning of a line that
separates pages (*note Regexps::).  The normal value of this variable is
‘"^\f"’, which matches a formfeed character at the beginning of a line.
--8<---------------cut here---------------end--------------->8---

Is it me or is the above ambiguous?


I'm a bit busy now, but I'll make a second attempt at this issue within
a few days.  One of the problems is that (as the above paragraph seems
to confirm) the very notion of a "page" in Emacs is vague.  IMHO we
should start with a clear definition of a "page".  It is well possible
that different functions in page.el use different interpretations of
this notion, and the bug is just a symptom if such a mess.

My proposal is that a "page separator" would be a position in the buffer
where (looking-at-p page-delimiter) is true, and if point is at such
a place, then we consider it on the next page.  I.e., in this situation

abcabcabc
-!-^L
cbacbacba

the point is already on the second page (unlike the default Emacs
behavior).

Then, someone should study page.el and where necessary, update it to the
precisely defined notion of a "page".  (The mythical "someone" might be
me.)

WDYT?

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-13 17:53               ` Marcin Borkowski
@ 2016-04-13 20:14                 ` John Mastro
  2016-04-13 20:54                   ` Marcin Borkowski
  2016-04-16 11:03                 ` Marcin Borkowski
  2016-04-16 11:26                 ` Eli Zaretskii
  2 siblings, 1 reply; 21+ messages in thread
From: John Mastro @ 2016-04-13 20:14 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663, ambrevar

Marcin Borkowski <mbork@mbork.pl> wrote:
> My proposal is that a "page separator" would be a position in the buffer
> where (looking-at-p page-delimiter) is true, and if point is at such
> a place, then we consider it on the next page.  I.e., in this situation
>
> abcabcabc
> -!-^L
> cbacbacba
>
> the point is already on the second page (unlike the default Emacs
> behavior).

That seems somewhat confusing to me. Intuitively, I would expect the new
page to start after the delimiter, not immediately before it

For comparison, when (looking-at-p "$") returns non-nil, that means
point is at the end of the current line (i.e. before the "\n"), not the
beginning of the next one. (Of course, they're not exactly the same,
since page-delimiter can match multiple characters.)

-- 
john





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-13 20:14                 ` John Mastro
@ 2016-04-13 20:54                   ` Marcin Borkowski
  0 siblings, 0 replies; 21+ messages in thread
From: Marcin Borkowski @ 2016-04-13 20:54 UTC (permalink / raw)
  To: John Mastro; +Cc: 20663, ambrevar


On 2016-04-13, at 20:14, John Mastro <john.b.mastro@gmail.com> wrote:

> Marcin Borkowski <mbork@mbork.pl> wrote:
>> My proposal is that a "page separator" would be a position in the buffer
>> where (looking-at-p page-delimiter) is true, and if point is at such
>> a place, then we consider it on the next page.  I.e., in this situation
>>
>> abcabcabc
>> -!-^L
>> cbacbacba
>>
>> the point is already on the second page (unlike the default Emacs
>> behavior).
>
> That seems somewhat confusing to me. Intuitively, I would expect the new
> page to start after the delimiter, not immediately before it
>
> For comparison, when (looking-at-p "$") returns non-nil, that means
> point is at the end of the current line (i.e. before the "\n"), not the
> beginning of the next one. (Of course, they're not exactly the same,
> since page-delimiter can match multiple characters.)

Well, I'm fine with that version, too - but I'd insist that we should
settle on _something_ and make it clear in the docs.

BTW, the argument for my variant would be that a new page would always
start at beginning of (some) line (assuming that `page-delimiter' starts
with "^", as it does by default).  (I don't claim that it's especially
strong argument, I just wanted to mention it.)

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-13 17:53               ` Marcin Borkowski
  2016-04-13 20:14                 ` John Mastro
@ 2016-04-16 11:03                 ` Marcin Borkowski
  2016-04-16 11:26                 ` Eli Zaretskii
  2 siblings, 0 replies; 21+ messages in thread
From: Marcin Borkowski @ 2016-04-16 11:03 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 20663, ambrevar


On 2016-04-13, at 17:53, Marcin Borkowski <mbork@mbork.pl> wrote:

> Here's the relevant excerpt from the manual:
>
> --8<---------------cut here---------------start------------->8---
>    The variable ‘page-delimiter’ controls where pages begin.  Its value
> is a regular expression that matches the beginning of a line that
> separates pages (*note Regexps::).  The normal value of this variable is
> ‘"^\f"’, which matches a formfeed character at the beginning of a line.
> --8<---------------cut here---------------end--------------->8---
>
> Is it me or is the above ambiguous?
>
>
> I'm a bit busy now, but I'll make a second attempt at this issue within
> a few days.  One of the problems is that (as the above paragraph seems
> to confirm) the very notion of a "page" in Emacs is vague.  IMHO we
> should start with a clear definition of a "page".  It is well possible
> that different functions in page.el use different interpretations of
> this notion, and the bug is just a symptom if such a mess.
>
> My proposal is that a "page separator" would be a position in the buffer
> where (looking-at-p page-delimiter) is true, and if point is at such
> a place, then we consider it on the next page.  I.e., in this situation
>
> abcabcabc
> -!-^L
> cbacbacba
>
> the point is already on the second page (unlike the default Emacs
> behavior).
>
> Then, someone should study page.el and where necessary, update it to the
> precisely defined notion of a "page".  (The mythical "someone" might be
> me.)
>
> WDYT?

OK, I got almost no responses so far:-(.  In the meantime, I started to
study this issue.  It seems that `forward-page' indeed has a bug.
Here's the recipe (slightly modified from the OP's one).  With this
buffer state:

--8<---------------cut here---------------start------------->8---
Page 1
-!-^L
Page 2
^L
Page 3
--8<---------------cut here---------------end--------------->8---

press `C-x ]' (`forward-page').  Now the point moves across /two/
form-feed characters.  Is that intentional?  Seems broken for me.

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-13 17:53               ` Marcin Borkowski
  2016-04-13 20:14                 ` John Mastro
  2016-04-16 11:03                 ` Marcin Borkowski
@ 2016-04-16 11:26                 ` Eli Zaretskii
  2016-04-20  7:32                   ` Marcin Borkowski
  2 siblings, 1 reply; 21+ messages in thread
From: Eli Zaretskii @ 2016-04-16 11:26 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663, ambrevar

> From: Marcin Borkowski <mbork@mbork.pl>
> Cc: ambrevar@gmail.com, 20663@debbugs.gnu.org
> Date: Wed, 13 Apr 2016 19:53:31 +0200
> 
> > Can one of you please explain why the original code misbehaves?
> 
> Quoting from the original report:
> 
> --8<---------------cut here---------------start------------->8---
> > Running `emacs -Q example.txt`:
> >
> > 	M-<
> > 	C-x n p
> > 	M->
> > 	M-1 C-x n p
> >
> > This should bring us from page 1 to page 2, but page 3 gets displayed instead.
> --8<---------------cut here---------------end--------------->8---

Yes, I've read that.  I asked for an explanation of _why_ the code
currently in Emacs misbehaves in this recipe.  Can one of you describe
that?

Thanks.





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-16 11:26                 ` Eli Zaretskii
@ 2016-04-20  7:32                   ` Marcin Borkowski
  2016-04-27  7:57                     ` Pierre Neidhardt
  0 siblings, 1 reply; 21+ messages in thread
From: Marcin Borkowski @ 2016-04-20  7:32 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 20663, ambrevar


On 2016-04-16, at 11:26, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Marcin Borkowski <mbork@mbork.pl>
>> Cc: ambrevar@gmail.com, 20663@debbugs.gnu.org
>> Date: Wed, 13 Apr 2016 19:53:31 +0200
>> 
>> > Can one of you please explain why the original code misbehaves?
>> 
>> Quoting from the original report:
>> 
>> --8<---------------cut here---------------start------------->8---
>> > Running `emacs -Q example.txt`:
>> >
>> > 	M-<
>> > 	C-x n p
>> > 	M->
>> > 	M-1 C-x n p
>> >
>> > This should bring us from page 1 to page 2, but page 3 gets displayed instead.
>> --8<---------------cut here---------------end--------------->8---
>
> Yes, I've read that.  I asked for an explanation of _why_ the code
> currently in Emacs misbehaves in this recipe.  Can one of you describe
> that?

Let me try (note: this is partly my conjecture!).

Since the concept of a "page delimiter" is vague in Emacs (the manual
suggests that a page delimiter is a _line_ such that `page-delimiter'
matches at its beginning, common sense suggests that it is a _substring_
of the buffer matching `page-delimiter', it's probable that someone
decided that if the point is at the beginning of a line and matches
`page-delimiter', then `forward-page' shouldn't just move past the text
matching `page-delimiter' we are on, but the next one.  So the author of
the current version of `forward-page' decided just to move one character
forward (in case we are at the line beginning), so that `page-delimiter'
won't match.

This I consider a Bad Idea™.  As I mentioned earlier, I think the
concept of a "page" and "page delimiter" should be made more precise;
then, it should be enough to correct `forward-page' (basically all other
functions in page.el depend on it, directly or not).  Currently Emacs
seems to treat the _line_ as the page delimiter, but as the OP noted,
this yields strange/unintuitive results with narrowing.

And by the way, the patch the OP gave is also wrong, though in
a different way.  (I should have noticed that earlier.)  The OP proposed
this instead of (if (bolp) (forward-char 1)):

(if (string= page-delimiter "") (forward-char 1))

Of course, this condition should never be true: if `page-delimiter' is
"", functions from page.el will most probably never work correctly
anyway.  What (maybe) should have been tested would be

(eq (match-beginning 0) (match-end 0))

but anyway, since Emacs regex engine does not have a lot of zero-width
assertions, this is not going to happen very often anyway (certainly
never with the default value of `page-delimiter', which explains why the
OP hasn't noticed any problems with his patch).  One possible value of
`page-delimiter' that comes to my mind which could lead to the above
condition holding would be "^$" -- though I can hardly see any practical
use for it.

So my proposal would be to just delete the offending line altogether.
I'd be very surprised if we heard any complaints afterwards.

BTW, the analogous code for moving back one page seems also suspicious
to me.  I'll look into it tomorrow.

> Thanks.

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-20  7:32                   ` Marcin Borkowski
@ 2016-04-27  7:57                     ` Pierre Neidhardt
  2016-05-02 20:42                       ` Marcin Borkowski
  0 siblings, 1 reply; 21+ messages in thread
From: Pierre Neidhardt @ 2016-04-27  7:57 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663

On 16-04-20 09:32:37, Marcin Borkowski wrote:
> And by the way, the patch the OP gave is also wrong, though in
> a different way.  (I should have noticed that earlier.)  The OP proposed
> this instead of (if (bolp) (forward-char 1)):
> 
> (if (string= page-delimiter "") (forward-char 1))
> 
> Of course, this condition should never be true: if `page-delimiter' is
> "", functions from page.el will most probably never work correctly
> anyway.  What (maybe) should have been tested would be
> 
> (eq (match-beginning 0) (match-end 0))
> 
> but anyway, since Emacs regex engine does not have a lot of zero-width
> assertions, this is not going to happen very often anyway (certainly
> never with the default value of `page-delimiter', which explains why the
> OP hasn't noticed any problems with his patch).  One possible value of
> `page-delimiter' that comes to my mind which could lead to the above
> condition holding would be "^$" -- though I can hardly see any practical
> use for it.
> 
> So my proposal would be to just delete the offending line altogether.
> I'd be very surprised if we heard any complaints afterwards.

Why disallowing "^$" as a page delimiter? I would not use it either, but I can
fathom that somebody else would. At the end of the day, the page display is like
colors: a matter of taste. So instead of removing the line, I'd use the
replacement you suggested.

> BTW, the analogous code for moving back one page seems also suspicious
> to me.  I'll look into it tomorrow.

The code for moving back first skips the page delimiter we are currently on, if
any. This is not problematic if the page was narrowed down since it is not
possible to be on the page delimiter at (point-min).

The code looks good to me if we consider that the delimiters belong to the end
of a page.

-- 
Pierre Neidhardt





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-04-27  7:57                     ` Pierre Neidhardt
@ 2016-05-02 20:42                       ` Marcin Borkowski
  2016-06-04  9:55                         ` Pierre Neidhardt
  0 siblings, 1 reply; 21+ messages in thread
From: Marcin Borkowski @ 2016-05-02 20:42 UTC (permalink / raw)
  To: Pierre Neidhardt; +Cc: 20663


On 2016-04-27, at 09:57, Pierre Neidhardt <ambrevar@gmail.com> wrote:

> On 16-04-20 09:32:37, Marcin Borkowski wrote:
>> And by the way, the patch the OP gave is also wrong, though in
>> a different way.  (I should have noticed that earlier.)  The OP proposed
>> this instead of (if (bolp) (forward-char 1)):
>> 
>> (if (string= page-delimiter "") (forward-char 1))
>> 
>> Of course, this condition should never be true: if `page-delimiter' is
>> "", functions from page.el will most probably never work correctly
>> anyway.  What (maybe) should have been tested would be
>> 
>> (eq (match-beginning 0) (match-end 0))
>> 
>> but anyway, since Emacs regex engine does not have a lot of zero-width
>> assertions, this is not going to happen very often anyway (certainly
>> never with the default value of `page-delimiter', which explains why the
>> OP hasn't noticed any problems with his patch).  One possible value of
>> `page-delimiter' that comes to my mind which could lead to the above
>> condition holding would be "^$" -- though I can hardly see any practical
>> use for it.
>> 
>> So my proposal would be to just delete the offending line altogether.
>> I'd be very surprised if we heard any complaints afterwards.
>
> Why disallowing "^$" as a page delimiter? I would not use it either, but I can
> fathom that somebody else would. At the end of the day, the page display is like
> colors: a matter of taste. So instead of removing the line, I'd use the
> replacement you suggested.

I'm not saying it should be disallowed.  I'm saying that most probably
it's not very useful, and I'd be surprised if anyone used it.  But
I agree with your suggestion anyway, if only for aesthetical reasons.

>> BTW, the analogous code for moving back one page seems also suspicious
>> to me.  I'll look into it tomorrow.
>
> The code for moving back first skips the page delimiter we are currently on, if
> any. This is not problematic if the page was narrowed down since it is not
> possible to be on the page delimiter at (point-min).
>
> The code looks good to me if we consider that the delimiters belong to the end
> of a page.

I'll try to look at them again.

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-05-02 20:42                       ` Marcin Borkowski
@ 2016-06-04  9:55                         ` Pierre Neidhardt
  2016-06-04 20:36                           ` Marcin Borkowski
  0 siblings, 1 reply; 21+ messages in thread
From: Pierre Neidhardt @ 2016-06-04  9:55 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663

Any update?





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-06-04  9:55                         ` Pierre Neidhardt
@ 2016-06-04 20:36                           ` Marcin Borkowski
  2020-09-15 13:53                             ` Lars Ingebrigtsen
  0 siblings, 1 reply; 21+ messages in thread
From: Marcin Borkowski @ 2016-06-04 20:36 UTC (permalink / raw)
  To: Pierre Neidhardt; +Cc: 20663


On 2016-06-04, at 11:55, Pierre Neidhardt <ambrevar@gmail.com> wrote:

> Any update?

Sorry, no; I don't have much time for Emacs bugs now.  I'm still working
on them, but very slowly; I'll get to this issue next (hopefully).  Stay
tuned.

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2016-06-04 20:36                           ` Marcin Borkowski
@ 2020-09-15 13:53                             ` Lars Ingebrigtsen
  0 siblings, 0 replies; 21+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-15 13:53 UTC (permalink / raw)
  To: Marcin Borkowski; +Cc: 20663, Pierre Neidhardt

Marcin Borkowski <mbork@mbork.pl> writes:

> On 2016-06-04, at 11:55, Pierre Neidhardt <ambrevar@gmail.com> wrote:
>
>> Any update?
>
> Sorry, no; I don't have much time for Emacs bugs now.  I'm still working
> on them, but very slowly; I'll get to this issue next (hopefully).  Stay
> tuned.

(This was four years ago.)

This bug is still present in Emacs 28 -- did you find any time to work
on this problem some more?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
  2015-05-26 17:14 bug#20663: page.el (forward-page): Avoid skipping pages Pierre Neidhardt
  2016-04-09 10:13 ` Marcin Borkowski
@ 2022-04-22 12:05 ` Lars Ingebrigtsen
       [not found]   ` <87pmjje5mt.fsf@kraus.my>
  1 sibling, 1 reply; 21+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-22 12:05 UTC (permalink / raw)
  To: Pierre Neidhardt; +Cc: 20663

Pierre Neidhardt <ambrevar@gmail.com> writes:

> When `page-delimiter` starts at the beginning of the line and the position is
> also at the beginning of the line, calling `forward-page` will skip one page.
>
> Running `emacs -Q example.txt`:
>
> 	M-<
> 	C-x n p
> 	M->
> 	M-1 C-x n p
>
> This should bring us from page 1 to page 2, but page 3 gets displayed instead.
>
> The attached patch fixes it by changing the code to actually match its
> surrounding comments.

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

I've now fixed this problem in a somewhat different way than originally
suggested in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 21+ messages in thread

* bug#20663: page.el (forward-page): Avoid skipping pages
       [not found]   ` <87pmjje5mt.fsf@kraus.my>
@ 2022-06-09 10:21     ` Lars Ingebrigtsen
  0 siblings, 0 replies; 21+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-09 10:21 UTC (permalink / raw)
  To: Daniel Kraus; +Cc: 20663, Pierre Neidhardt

Daniel Kraus <daniel@kraus.my> writes:

>>> When `page-delimiter` starts at the beginning of the line and the position is
>>> also at the beginning of the line, calling `forward-page` will skip one page.
>>>
>>> Running `emacs -Q example.txt`:
>>>
>>> 	M-<
>>> 	C-x n p
>>> 	M->
>>> 	M-1 C-x n p
>>>
>>> This should bring us from page 1 to page 2, but page 3 gets
>>> displayed instead.

[...]

> This patch seems to change the behavior of `forward-page` compared to before.
> Before when the point was inside (/beginning) of the page-delimiter regex
> it would jump imho correctly to the next page.
>
> Now it just skips the page delimiter and goes to the end.

Well, that was the point of the change, really

The problem with the old behaviour was most obvious when narrowing to a
page.  If you're narrowed to page 1 (with point at the end of the
buffer), issuing a "go to the next page" would take you to page 3.

I.e., there was a disconnect between what it was considering the
"current page" and how narrowing to a page would display that, and the
change made these things match up -- now (with point just before the
page delimiter) it says that it's on the previous page, not the next
page.

Perhaps this should be tweaked for longer page delimiters, so that we're
on the next page when point is inside the delimiter?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-06-09 10:21 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-26 17:14 bug#20663: page.el (forward-page): Avoid skipping pages Pierre Neidhardt
2016-04-09 10:13 ` Marcin Borkowski
2016-04-09 12:00   ` Eli Zaretskii
2016-04-09 18:16     ` Marcin Borkowski
2016-04-09 19:34       ` Eli Zaretskii
2016-04-10  1:29         ` Pierre Neidhardt
2016-04-11 10:20           ` Marcin Borkowski
2016-04-11 15:35             ` Eli Zaretskii
2016-04-13 17:53               ` Marcin Borkowski
2016-04-13 20:14                 ` John Mastro
2016-04-13 20:54                   ` Marcin Borkowski
2016-04-16 11:03                 ` Marcin Borkowski
2016-04-16 11:26                 ` Eli Zaretskii
2016-04-20  7:32                   ` Marcin Borkowski
2016-04-27  7:57                     ` Pierre Neidhardt
2016-05-02 20:42                       ` Marcin Borkowski
2016-06-04  9:55                         ` Pierre Neidhardt
2016-06-04 20:36                           ` Marcin Borkowski
2020-09-15 13:53                             ` Lars Ingebrigtsen
2022-04-22 12:05 ` Lars Ingebrigtsen
     [not found]   ` <87pmjje5mt.fsf@kraus.my>
2022-06-09 10:21     ` Lars Ingebrigtsen

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).