bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

[Ivan Shmakov]

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

Package:  emacs
Severity: normal

	Shortly: go to *eww*, H, C-x 1, C-x b *foo* RET, C-x b RET, RET,
	– the EWW history entry will be rendered in *foo*, /not/ *eww*.

	As of 0ee10aff098b, eww-history-browse uses quit-window to leave
	the history buffer, switching to the “next” buffer – as per
	(buffer-list), – and calls eww-restore-history right there.

	While the user may generally be expected to use M-x eww-history
	(which makes the current *eww* buffer the “next” one), select an
	entry, and M-x eww-history-browse it, – it’s also possible for
	the user to switch buffers arbitrarily between the calls.  This
	way, quit-window may select a completely unrelated buffer, and
	eww-restore-history will then call (erase-buffer) for it.

	(Naturally, that gets much worse if the buffer that the user
	choose to take a look at actually has some important data.)

	As for #16211, my suggestion would be to employ a separate
	buffer-local variable to track the current “browsing” buffer.
	In the patch MIMEd, I assume that it’s named eww-current-buffer
	(just like in the #16211 patch), although eww-browsing-buffer
	seems like a more appropriate name to me now.

	As suggested, the code behaves as before when the variable is
	nil.  It may make sense to raise an error in that case instead,
	so to avoid possible corruption of an arbitrary buffer.

PS.  Beware of the loose line numbers in the diff.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 814 bytes --]

--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -1290,9 +1290,11 @@ defun eww-list-histories ()
   (interactive)
   (when (null eww-history)
     (error "No eww-histories are defined"))
