gnus-html, url and curl

gnus-html, url and curl

[Julien Danjou]

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

Hi,

Reading gnus-html, I see it uses curl to fetch images. Is there any
reason to not use `url-retrieve' to do that?

`url' has also a `url-cache' module that can be used to do caching. I
recently sent a set of patches to emacs-devel to improve it, and it may
be a good candidate to store and cache HTML images.

I think it would be smarter to use that modules rather than curl, but
maybe you did have a good reason to use curl, Lars?

I can volunteer to rewrite that code with `url' if you think it's a good
idea. :)

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: gnus-html, url and curl

[Lars Magne Ingebrigtsen]

Julien Danjou <julien@danjou.info> writes:

> Reading gnus-html, I see it uses curl to fetch images. Is there any
> reason to not use `url-retrieve' to do that?

No, as long as it's async, that would make more sense than using an
external program.  My worry was that it would be (much) slower using the
`url-*' functions to download (potentially) big pictures, but if it
isn't, I'm all for it.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: gnus-html, url and curl

[Julien Danjou]

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

On Sat, Sep 18 2010, Lars Magne Ingebrigtsen wrote:

> No, as long as it's async, that would make more sense than using an
> external program.  My worry was that it would be (much) slower using the
> `url-*' functions to download (potentially) big pictures, but if it
> isn't, I'm all for it.

It is async. I'll do that then.

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: gnus-html, url and curl

[Lars Magne Ingebrigtsen]

Julien Danjou <julien@danjou.info> writes:

>> No, as long as it's async, that would make more sense than using an
>> external program.  My worry was that it would be (much) slower using the
>> `url-*' functions to download (potentially) big pictures, but if it
>> isn't, I'm all for it.
>
> It is async. I'll do that then.

But is it as fast?  :-)  It might be faster, for all I know, since it
doesn't have to fork curl.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: gnus-html, url and curl

[Steinar Bang]

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

> But is it as fast?  :-)  It might be faster, for all I know, since it
> doesn't have to fork curl.

Just assuming, and not backed by any numbers, it seems likely that the
bottleneck will be the network transfer and not the language handling
the transfer at either end.

And losing a fork should make it faster.

Re: gnus-html, url and curl

[Lars Magne Ingebrigtsen]

Steinar Bang <sb@dod.no> writes:

> Just assuming, and not backed by any numbers, it seems likely that the
> bottleneck will be the network transfer and not the language handling
> the transfer at either end.

It should be, but when doing things in Emacs buffers, it's really easy
to do something that seems straight-forward, but ends up taking a lot of
time.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

[PATCH] Use `url' rather than curl

[Julien Danjou]

Signed-off-by: Julien Danjou <julien@danjou.info>
---
 lisp/ChangeLog    |    5 ++++
 lisp/gnus-html.el |   55 +++++++++++++++++++++++-----------------------------
 2 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index cf95e15..832458f 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2010-09-18  Julien Danjou  <julien@danjou.info>
