Re: Patch: Support for non-ascii characters in imap group names

Re: Patch: Support for non-ascii characters in imap group names

[Ted Zlatanov]

On Sun, 21 Feb 2010 23:15:42 +0100 Martin Stjernholm <mast@lysator.liu.se> wrote: 

MS> [Sent this some time ago but I suspect it got caught in a spam queue, so
MS> I'm trying again. I apologize if it's a duplicate.]

Thank you, it was my fault for ignoring it.

MS> Hello, this patch should fix the long-standing problem with non-ascii
MS> characters in imap group names (see e.g.
MS> http://thread.gmane.org/gmane.emacs.gnus.general/67087 and
MS> http://thread.gmane.org/gmane.emacs.gnus.general/67209).

I know you have Emacs papers.  I don't know if you need Gnus papers as
well (since you can contribute to Gnus through Emacs).  So I'm comitting
the patch but Reiner should comment; I hope it doesn't need to be
reverted.  You can always sign the Gnus papers as well to be certain.

MS> It is done by making the nnimap back end support
MS> `gnus-group-name-charset-group-alist' and
MS> `gnus-group-name-charset-method-alist'. To allow non-ascii groups it's
MS> therefore necessary to configure them to use a sufficiently expressive
MS> coding system, e.g. UTF-8. The default value for
MS> `gnus-group-name-charset-group-alist' accomplishes that.

I agree this is a clumsy fix but don't want to reengineer the groups and
the agent for this.  I do think, however, that this could be a target
for a more ambitious Gnus rewrite that uses the newer Emacs features
added since version 20 or so :)

I'm not sure why you need

	(nnimap-close-group): Call `imap-current-mailbox' instead of using the
	variable `imap-current-mailbox'.

but left it in, assuming it was sensible.

Gnus folk, please test this if you can.  It may break other Gnus pieces
that assume an unencoded group name, but the way it's done seems to be
clean to me.

Thanks
Ted

Re: Patch: Support for non-ascii characters in imap group names

[Martin Stjernholm]

Ted Zlatanov <tzz@lifelogs.com> wrote:

> Thank you, it was my fault for ignoring it.

I'm glad it got through. I trust then that my followup patch to improve
the nnimap startup time has reached relevant people even though I have
yet to see it in list archives.

> I know you have Emacs papers.  I don't know if you need Gnus papers as
> well (since you can contribute to Gnus through Emacs).  So I'm comitting
> the patch but Reiner should comment; I hope it doesn't need to be
> reverted.  You can always sign the Gnus papers as well to be certain.

Indeed, I have signed Emacs papers. I hope that's enough, but if this
turns out to be a problem then please let me know and I'll help with the
necessary paperwork for Gnus as well.

> I'm not sure why you need
>
> 	(nnimap-close-group): Call `imap-current-mailbox' instead of using the
> 	variable `imap-current-mailbox'.
>
> but left it in, assuming it was sensible.

The variable is utf-7 encoded while the function returns the decoded
name, so using the variable would make the name would look garbled in
that message. In general, I tried to use the functions in imap.el that
deal with decoded names, to keep it clean. There are a few cases where
the variable is still used, when only nil/non-nil matters.

Re: Patch: Support for non-ascii characters in imap group names

[Ted Zlatanov]

On Sun, 21 Mar 2010 18:00:55 +0100 Martin Stjernholm <mast@lysator.liu.se> wrote: 

MS> Ted Zlatanov <tzz@lifelogs.com> wrote:
>> Thank you, it was my fault for ignoring it.

MS> I'm glad it got through. I trust then that my followup patch to improve
MS> the nnimap startup time has reached relevant people even though I have
MS> yet to see it in list archives.

I haven't seen it (which is probably my fault as I was not paying
attention), can you please resend and CC me?

>> I'm not sure why you need
>> 
>> (nnimap-close-group): Call `imap-current-mailbox' instead of using the
>> variable `imap-current-mailbox'.
>> 
>> but left it in, assuming it was sensible.

MS> The variable is utf-7 encoded while the function returns the decoded
MS> name, so using the variable would make the name would look garbled in
MS> that message. In general, I tried to use the functions in imap.el that
MS> deal with decoded names, to keep it clean. There are a few cases where
MS> the variable is still used, when only nil/non-nil matters.

Interesting.  It sounds like we should get rid of the variable in code
completely and always use an accessor function with a "decoding"
parameter (so we can get the encoded or the unencoded name).  I don't
like the current hidden behavior.  Do you agree?

Thanks
Ted

Re: Patch: Support for non-ascii characters in imap group names

[Martin Stjernholm]

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

Ted Zlatanov <tzz@lifelogs.com> wrote:

> I haven't seen it (which is probably my fault as I was not paying
> attention), can you please resend and CC me?

Sure, it's attached below. For me it makes the startup time go down from
5 minutes or more to 10-20 seconds when I start up Gnus with my ~140
imap groups over a slow mobile network. The thing is that
nnimap-retrieve-groups is very sensitive to high latency. The patch
doesn't remedy that, but it cuts down the number of groups it polls.

> Interesting.  It sounds like we should get rid of the variable in code
> completely and always use an accessor function with a "decoding"
> parameter (so we can get the encoded or the unencoded name).  I don't
> like the current hidden behavior.  Do you agree?

You're right, using the variable instead in situations where the
encoding isn't relevant is arguably a sort of micro-optimization. It
shouldn't be a significant performance hit to use the function instead.
Or if it is, the right solution would be to optimize in imap.el by
keeping the decoded mailbox name in a variable.

I don't really see the point with adding a "decoding" parameter though.
The utf-7 encoding is just for transport in the imap protocol, and I
can't see any reason why higher code would be interested in that. If
anything, it'd be reasonable to change imap-current-mailbox (and
possibly other variables) to contain decoded mailbox names instead, but
that'd of course have compatibility implications which probably don't
make it worth the hassle.



[-- Attachment #2: Type: message/rfc822, Size: 11145 bytes --]

From: Martin Stjernholm <mast@lysator.liu.se>
To: ding@gnus.org
Subject: Patch: Faster imap folder scanning at startup
Date: Sun, 14 Feb 2010 16:38:39 +0100
Message-ID: <7mhbpj4ykg.fsf@kolon.stjernholm.org>

Hello again. This is a patch that significantly improves the Gnus
startup time when there are many subscribed imap folders. It does not
depend functionally on my previous patch to fix non-ascii imap group
names, but it does require that one to apply cleanly.

I've split it up as a patch sequence with atomic changes: First two
patches to fix some bugs in the uidvalidity handling, then one that
introduces a function to avoid code duplication, and last the real
patch. See detailed descriptions below.


----------------------------------------

(nnimap-verify-uidvalidity): Fixed bug where uidvalidity wasn't updated
after mismatch.

--- a/lisp/nnimap.el
+++ b/lisp/nnimap.el
@@ -548,8 +548,9 @@ If SERVER is nil, uses the current server."
     (if old-uidvalidity
 	(if (not (equal old-uidvalidity new-uidvalidity))
 	    ;; uidvalidity clash
-	    (gnus-delete-file file)
-	  (gnus-group-set-parameter gnusgroup 'uidvalidity new-uidvalidity)
+	    (progn
+	      (gnus-group-set-parameter gnusgroup 'uidvalidity new-uidvalidity)
+	      (gnus-delete-file file))
 	  t)
       (gnus-group-add-parameter gnusgroup (cons 'uidvalidity new-uidvalidity))
       t)))


----------------------------------------

(nnimap-verify-uidvalidity): Clear cached mailbox info correctly when
uidvalidity changes.

--- a/lisp/nnimap.el
+++ b/lisp/nnimap.el
@@ -550,9 +550,14 @@ If SERVER is nil, uses the current server."
 	    ;; uidvalidity clash
 	    (progn
 	      (gnus-group-set-parameter gnusgroup 'uidvalidity new-uidvalidity)
+	      (gnus-sethash (gnus-group-prefixed-name group server)
+			    nil nnimap-mailbox-info)
 	      (gnus-delete-file file))
 	  t)
       (gnus-group-add-parameter gnusgroup (cons 'uidvalidity new-uidvalidity))
+      (gnus-sethash			; Maybe not necessary here.
+       (gnus-group-prefixed-name group server)
+       nil nnimap-mailbox-info)
       t)))
 
 (defun nnimap-before-find-minmax-bugworkaround ()


----------------------------------------

(nnimap-group-prefixed-name): New function to avoid some code duplication.

(nnimap-verify-uidvalidity, nnimap-group-overview-filename,
nnimap-request-group): Use it.

--- a/lisp/nnimap.el
+++ b/lisp/nnimap.el
@@ -505,6 +505,12 @@ See also `nnimap-log'."
   (and group
        (mm-encode-coding-string group (gnus-group-name-charset nil group))))
 