-  (let ((eww-history-trans eww-history))
+  (let ((eww-history-trans eww-history)
+	(buffer (current-buffer)))
     (set-buffer (get-buffer-create "*eww history*"))
     (eww-history-mode)
+    (setq-local eww-current-buffer buffer)
     (let ((inhibit-read-only t)
 	  (domain-length 0)
 	  (title-length 0)
@@ -1302,6 +1304,9 @@ defun eww-history-browse ()
     (unless history
       (error "No history on the current line"))
-    (quit-window)
+    (let ((buffer eww-current-buffer))
+      (quit-window)
+      (when buffer
+	(switch-to-buffer buffer)))
     (eww-restore-history history)))
 
 (defvar eww-history-mode-map

bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

[Lars Magne Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> Shortly: go to *eww*, H, C-x 1, C-x b *foo* RET, C-x b RET, RET,
> – the EWW history entry will be rendered in *foo*, /not/ *eww*.
>
> As of 0ee10aff098b, eww-history-browse uses quit-window to leave
> the history buffer, switching to the “next” buffer – as per
> (buffer-list), – and calls eww-restore-history right there.
>
> While the user may generally be expected to use M-x eww-history
> (which makes the current *eww* buffer the “next” one), select an
> entry, and M-x eww-history-browse it, – it’s also possible for
> the user to switch buffers arbitrarily between the calls.  This
> way, quit-window may select a completely unrelated buffer, and
> eww-restore-history will then call (erase-buffer) for it.

Hm....

--------
(quit-window &optional KILL WINDOW)

Quit WINDOW and bury its buffer.
WINDOW must be a live window and defaults to the selected one.
With prefix argument KILL non-nil, kill the buffer instead of
burying it.

According to information stored in WINDOW's `quit-restore' window
parameter either (1) delete WINDOW and its frame, (2) delete
WINDOW, (3) restore the buffer previously displayed in WINDOW,
or (4) make WINDOW display some other buffer than the present
one.  If non-nil, reset `quit-restore' parameter to nil.
--------

So I guess eww should set up the `quit-restore' thing properly.  I'll
have a peek.

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

bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> So I guess eww should set up the `quit-restore' thing properly.  I'll
> have a peek.

Actually, that doesn't provide a way to do this (rather obvious) thing.

Shouldn't there be a way to instruct `quit-window' to go to a specific
buffer (if that buffer is still alive)?

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

bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

[Ivan Shmakov]

>>>>> Lars Magne Ingebrigtsen <larsi@gnus.org> writes:
>>>>> Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

 >> So I guess eww should set up the `quit-restore' thing properly.
 >> I'll have a peek.

 > Actually, that doesn't provide a way to do this (rather obvious)
 > thing.

 > Shouldn't there be a way to instruct `quit-window' to go to a
 > specific buffer (if that buffer is still alive)?

	I see no reason to abuse quit-window for what’s essentially
	switching to a buffer to edit one.  That is: the
	eww-restore-history call right after quit-window edits the
	/current/ buffer.  Thus, it indeed makes sense to explicitly use
	set-buffer before that.  (Or to wrap the call in
	with-current-buffer, anyway.)

	Moreover, vc.el already uses a dedicated buffer-local variable
	for a similar purpose, and so does rcirc.el, and perhaps a few
	more modes out there.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

[Lars Magne Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> I see no reason to abuse quit-window for what’s essentially
> switching to a buffer to edit one.  That is: the
> eww-restore-history call right after quit-window edits the
> /current/ buffer.  Thus, it indeed makes sense to explicitly use
> set-buffer before that.  (Or to wrap the call in
> with-current-buffer, anyway.)
>
> Moreover, vc.el already uses a dedicated buffer-local variable
> for a similar purpose, and so does rcirc.el, and perhaps a few
> more modes out there.

I think you're arguing against yourself here.  If this is such a common
thing to do, then something in this setup should provide this
functionality, so that all these modes don't have to implement it again
and again.

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

bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

[Ivan Shmakov]

>>>>> Lars Magne Ingebrigtsen <larsi@gnus.org> writes:
>>>>> Ivan Shmakov <ivan@siamics.net> writes:

 >> I see no reason to abuse quit-window for what’s essentially
 >> switching to a buffer to edit one.  That is: the eww-restore-history
 >> call right after quit-window edits the /current/ buffer.  Thus, it
 >> indeed makes sense to explicitly use set-buffer before that.  (Or to
 >> wrap the call in with-current-buffer, anyway.)

 >> Moreover, vc.el already uses a dedicated buffer-local variable for a
 >> similar purpose, and so does rcirc.el, and perhaps a few more modes
 >> out there.

 > I think you're arguing against yourself here.

	Not in the least.

 > If this is such a common thing to do, then something in this setup
 > should provide this functionality, so that all these modes don't have
 > to implement it again and again.

	The vc-parent-buffer variable is /not/ used just to get back to
	the controlled file’s buffer; instead, it allows for the VC
	commands to be used in the associated buffers /as if/ they were
	invoked in the file’s own one.  As in: the user does C-x v = to
	see the diff; finds something of interest, and – whoa! – the VC
	log is only C-x v l away.

	In the case of eww-history-browse, (quit-window) is part of the
	user interface, and I’m perfectly fine with it.  (Although I
	/do/ imagine a use case which would require eww-history-browse
	to leave the history window in place.)

	On the contrary, the requirement to be invoked in the right
	buffer is part of the eww-restore-history calling convention.
	Even though the users may have different opinions regarding
	which buffer or window (if any) to select upon finishing with
	the history, the necessity to call eww-restore-history from the
	right one remains entirely the same.

	There is indeed some freedom with respect to the location the
	EWW buffer proper gets referenced from, – it could be a variable
	(as I’ve suggested before), a text property, or some arcane
	location quit-window would use.  However, I’d like to note that
	I currently also use EWW to render previews of the buffer’s
	MediaWiki markup [1], and that already benefits from having the
	target EWW buffer linked via a buffer-local variable.  (Contrary
	to, say, M-x mml-preview, it /is/ reasonable to direct the
	output into a single buffer for all the repeated uses of the
	command in the same “source” buffer, thanks to this very
	“history” feature EWW provides.)

[1] http://am-1.org/~ivan/archives/git/gitweb.cgi?p=mw-el-2014.git;a=blob;f=mw-eww.el

	I presume that such an approach will hold for the other cases
	where a given buffer’s contents needs to be rendered with EWW,
	possibly after passing through a specific local (say,
	$ markdown) or remote software.  It’s even possible to provide
	an M-x eww-preview command of some kind for that very purpose.
	The command will either use the (live) buffer pointed to by this
	new buffer-local variable; or will search for one, or create one
	anew, possibly by running a user-specified function or hook.

	(The command would employ one another hook to perform the
	conversion of the source data into HTML.)

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#18550: eww-history-browse may end up calling eww-restore-history in an arbitrary buffer

[Lars Magne Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> 	Shortly: go to *eww*, H, C-x 1, C-x b *foo* RET, C-x b RET, RET,
> 	– the EWW history entry will be rendered in *foo*, /not/ *eww*.

Thanks; applied.

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