Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[Stefan Monnier]

> +    ;; Save and restore point across with-temp-buffer, since
> +    ;; shr-pixel-column uses save-window-excursion, which can reset
> +    ;; point to 1.
> +    (let ((pt (point)))
> +      (with-temp-buffer
> +        (insert string)
> +        (shr-pixel-column))
> +      (goto-char pt))))

Is it possible to fix the problem in shr-pixel-column (since, according
to the comment above, the problems comes from there)?


        Stefan

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 16 Dec 2017 10:04:06 -0500
> 
> > +    ;; Save and restore point across with-temp-buffer, since
> > +    ;; shr-pixel-column uses save-window-excursion, which can reset
> > +    ;; point to 1.
> > +    (let ((pt (point)))
> > +      (with-temp-buffer
> > +        (insert string)
> > +        (shr-pixel-column))
> > +      (goto-char pt))))
> 
> Is it possible to fix the problem in shr-pixel-column (since, according
> to the comment above, the problems comes from there)?

No, because the value of point to preserve is from the buffer which
was current before with-temp-buffer.

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[Stefan Monnier]

>> > +    ;; Save and restore point across with-temp-buffer, since
>> > +    ;; shr-pixel-column uses save-window-excursion, which can reset
>> > +    ;; point to 1.
>> > +    (let ((pt (point)))
>> > +      (with-temp-buffer
>> > +        (insert string)
>> > +        (shr-pixel-column))
>> > +      (goto-char pt))))
>> Is it possible to fix the problem in shr-pixel-column (since, according
>> to the comment above, the problems comes from there)?
> No, because the value of point to preserve is from the buffer which
> was current before with-temp-buffer.

But when we enter shr-pixel-column, that buffer's point is still the
right one, and when we leave shr-pixel-column it isn't any more, so
somehow shr-pixel-column manages to "find" that buffer in order to
modify it and hence we should also be able to similarly find that buffer
and restore its point.

I mean, from a purely theoretical point of view, it *can* be
solved there.  What I don't understand is where practical aspects end up
getting it the way.


        Stefan

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Sat, 16 Dec 2017 17:23:02 -0500
> 
> >> Is it possible to fix the problem in shr-pixel-column (since, according
> >> to the comment above, the problems comes from there)?
> > No, because the value of point to preserve is from the buffer which
> > was current before with-temp-buffer.
> 
> But when we enter shr-pixel-column, that buffer's point is still the
> right one, and when we leave shr-pixel-column it isn't any more, so
> somehow shr-pixel-column manages to "find" that buffer in order to
> modify it and hence we should also be able to similarly find that buffer
> and restore its point.
> 
> I mean, from a purely theoretical point of view, it *can* be
> solved there.  What I don't understand is where practical aspects end up
> getting it the way.

Purely theoretically, you could always do something like

  (with-current-buffer (window-buffer)
    (point))

to get at the value of point at entry into shr-pixel-column, and then
a similar gork before exiting.  But is that a good idea?  It will slow
down shr-pixel-column, which is already one of the hottest spots in
shr.el's rendering.  I don't think Lars will want to talk to me after
that...