+
+	* gnus-html.el (gnus-html-schedule-image-fetching): Use `url' rather
+	than curl to retrieve images.
+
 2010-09-18  Lars Magne Ingebrigtsen  <larsi@gnus.org>
 
 	* nnimap.el (nnimap-update-info): Extend the info so that we can set
diff --git a/lisp/gnus-html.el b/lisp/gnus-html.el
index e74fd5b..40b74c2 100644
--- a/lisp/gnus-html.el
+++ b/lisp/gnus-html.el
@@ -33,6 +33,7 @@
 
 (require 'gnus-art)
 (require 'mm-url)
+(require 'url)
 
 (defcustom gnus-html-cache-directory (nnheader-concat gnus-directory "html-cache/")
   "Where Gnus will cache images it downloads from the web."
@@ -296,42 +297,34 @@ fit these criteria."
 (defun gnus-html-schedule-image-fetching (buffer images)
   (gnus-message 8 "gnus-html-schedule-image-fetching: buffer %s, images %s"
                 buffer images)
-  (when (executable-find "curl")
-    (let* ((url (caar images))
-	   (process (start-process
-		     "images" nil "curl"
-		     "-s" "--create-dirs"
-		     "--location"
-		     "--max-time" "60"
-		     "-o" (gnus-html-image-id url)
-		     (mm-url-decode-entities-string url))))
-      (gnus-set-process-query-on-exit-flag process nil)
-      (set-process-sentinel process 'gnus-html-curl-sentinel)
-      (gnus-set-process-plist process (list 'images images
-					    'buffer buffer)))))
+  (url-retrieve (caar images)
+                'gnus-html-image-fetched
+                (list buffer images)))
 
 (defun gnus-html-image-id (url)
   (expand-file-name (sha1 url) gnus-html-cache-directory))
 
-(defun gnus-html-curl-sentinel (process event)
-  (when (string-match "finished" event)
-    (let* ((images (gnus-process-get process 'images))
-	   (buffer (gnus-process-get process 'buffer))
-	   (spec (pop images))
-	   (file (gnus-html-image-id (car spec))))
-      (when (and (buffer-live-p buffer)
-		 ;; If the position of the marker is 1, then that
-		 ;; means that the text it was in has been deleted;
-		 ;; i.e., that the user has selected a different
-		 ;; article before the image arrived.
-		 (not (= (marker-position (cadr spec)) (point-min))))
-	(with-current-buffer buffer
-	  (let ((inhibit-read-only t)
-		(string (buffer-substring (cadr spec) (caddr spec))))
-	    (delete-region (cadr spec) (caddr spec))
-	    (gnus-html-put-image file (cadr spec) string))))
+(defun gnus-html-image-fetched (status buffer images)
+  (let ((spec (pop images)))
+    (when (and (buffer-live-p buffer)
+               ;; If the position of the marker is 1, then that
+               ;; means that the text it was in has been deleted;
+               ;; i.e., that the user has selected a different
+               ;; article before the image arrived.
+               (not (= (marker-position (cadr spec)) (point-min))))
+      (let ((file (gnus-html-image-id (car spec))))
+        ;; Search the start of the image data
+        (search-forward "\n\n")
+        ;; Write region (image) silently
+        (write-region (point) (point-max) file nil 1)
+        (kill-buffer)
+        (with-current-buffer buffer
+          (let ((inhibit-read-only t)
+                (string (buffer-substring (cadr spec) (caddr spec))))
+            (delete-region (cadr spec) (caddr spec))
+            (gnus-html-put-image file (cadr spec) string))))
       (when images
-	(gnus-html-schedule-image-fetching buffer images)))))
+        (gnus-html-schedule-image-fetching buffer images)))))
 
 (defun gnus-html-put-image (file point string &optional url alt-text)
   (when (gnus-graphic-display-p)
-- 
1.7.1

Re: [PATCH] Use `url' rather than curl

[Leo]

On 2010-09-18 19:22 +0100, Julien Danjou wrote:
> +        (kill-buffer)

`kill-buffer' in the callback produces random jumps (usually moves the
point to the end of buffer) in the buffer where the command is invoked.

Use url-mark-buffer-as-dead instead. I have been using this in two of my
packages after being annoyed by the random jumps for a while.

Leo

Re: [PATCH] Use `url' rather than curl

[Julien Danjou]

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

On Sat, Sep 18 2010, Leo wrote:

> `kill-buffer' in the callback produces random jumps (usually moves the
> point to the end of buffer) in the buffer where the command is invoked.

The current buffer is the one we want to kill, so I don't see any
problem to jump around. Or am I misunderstanding you?

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Use `url' rather than curl

[Lars Magne Ingebrigtsen]

Julien Danjou <julien@danjou.info> writes:

> +	* gnus-html.el (gnus-html-schedule-image-fetching): Use `url' rather
> +	than curl to retrieve images.

Seems to work great!  Applied and pushed.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: [PATCH] Use `url' rather than curl

[Leo]

On 2010-09-18 19:34 +0100, Julien Danjou wrote:
>> `kill-buffer' in the callback produces random jumps (usually moves the
>> point to the end of buffer) in the buffer where the command is invoked.
>
> The current buffer is the one we want to kill, so I don't see any
> problem to jump around. Or am I misunderstanding you?

It jumps around in the buffer it switches to when the process buffer is
killed, so it is usually the buffer before invoking the process.

Because the callback is run in the process's filter, if you kill the
buffer, some unpleasant things happen. That point movement was what
annoyed me and how I noticed.

Leo

Re: Use `url' rather than curl

[Adam Sjøgren]

On Sat, 18 Sep 2010 20:39:45 +0200, Lars wrote:

> Julien Danjou <julien@danjou.info> writes:
>> +	* gnus-html.el (gnus-html-schedule-image-fetching): Use `url' rather
>> +	than curl to retrieve images.

> Seems to work great!  Applied and pushed.

I'm afraid it doesn't really work for me in XEmacs 21.5 beta. I get
this, for instance, in the *Messages* buffer:

  Contacting englishrussia.com:80
  Request sent, waiting for response...
  Reading [image/jpeg]... 49k of 175k (28%)
  Retrieval complete.

but the article buffer just keeps containing the - I think -
w3m.el-munged HTML, and no pictures appears.


  Best regards,

    Adam

-- 
 "It's my chainsaw                                            Adam Sjøgren
  Division is mine"                                      asjo@koldfront.dk

Re: [PATCH] Use `url' rather than curl

[Lars Magne Ingebrigtsen]

Leo <sdl.web@gmail.com> writes:

