* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' [not found] ` <20171216141056.8391A24612@vcs0.savannah.gnu.org> @ 2017-12-16 15:04 ` Stefan Monnier 2017-12-16 16:04 ` Eli Zaretskii 0 siblings, 1 reply; 9+ messages in thread From: Stefan Monnier @ 2017-12-16 15:04 UTC (permalink / raw) To: emacs-devel; +Cc: Eli Zaretskii > + ;; 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-16 15:04 ` [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' Stefan Monnier @ 2017-12-16 16:04 ` Eli Zaretskii 2017-12-16 22:23 ` Stefan Monnier 0 siblings, 1 reply; 9+ messages in thread From: Eli Zaretskii @ 2017-12-16 16:04 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-16 16:04 ` Eli Zaretskii @ 2017-12-16 22:23 ` Stefan Monnier 2017-12-17 15:44 ` Eli Zaretskii 0 siblings, 1 reply; 9+ messages in thread From: Stefan Monnier @ 2017-12-16 22:23 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel >> > + ;; 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-16 22:23 ` Stefan Monnier @ 2017-12-17 15:44 ` Eli Zaretskii 2017-12-17 17:54 ` martin rudalics 0 siblings, 1 reply; 9+ messages in thread From: Eli Zaretskii @ 2017-12-17 15:44 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-17 15:44 ` Eli Zaretskii @ 2017-12-17 17:54 ` martin rudalics 2017-12-17 20:22 ` Eli Zaretskii 0 siblings, 1 reply; 9+ messages in thread From: martin rudalics @ 2017-12-17 17:54 UTC (permalink / raw) To: Eli Zaretskii, Stefan Monnier; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-17 17:54 ` martin rudalics @ 2017-12-17 20:22 ` Eli Zaretskii 2017-12-17 21:20 ` martin rudalics 0 siblings, 1 reply; 9+ messages in thread From: Eli Zaretskii @ 2017-12-17 20:22 UTC (permalink / raw) To: martin rudalics; +Cc: monnier, emacs-devel > 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.) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-17 20:22 ` Eli Zaretskii @ 2017-12-17 21:20 ` martin rudalics 2017-12-18 3:09 ` Stefan Monnier 0 siblings, 1 reply; 9+ messages in thread From: martin rudalics @ 2017-12-17 21:20 UTC (permalink / raw) To: Eli Zaretskii; +Cc: monnier, emacs-devel > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-17 21:20 ` martin rudalics @ 2017-12-18 3:09 ` Stefan Monnier 2017-12-18 7:26 ` martin rudalics 0 siblings, 1 reply; 9+ messages in thread From: Stefan Monnier @ 2017-12-18 3:09 UTC (permalink / raw) To: emacs-devel >> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' 2017-12-18 3:09 ` Stefan Monnier @ 2017-12-18 7:26 ` martin rudalics 0 siblings, 0 replies; 9+ messages in thread From: martin rudalics @ 2017-12-18 7:26 UTC (permalink / raw) To: Stefan Monnier, emacs-devel > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-18 7:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20171216141055.30854.67661@vcs0.savannah.gnu.org> [not found] ` <20171216141056.8391A24612@vcs0.savannah.gnu.org> 2017-12-16 15:04 ` [Emacs-diffs] emacs-26 f274cbd: Avoid reordering of output in 'shr-insert-document' Stefan Monnier 2017-12-16 16:04 ` Eli Zaretskii 2017-12-16 22:23 ` Stefan Monnier 2017-12-17 15:44 ` Eli Zaretskii 2017-12-17 17:54 ` martin rudalics 2017-12-17 20:22 ` Eli Zaretskii 2017-12-17 21:20 ` martin rudalics 2017-12-18 3:09 ` Stefan Monnier 2017-12-18 7:26 ` martin rudalics
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.