+(defun nnimap-group-prefixed-name (group &optional server)
+  (gnus-group-prefixed-name group
+			    (gnus-server-to-method
+			     (format "nnimap:%s"
+				     (or server nnimap-current-server)))))
+
 (defsubst nnimap-get-server-buffer (server)
   "Return buffer for SERVER, if nil use current server."
   (cadr (assoc (or server nnimap-current-server) nnimap-server-buffer-alist)))
@@ -525,9 +531,7 @@ If SERVER is nil, uses the current server."
 
 (defun nnimap-verify-uidvalidity (group server)
   "Verify stored uidvalidity match current one in GROUP on SERVER."
-  (let* ((gnusgroup (gnus-group-prefixed-name
-		     group (gnus-server-to-method
-			    (format "nnimap:%s" server))))
+  (let* ((gnusgroup (nnimap-group-prefixed-name group server))
 	 (new-uidvalidity (imap-mailbox-get 'uidvalidity))
 	 (old-uidvalidity (gnus-group-get-parameter gnusgroup 'uidvalidity))
 	 (dir (file-name-as-directory (expand-file-name nnimap-directory)))
@@ -678,9 +682,7 @@ If EXAMINE is non-nil the group is selected read-only."
   "Make file name for GROUP on SERVER."
   (let* ((dir (file-name-as-directory (expand-file-name nnimap-directory)))
 	 (uidvalidity (gnus-group-get-parameter
-		       (gnus-group-prefixed-name
-			group (gnus-server-to-method
-			       (format "nnimap:%s" server)))
+		       (nnimap-group-prefixed-name group server)
 		       'uidvalidity))
 	 (name (nnheader-translate-file-chars
 		(concat nnimap-nov-file-name
@@ -1034,8 +1036,7 @@ function is generally only called when Gnus is shutting down."
 (deffoo nnimap-request-group (group &optional server fast)
   (nnimap-request-update-info-internal
    group
-   (gnus-get-info (gnus-group-prefixed-name
-		   group (gnus-server-to-method (format "nnimap:%s" server))))
+   (gnus-get-info (nnimap-group-prefixed-name group server))
    server)
   (when (nnimap-possibly-change-group group server)
     (nnimap-before-find-minmax-bugworkaround)


----------------------------------------

(nnimap-retrieve-groups, nnimap-verify-uidvalidity, nnimap-update-unseen):
Significantly improved speed of Gnus startup with many imap folders.

This is done by caching the group status from the imap server
persistently in a group parameter `imap-status'. (This was cached
before too if `nnimap-retrieve-groups-asynchronous' was set, but not
persistently, so every Gnus startup was still very slow.)

--- a/lisp/nnimap.el
+++ b/lisp/nnimap.el
@@ -554,11 +554,13 @@ If SERVER is nil, uses the current server."
 	    ;; uidvalidity clash
 	    (progn
 	      (gnus-group-set-parameter gnusgroup 'uidvalidity new-uidvalidity)
+	      (gnus-group-remove-parameter gnusgroup 'imap-status)
 	      (gnus-sethash (gnus-group-prefixed-name group server)
 			    nil nnimap-mailbox-info)
 	      (gnus-delete-file file))
 	  t)
       (gnus-group-add-parameter gnusgroup (cons 'uidvalidity new-uidvalidity))
+      (gnus-group-remove-parameter gnusgroup 'imap-status)
       (gnus-sethash			; Maybe not necessary here.
        (gnus-group-prefixed-name group server)
        nil nnimap-mailbox-info)
@@ -1060,8 +1062,7 @@ function is generally only called when Gnus is shutting down."
 				 nnimap-mailbox-info)))
      (list (nth 0 old) (nth 1 old)
 	   (imap-mailbox-status (nnimap-decode-group-name group)
-				'unseen nnimap-server-buffer)
-	   (nth 3 old)))
+				'unseen nnimap-server-buffer)))
    nnimap-mailbox-info))
 
 (defun nnimap-close-group (group &optional server)
@@ -1167,9 +1168,9 @@ function is generally only called when Gnus is shutting down."
 	    (setq decoded-group (nnimap-decode-group-name group))
 	    (gnus-message 9 "nnimap: Quickly checking mailbox %s"
 			  decoded-group)
-	    (add-to-list (if (gnus-gethash-safe
-			      (gnus-group-prefixed-name group server)
-			      nnimap-mailbox-info)
+	    (add-to-list (if (gnus-group-get-parameter
+			      (nnimap-group-prefixed-name group)
+			      'imap-status)
 			     'asyncgroups
 			   'slowgroups)
 			 (list group (imap-mailbox-status-asynch
@@ -1177,61 +1178,71 @@ function is generally only called when Gnus is shutting down."
 				      '(uidvalidity uidnext unseen)
 				      nnimap-server-buffer))))
 	  (dolist (asyncgroup asyncgroups)
-	    (let ((group (nth 0 asyncgroup))
-		  (tag   (nth 1 asyncgroup))
-		  new old)
+	    (let* ((group (nth 0 asyncgroup))
+		   (tag   (nth 1 asyncgroup))
+		   (gnusgroup (nnimap-group-prefixed-name group))
+		   (saved-uidvalidity (gnus-group-get-parameter gnusgroup
+								'uidvalidity))
+		   (saved-imap-status (gnus-group-get-parameter gnusgroup
+								'imap-status))
+		   (saved-info (and saved-imap-status
+				    (split-string saved-imap-status " "))))
 	      (setq decoded-group (nnimap-decode-group-name group))
 	      (when (imap-ok-p (imap-wait-for-tag tag nnimap-server-buffer))
-		(if (or (not (string=
-			      (nth 0 (gnus-gethash (gnus-group-prefixed-name
-						    group server)
-						   nnimap-mailbox-info))
+		(if (or (not (equal
+			      saved-uidvalidity
 			      (imap-mailbox-get 'uidvalidity decoded-group
 						nnimap-server-buffer)))
-			(not (string=
-			      (nth 1 (gnus-gethash (gnus-group-prefixed-name
-						    group server)
-						   nnimap-mailbox-info))
+			(not (equal
+			      (nth 0 saved-info)
 			      (imap-mailbox-get 'uidnext decoded-group
 						nnimap-server-buffer))))
 		    (push (list group) slowgroups)
-		  (insert (nth 3 (gnus-gethash (gnus-group-prefixed-name
-						group server)
-					       nnimap-mailbox-info))))))))
+		  (gnus-sethash
+		   (gnus-group-prefixed-name group server)
+		   (list (imap-mailbox-get 'uidvalidity
+					   decoded-group nnimap-server-buffer)
+			 (imap-mailbox-get 'uidnext
+					   decoded-group nnimap-server-buffer)
+			 (imap-mailbox-get 'unseen
+					   decoded-group nnimap-server-buffer))
+		   nnimap-mailbox-info)
+		  (insert (format "\"%s\" %s %s y\n" group
+				  (nth 2 saved-info)
+				  (nth 1 saved-info))))))))
 	(dolist (group slowgroups)
 	  (if nnimap-retrieve-groups-asynchronous
 	      (setq group (car group)))
 	  (setq decoded-group (nnimap-decode-group-name group))
 	  (gnus-message 7 "nnimap: Mailbox %s modified" decoded-group)
-	  (imap-mailbox-put 'uidnext nil decoded-group nnimap-server-buffer)
 	  (or (member "\\NoSelect" (imap-mailbox-get 'list-flags decoded-group
 						     nnimap-server-buffer))
-	      (let* ((info (nnimap-find-minmax-uid group 'examine))
-		     (str (format "\"%s\" %d %d y\n" group
-				  (or (nth 2 info) 0)
-				  (max 1 (or (nth 1 info) 1)))))
+	      (let* ((gnusgroup (nnimap-group-prefixed-name group))
+		     (status (imap-mailbox-status
+			      decoded-group '(uidvalidity uidnext unseen)
+			      nnimap-server-buffer))
+		     (info (nnimap-find-minmax-uid group 'examine))
+		     (min-uid (max 1 (or (nth 1 info) 1)))
+		     (max-uid (or (nth 2 info) 0)))
 		(when (> (or (imap-mailbox-get 'recent decoded-group
 					       nnimap-server-buffer) 0)
 			 0)
 		  (push (list (cons decoded-group 0)) nnmail-split-history))
-		(insert str)
-		(when nnimap-retrieve-groups-asynchronous
-		  (gnus-sethash
-		   (gnus-group-prefixed-name group server)
-		   (list (or (imap-mailbox-get
-			      'uidvalidity decoded-group nnimap-server-buffer)
-			     (imap-mailbox-status
-			      decoded-group 'uidvalidity nnimap-server-buffer))
-			 (or (imap-mailbox-get
-			      'uidnext decoded-group nnimap-server-buffer)
-			     (imap-mailbox-status
-			      decoded-group 'uidnext nnimap-server-buffer))
-			 (or (imap-mailbox-get
-			      'unseen decoded-group nnimap-server-buffer)
-			     (imap-mailbox-status
-			      decoded-group 'unseen nnimap-server-buffer))
-			 str)
-		   nnimap-mailbox-info)))))))
+		(insert (format "\"%s\" %d %d y\n" group max-uid min-uid))
+		(gnus-sethash
+		 (gnus-group-prefixed-name group server)
+		 status
+		 nnimap-mailbox-info)
+		(if (not (equal (nth 0 status)
+				(gnus-group-get-parameter gnusgroup
+							  'uidvalidity)))
+		    (nnimap-verify-uidvalidity group nnimap-current-server))
+		;; The imap-status parameter is a string on the form
+		;; "<uidnext> <min-uid> <max-uid>".
+		(gnus-group-add-parameter
+		 gnusgroup
+		 (cons 'imap-status
+		       (format "%s %s %s" (nth 1 status) min-uid max-uid))))))))
     (gnus-message 5 "nnimap: Checking mailboxes...done")
     'active))
 

Re: Patch: Support for non-ascii characters in imap group names

[Ted Zlatanov]

On Sun, 21 Mar 2010 21:26:12 +0100 Martin Stjernholm <mast@lysator.liu.se> wrote: 

MS> Ted Zlatanov <tzz@lifelogs.com> wrote:
>> I haven't seen it (which is probably my fault as I was not paying
>> attention), can you please resend and CC me?

MS> Sure, it's attached below. For me it makes the startup time go down from
MS> 5 minutes or more to 10-20 seconds when I start up Gnus with my ~140
MS> imap groups over a slow mobile network. The thing is that
MS> nnimap-retrieve-groups is very sensitive to high latency. The patch
MS> doesn't remedy that, but it cuts down the number of groups it polls.

The patch looks good and did not break anything for me so I added it.
We're still in CVS land so I didn't commit your changes individually but
as one big patch to avoid 3 separate commits.

>> Interesting.  It sounds like we should get rid of the variable in code
>> completely and always use an accessor function with a "decoding"
>> parameter (so we can get the encoded or the unencoded name).  I don't
>> like the current hidden behavior.  Do you agree?

MS> You're right, using the variable instead in situations where the
MS> encoding isn't relevant is arguably a sort of micro-optimization. It
MS> shouldn't be a significant performance hit to use the function instead.
MS> Or if it is, the right solution would be to optimize in imap.el by
MS> keeping the decoded mailbox name in a variable.

MS> I don't really see the point with adding a "decoding" parameter though.
MS> The utf-7 encoding is just for transport in the imap protocol, and I
MS> can't see any reason why higher code would be interested in that. If
MS> anything, it'd be reasonable to change imap-current-mailbox (and
MS> possibly other variables) to contain decoded mailbox names instead, but
MS> that'd of course have compatibility implications which probably don't
MS> make it worth the hassle.

Only nnimap.el and imap.el use it directly so I think it's OK to change
it.  mail-source.el and nnir.el also may need a slight adjustment for
their use of imap-mailbox-*.  It would simplify the code a little and
make the variable more sensible IMO (but if anyone else disagrees,
please speak up).  If you or anyone else is interested in doing it as
you propose, keeping the decoded variable and encoding it on the fly, go
ahead.

Ted

Re: Patch: Support for non-ascii characters in imap group names

[Reiner Steib]

On Sun, Mar 21 2010, Martin Stjernholm wrote:

> Ted Zlatanov <tzz@lifelogs.com> wrote:
> I'm glad it got through. I trust then that my followup patch to improve
> the nnimap startup time has reached relevant people even though I have
> yet to see it in list archives.
>
>> I know you have Emacs papers.  I don't know if you need Gnus papers as
>> well (since you can contribute to Gnus through Emacs).  So I'm comitting
>> the patch but Reiner should comment; I hope it doesn't need to be
>> reverted.  You can always sign the Gnus papers as well to be certain.
>
> Indeed, I have signed Emacs papers.

Papers for Emacs cover contributions to Gnus as well.  Thanks.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/