> It jumps around in the buffer it switches to when the process buffer is
> killed, so it is usually the buffer before invoking the process.

I kill buffers with processes in them all the time, and I haven't
noticed this.  Sounds like an Emacs bug of some kind?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: [PATCH] Use `url' rather than curl

[Leo]

On 2010-09-18 20:12 +0100, Lars Magne Ingebrigtsen wrote:
> Leo <sdl.web@gmail.com> writes:
>
>> It jumps around in the buffer it switches to when the process buffer is
>> killed, so it is usually the buffer before invoking the process.
>
> I kill buffers with processes in them all the time, and I haven't
> noticed this.  Sounds like an Emacs bug of some kind?

I think (not sure) that is part of the URL's flow. After it invokes the
callback provided by the user, it does other things which require the
process buffer to be around. kill-buffer inside the process's
filter/sentinel, disrupt its working.

When you kill the buffer (C-x k), it is not inside the callback anymore
ie most likely the URL's job is done.

Leo

-- 
Any Emacs contains an ad hoc, informally-specified, bug-ridden, slow
implementation of half of Common Lisp.

Re: [PATCH] Use `url' rather than curl

[Lars Magne Ingebrigtsen]

Leo <sdl.web@gmail.com> writes:

> I think (not sure) that is part of the URL's flow. After it invokes the
> callback provided by the user, it does other things which require the
> process buffer to be around. kill-buffer inside the process's
> filter/sentinel, disrupt its working.

Right.  However, I'm seeing no jumping cursors with Julien's patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: [PATCH] Use `url' rather than curl

[Leo]

On 2010-09-18 20:28 +0100, Lars Magne Ingebrigtsen wrote:
>> I think (not sure) that is part of the URL's flow. After it invokes
>> the callback provided by the user, it does other things which require
>> the process buffer to be around. kill-buffer inside the process's
>> filter/sentinel, disrupt its working.
>
> Right.  However, I'm seeing no jumping cursors with Julien's patch.

Alright. In my experience, it wasn't reliably reproducible making it
difficult to debug. For example, it could depend on the time it takes
to retrieve a page.

I actually had this comment in June in a personal code snippet:

-    ;; xxx: sometimes it moves point
-    ;; (ircpaste-lodgeit-getpaste 215625) usually fails
-    ;; (ircpaste-lodgeit-getpaste 215623) usually works

Referring to the two pastes:

http://paste.pocoo.org/show/215625/
http://paste.pocoo.org/show/215623/

Anyway URL provides a pair url-mark-buffer-as-dead and
url-gc-dead-buffers (called at the beginning of url-retrieve-internal).

Leo

Re: [PATCH] Use `url' rather than curl

[Leo]