What actually happens here is that save-window-excursion gets run
afoul by the preceding with-temp-buffer, and ends up saving the wrong
value of point.  Specifically, save_window_save has this snippet:

      if (BUFFERP (w->contents))
	{
	  /* Save w's value of point in the window configuration.  If w
	     is the selected window, then get the value of point from
	     the buffer; pointm is garbage in the selected window.  */
	  if (EQ (window, selected_window))
	    p->pointm = build_marker (XBUFFER (w->contents),
				      BUF_PT (XBUFFER (w->contents)),
				      BUF_PT_BYTE (XBUFFER (w->contents)));
	  else
	    p->pointm = Fcopy_marker (w->pointm, Qnil);

In the scenario we have in this case, it decides to use w->pointm,
whereas the correct value is in BUF_PT.

Maybe Martin will have an idea for how to fix the logic there,
although it already sounds quite fragile to me, enough so to be afraid
of making any changes there on the release branch.  But maybe for
master we could find a solution that is reliable enough.  Maybe.  On
the release branch, I see no cake left to eat.

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[martin rudalics]

 > Purely theoretically, you could always do something like
 >
 >    (with-current-buffer (window-buffer)
 >      (point))
 > to get at the value of point at entry into shr-pixel-column, and then
 > a similar gork before exiting.  But is that a good idea?  It will slow
 > down shr-pixel-column, which is already one of the hottest spots in
 > shr.el's rendering.  I don't think Lars will want to talk to me after
 > that...

We could add a new function `buffer-point' to return the position of
point in an arbitrary buffer.  Moreover IMO `save-window-excursion'
already eats much more time and space than `with-current-buffer'.

But I think Stefan is correct in the sense that this is a problem with
`shr-pixel-column' and the fix should be there.  Something like

(defun shr-pixel-column ()
   (if (not shr-use-fonts)
       (current-column)
     (if (not (get-buffer-window (current-buffer)))
         (let ((window-point (window-point)))
           (prog1
               (save-window-excursion
                 ;; Avoid errors if the selected window is a dedicated one,
                 ;; and they just want to insert a document into it.
                 (set-window-dedicated-p nil nil)
	        (set-window-buffer nil (current-buffer))
	        (car (window-text-pixel-size nil (line-beginning-position) (point))))
             (set-window-point nil window-point)))
       (car (window-text-pixel-size nil (line-beginning-position) (point))))))

but maybe I'm completely misunderstanding the problem at hand.

martin

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[Eli Zaretskii]

> Date: Sun, 17 Dec 2017 18:54:05 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: emacs-devel@gnu.org
> 
>      (if (not (get-buffer-window (current-buffer)))
>          (let ((window-point (window-point)))
>            (prog1
>                (save-window-excursion
>                  ;; Avoid errors if the selected window is a dedicated one,
>                  ;; and they just want to insert a document into it.
>                  (set-window-dedicated-p nil nil)
> 	        (set-window-buffer nil (current-buffer))
> 	        (car (window-text-pixel-size nil (line-beginning-position) (point))))
>              (set-window-point nil window-point)))

If we do that, then why use save-window-excursion in the first place?
That's what it's supposed to preserve (among other things), right?
If it doesn't do that much, let's do its job by hand altogether.  (And
then, of course, a few years down the line someone will come and ask
herself why don't they just use save-window-excursion.)

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[martin rudalics]

 > If we do that, then why use save-window-excursion in the first place?
 > That's what it's supposed to preserve (among other things), right?

Not really.  The Elisp manual says about `save-window-excursion' that

      This macro records the window configuration of the selected frame,
      executes FORMS in sequence, then restores the earlier window
      configuration.

and about window configurations

     As a special exception, the window
   configuration does not record the value of point in the selected window
   for the current buffer.

so the point of the selected window is not necessarily restored.

 > If it doesn't do that much, let's do its job by hand altogether.  (And
 > then, of course, a few years down the line someone will come and ask
 > herself why don't they just use save-window-excursion.)

Well, `set-window-buffer' isn't that innocuous either.

Note that `shr-pixel-column' will also fail when the current buffer is
not shown in the selected window.  Hence, a more correct version should
do:

(defun shr-pixel-column ()
   (cond
    ((not shr-use-fonts)
     (current-column))
    ((not (eq (window-buffer) (current-buffer)))
     (let ((window-point (window-point)))
       (prog1
           (save-window-excursion
             ;; Avoid errors if the selected window is a dedicated one,
             ;; and they just want to insert a document into it.
             (set-window-dedicated-p nil nil)
	    (set-window-buffer nil (current-buffer))
	    (car (window-text-pixel-size nil (line-beginning-position) (point))))
         (set-window-point nil window-point))))
    (t
     (car (window-text-pixel-size nil (line-beginning-position) (point))))))

But without a doc-string I obviously can't tell for sure.

martin

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[Stefan Monnier]

>> If we do that, then why use save-window-excursion in the first place?
>> That's what it's supposed to preserve (among other things), right?
> Not really.  The Elisp manual says about `save-window-excursion' that

I think what Eli is getting at is that instead of save-window-excursion
we could just do add a `set-window-buffer` before `set-window-point`.

I think it would make a lot of sense, actually, since
save-window-excursion saves the whole window-config (i.e. data about
all windows in the current frame), whereas we're only going to touch the
selected window.


        Stefan

Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document'

[martin rudalics]

 > I think what Eli is getting at is that instead of save-window-excursion
 > we could just do add a `set-window-buffer` before `set-window-point`.
 >
 > I think it would make a lot of sense, actually, since
 > save-window-excursion saves the whole window-config (i.e. data about
 > all windows in the current frame), whereas we're only going to touch the
 > selected window.

`save-window-excursion' is a pain.  But as I mentioned in my last mail,
`set-window-buffer' is not so innocuous either.  We'd have to care about
the selectd window's old_pointm, start_at_line_beg and start slots at
least.  Something `save-window-excursion' (maybe) does auto-magically.
So I doubt we should do such a change in the release.

martin