On 2010-09-18 19:22 +0100, Julien Danjou wrote:
> +(defun gnus-html-image-fetched (status buffer images)
> +  (let ((spec (pop images)))
> +    (when (and (buffer-live-p buffer)
> +               ;; If the position of the marker is 1, then that
> +               ;; means that the text it was in has been deleted;
> +               ;; i.e., that the user has selected a different
> +               ;; article before the image arrived.
> +               (not (= (marker-position (cadr spec)) (point-min))))
> +      (let ((file (gnus-html-image-id (car spec))))
> +        ;; Search the start of the image data
> +        (search-forward "\n\n")
> +        ;; Write region (image) silently
> +        (write-region (point) (point-max) file nil 1)
> +        (kill-buffer)
> +        (with-current-buffer buffer
> +          (let ((inhibit-read-only t)
> +                (string (buffer-substring (cadr spec) (caddr spec))))
> +            (delete-region (cadr spec) (caddr spec))
> +            (gnus-html-put-image file (cadr spec) string))))
>        (when images
> -	(gnus-html-schedule-image-fetching buffer images)))))
> +        (gnus-html-schedule-image-fetching buffer images)))))
>  
>  (defun gnus-html-put-image (file point string &optional url alt-text)
>    (when (gnus-graphic-display-p)

Another comment. May not be relevant since I am still using Gnus from
emacs-23.

Looks like we are doing a round trip here. Retrieve an image into a
buffer, save it to a file and then have it read back and displayed.
Would it be better in the image spec to use :data with value from
(string-to-unibyte (buffer-substring....)) and :type from
image-type-from-data?

Leo

Re: [PATCH] Use `url' rather than curl

[Lars Magne Ingebrigtsen]

Leo <sdl.web@gmail.com> writes:

> Looks like we are doing a round trip here. Retrieve an image into a
> buffer, save it to a file and then have it read back and displayed.
> Would it be better in the image spec to use :data with value from
> (string-to-unibyte (buffer-substring....)) and :type from
> image-type-from-data?

It's nice to cache the images in a file so that it can be redisplayed
the next time you view the article, I think.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: [PATCH] Use `url' rather than curl

[Leo]

On 2010-09-18 21:21 +0100, Lars Magne Ingebrigtsen wrote:
> It's nice to cache the images in a file so that it can be redisplayed
> the next time you view the article, I think.

Thanks.

Leo

Re: Use `url' rather than curl

[Adam Sjøgren]

On Sat, 18 Sep 2010 20:22:42 +0200, Julien wrote:

> +  (url-retrieve (caar images)
> +                'gnus-html-image-fetched
> +                (list buffer images)))

It looks like url-retrieve is different in (my copy of) XEmacs 21.5
beta:

,----[ C-h f url-retrieve RET ]
| `url-retrieve' is a compiled Lisp function
|   -- loaded from "url"
| (url-retrieve URL &optional NO-CACHE EXPECTED-MD5)
| 
| Documentation:
| Retrieve a document over the World Wide Web.
| The document should be specified by its fully specified
| Uniform Resource Locator.  No parsing is done, just return the
| document as the server sent it.  The document is left in the
| buffer specified by url-working-buffer.  url-working-buffer is killed
| immediately before starting the transfer, so that no buffer-local
| variables interfere with the retrieval.  HTTP/1.0 redirection will
| be honored before this function exits.
`----

edebugging reveals that gnus-html-image-fetched is never called when I
try to see an HTML-article with a picture in it.

Hm. What to do...


  Best regards,

    Adam

-- 
 "It's my chainsaw                                            Adam Sjøgren
  Division is mine"                                      asjo@koldfront.dk

Re: gnus-html, url and curl

[Katsumi Yamaoka]

Julien Danjou wrote:
> `url' has also a `url-cache' module that can be used to do caching.

Please note, as for XEmacs, the Emacs/W3 package[1] provides `url'
and `url-cache' modules.  That's very old (url-cache.el was last
updated at 1999-09-16).  Though the most recent gnus-html.el seems
to work with XEmacs, for good or evil.

[1] ftp://ftp.xemacs.org/xemacs/packages/w3-1.35-pkg.tar.gz

Re: gnus-html, url and curl

[Lars Magne Ingebrigtsen]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> Please note, as for XEmacs, the Emacs/W3 package[1] provides `url'
> and `url-cache' modules.  That's very old (url-cache.el was last
> updated at 1999-09-16).

Hm.  Do you know if there's there any plan to fix this in XEmacs?

> Though the most recent gnus-html.el seems to work with XEmacs, for
> good or evil.

:-)

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: gnus-html, url and curl

[Katsumi Yamaoka]

Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:
> Katsumi Yamaoka <yamaoka@jpl.org> writes:

>> Please note, as for XEmacs, the Emacs/W3 package[1] provides `url'
>> and `url-cache' modules.  That's very old (url-cache.el was last
>> updated at 1999-09-16).

> Hm.  Do you know if there's there any plan to fix this in XEmacs?

I have no idea to synch old url modules to those of Emacs.
Er, we may want to use curl again in the future.

Re: gnus-html, url and curl

[Robert Pluim]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:
>> Katsumi Yamaoka <yamaoka@jpl.org> writes:
>
>>> Please note, as for XEmacs, the Emacs/W3 package[1] provides `url'
>>> and `url-cache' modules.  That's very old (url-cache.el was last
>>> updated at 1999-09-16).
>
>> Hm.  Do you know if there's there any plan to fix this in XEmacs?
>
> I have no idea to synch old url modules to those of Emacs.

It's something that could be done, but will probably have to wait until
XEmacs has moved to GPLv3.  I strongly suspect that 'synch' will in that
case translate to 'rip out old and drop in new'

> Er, we may want to use curl again in the future.

It would be nice to have the option, at least.

Robert

Re: gnus-html, url and curl

[Lars Magne Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> It's something that could be done, but will probably have to wait until
> XEmacs has moved to GPLv3.

Do you know how long that's going to take, by any chance?  That is, what
are the hurdles?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: gnus-html, url and curl

[Adam Sjøgren]

On Thu, 23 Sep 2010 17:57:03 +0200, Lars wrote:

> Do you know how long that's going to take, by any chance?  That is, what
> are the hurdles?

Here is a recent thread on xemacs-beta about that very subject:

 * http://thread.gmane.org/gmane.emacs.xemacs.beta/33503

I wouldn't be surprised if it happens before 2015.


   :-),

    Adam

-- 
 "It's that thin, that wild mercury sound. It's               Adam Sjøgren
  metallic and bright gold, with whatever that conjures  asjo@koldfront.dk
  up. That's my particular sound."

Re: gnus-html, url and curl

[Lars Magne Ingebrigtsen]

asjo@koldfront.dk (Adam Sjøgren) writes:

> I wouldn't be surprised if it happens before 2015.
>
>    :-),

*sigh*

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen