User format functions - text properies get lost (XEmacs)

User format functions - text properies get lost (XEmacs)

[Paul Moore]

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

With a lot of help from Jesper Harder, I have identified a problem in
Oort Gnus running under XEmacs.

The problem arises when a user format function returns a string with
text properties set. My original example was a
"gnus-user-format-function-f" which returned the sender's name
(basically like %f) but the name is in red if the sender is in my
BBDB.

I set the properties '(gnus-face t face red), but found that the
summary buffer did not show the colour. On investigation, it became
clear that the problem was that gnus-parse-simple-format produces a
form which uses FORMAT to generate the summary line. On FSF Emacs,
format preserves text properties, but in XEmacs, it does not,
returning a string with no properties set.

The attached patch fixes this problem. It's a little rough around the
edges, mainly because I have pretty much no experience in elisp
programming, but it does fix the problem for me.

Is it possible for this workaround to be included, in some form, in
Gnus proper? I don't see any way of fixing the problem in user code
(ie, *without* patching Gnus).

Some things which need to be addressed (and which are basically beyond
me...)

1. The code I added to gnus-parse-simple-format only uses
   gnus-simple-format for XEmacs. This is to avoid FSF Emacs users
   having to take a performance hit from using a Lisp function where
   their built-in format function is fine. Jesper suggested using
     (eval-and-compile
        (unless (featurep 'xemacs)
           (defalias 'gnus-simple-format 'format)))
   instead, but (a) I couldn't decide where to put it, and (b) I
   suspect my solution is simpler. But I'm not sure, and I have no FSF
   Emacs to test it on, so I'd appreciate comments.

2. The code only handles %s and %%. Under XEmacs these are the only
   two format codes which are generated. However, if
   gnus-use-correct-string-widths is false in XEmacs, %Ns codes are
   generated (for numeric N). Adding code to gnus-simple-format to
   cope with this case is possible, but it complicates (and slows
   down) the code. Is it worth it? Alternatively, is there another way
   of achieving the same effect - maybe make Gnus never generate the
   %Ns codes for XEmacs (the gnus-use-correct-string-widths code
   %manages it by trimming the strings before passing them to format,
   %I think - is something similar possible?)

3. The whole thing (using a format replacement coded in lisp) is going
   to give a performance hit. I'm sure my code isn't anywhere near
   optimal - can anyone help in making it faster?

4. Even if optimised, the whole thing is still going to be slower than
   using format directly. And as it only causes problems in a few
   obscure cases, would it be worth making the whole thing optional,
   via a user configuration option? I've not the slightest idea how to
   do something like that, so I'm afraid I can't offer code here, just
   the suggestion (and the suggestion was originally Jesper's in any
   case :-))

I'd appreciate any comments - this is my first attempt at submitting a
patch for Gnus, so I apologise if I've made any obvious blunders, in
coding or in procedure.

Thanks in particular to Jesper, who helped me follow this through
until I understood precisely where the problem lay.

Paul.


[-- Attachment #2: Gnus patch --]
[-- Type: text/plain, Size: 2333 bytes --]

--- gnus-spec.el.orig	Wed Feb 20 00:15:32 2002
+++ gnus-spec.el	Sat Oct 12 16:38:48 2002
@@ -396,6 +396,24 @@
 		     ,(when (not side) '(make-string need ?\ )))
 	   val)))))
 
+(defun gnus-simple-format (str &rest objects)
+  ;; A replacement for FORMAT which only handles %s and %% formats.
+  ;; Xemacs FORMAT does not preserve text properties on the input
+  ;; strings, whereas this version does. Without this function, returning
+  ;; strings with properties set from user-defined spec functions does not
+  ;; work in xemacs. Arguably, this function should just be an alias for
+  ;; format in FSF emacs (where properties are preserved), for speed if
+  ;; nothing else.
+  (with-temp-buffer
+    (insert str)
+    (goto-char (point-min))
+    (while (search-forward "%s" nil t)
+      (let ((obj (car objects)))
+	(replace-match (if (stringp obj) obj (format "%s" obj))
+		       nil t))
+      (setq objects (cdr objects)))
+    (gnus-replace-in-string (buffer-string) "%%" "%")))
+
 (defun gnus-parse-format (format spec-alist &optional insert)
   ;; This function parses the FORMAT string with the help of the
   ;; SPEC-ALIST and returns a list that can be eval'ed to return the
@@ -642,6 +660,19 @@
       ;; A single string spec in the end of the spec.
       ((string-match "\\`\\([^%]+\\)%[sc]\\'" fstring)
        (list (match-string 1 fstring) (car flist)))
+      ;; Nothing but %% and (bare) %s specs.
+      ;; We use a special replacement for format here to work around the
+      ;; fact that XEmacs FORMAT does not preserve text properties,
+      ;; without having to reimplement FORMAT in all its glory.
+      ;; The code above never uses format codes other than %% and %s for
+      ;; XEmacs in any case. (It uses %NNNs if gnus-use-correct-string-widths
+      ;; is not set, but we don't consider that here - it's a non-standard
+      ;; case and complicates the implementation of gnus-simple-format).
+      ;; Only do this for XEmacs, as we don't want FSF Emacs users to suffer
+      ;; an unnecessary performance hit.
+      ((and (featurep 'xemacs)
+            (string-match "\\`\\([^%]\\|%[s%]\\)*\\'" fstring)))
+       (list (cons 'gnus-simple-format (cons fstring (nreverse flist)))))
       ;; A more complex spec.
       (t
        (list (cons 'format (cons fstring (nreverse flist)))))))

Re: User format functions - text properies get lost (XEmacs)

[Kai Großjohann]

Paul Moore <gustav@morpheus.demon.co.uk> writes:

> I'd appreciate any comments - this is my first attempt at submitting a
> patch for Gnus, so I apologise if I've made any obvious blunders, in
> coding or in procedure.

If it works for you, that should be okay.  It's a little suspicious
that it only handles %s and not, say %-20,20s.  But if it's enough
for you, why not.

kai
-- 
~/.signature is: umop ap!sdn    (Frank Nobis)

Re: User format functions - text properies get lost (XEmacs)

[Paul Moore]

Kai.Grossjohann@CS.Uni-Dortmund.DE (Kai Großjohann) writes:

> Paul Moore <gustav@morpheus.demon.co.uk> writes:
>
>> I'd appreciate any comments - this is my first attempt at submitting a
>> patch for Gnus, so I apologise if I've made any obvious blunders, in
>> coding or in procedure.
>
> If it works for you, that should be okay.  It's a little suspicious
> that it only handles %s and not, say %-20,20s.  But if it's enough
> for you, why not.

As far as I can see, Gnus only generates %s (and %%) under xemacs. If
gnus-use-correct-string-widths is nil (which is not the default) then
%20s can be generated, but in that case my code just falls through to
the default case (use format). So the formatting still happens OK,
it's just that text properties get lost in that case.

But is "it works for me" a good enough reason for a patch going into
Gnus? It doesn't feel to me as if this is "complete" - it needs
performance tuning, and it may need making optional (if the
unavoidable performance impact is non-trivial). But I don't know how
to do these things. I don't even know how to measure the performance
:-(

What's the next step for me if I want to get this included in Gnus?
I'm happy to work with someone developing the code, but if it's left
to me, it won't happen.

There's a real (if rare) issue here. There is no general workaround
available in user code - Gnus doesn't have the hooks to allow user
code to interact with this phase of summary buffer generation. My
patch works around the limitation (bug) in XEmacs, but it's not
without cost. Maybe there's a better alternative, which doesn't need
to work this way, but I can't follow the code well enough to see it.

Just to re-clarify the original requirement - I want to make a part of
the summary line have an alternative face in certain conditions
(specifically, make the sender name change face if the sender is in my
BBDB). Writing a user format function which returns a string which has
face (and gnus-face) properties set should work (and does in FSF
Emacs) but it fails to preserve the face in XEmacs.

Paul.

-- 
This signature intentionally left blank

Re: User format functions - text properies get lost (XEmacs)

[Kai Großjohann]

Paul Moore <gustav@morpheus.demon.co.uk> writes:

> What's the next step for me if I want to get this included in Gnus?
> I'm happy to work with someone developing the code, but if it's left
> to me, it won't happen.

Did you sign the copyright papers?  (I can't look on fencepost to
check, so I might ask people many times :-/).

How about you mail me the patch again after the paper signing has
happened?  Then I'll commit it.

kai
-- 
~/.signature is: umop ap!sdn    (Frank Nobis)

Re: User format functions - text properies get lost (XEmacs)

[Paul Moore]

Kai.Grossjohann@CS.Uni-Dortmund.DE (Kai Großjohann) writes:

> Paul Moore <gustav@morpheus.demon.co.uk> writes:
>
>> What's the next step for me if I want to get this included in Gnus?
>> I'm happy to work with someone developing the code, but if it's left
>> to me, it won't happen.
>
> Did you sign the copyright papers?  (I can't look on fencepost to
> check, so I might ask people many times :-/).

How do I do that?

> How about you mail me the patch again after the paper signing has
> happened?  Then I'll commit it.

But I don't want it committed - I'd like someone to review it, and
possibly assist me in improving it.

Can I repeat: *I am not an elisp programmer*. I do not have enough
confidence in this patch to feel that it should be committed. I
submitted it on the assumption that offering code (even code of
limited quality) would be more appreciated than a simple bug
report.

Paul.

-- 
This signature intentionally left blank

Re: User format functions - text properies get lost (XEmacs)

[Kai Großjohann]

Paul Moore <gustav@morpheus.demon.co.uk> writes:

> Kai.Grossjohann@CS.Uni-Dortmund.DE (Kai Großjohann) writes:
>
>> Paul Moore <gustav@morpheus.demon.co.uk> writes:
>>
>>> What's the next step for me if I want to get this included in Gnus?
>>> I'm happy to work with someone developing the code, but if it's left
>>> to me, it won't happen.
>>
>> Did you sign the copyright papers?  (I can't look on fencepost to
>> check, so I might ask people many times :-/).
>
> How do I do that?

It's easy.  I'll send you a form to fill out.  You email it to the
FSF, they send you a piece of paper, you sign it and snail-mail it
back to the FSF.

>> How about you mail me the patch again after the paper signing has
>> happened?  Then I'll commit it.
>
> But I don't want it committed - I'd like someone to review it, and
> possibly assist me in improving it.
>
> Can I repeat: *I am not an elisp programmer*. I do not have enough
> confidence in this patch to feel that it should be committed. I
> submitted it on the assumption that offering code (even code of
> limited quality) would be more appreciated than a simple bug
> report.

Okay, I've now thought long and hard about it, and -- lo! -- I found
a small problem.  There you are :-)  (See my other message from a few
minutes ago about this.)

Maybe I should have explicitly said "the patch seems good" in my
previous message, instead of just implying this when I said I would
commit.  Sorry.

kai
-- 
~/.signature is: umop ap!sdn    (Frank Nobis)

Re: User format functions - text properies get lost (XEmacs)

[Paul Moore]

Kai.Grossjohann@CS.Uni-Dortmund.DE (Kai Großjohann) writes:
> Okay, I've now thought long and hard about it, and -- lo! -- I found
> a small problem.  There you are :-)  (See my other message from a few
> minutes ago about this.)

Phew! I feel better now :-)

> Maybe I should have explicitly said "the patch seems good" in my
> previous message, instead of just implying this when I said I would
> commit.  Sorry.

That's OK. I was basically just sufficiently unsure about my code that
I couldn't believe that it would be OK without any changes...

Thanks,
Paul.

-- 
This signature intentionally left blank

Re: User format functions - text properies get lost (XEmacs)

[Kai Großjohann]

Paul Moore <gustav@morpheus.demon.co.uk> writes:

> +(defun gnus-simple-format (str &rest objects)
> +  ;; A replacement for FORMAT which only handles %s and %% formats.
> +  ;; Xemacs FORMAT does not preserve text properties on the input
> +  ;; strings, whereas this version does. Without this function, returning
> +  ;; strings with properties set from user-defined spec functions does not
> +  ;; work in xemacs. Arguably, this function should just be an alias for
> +  ;; format in FSF emacs (where properties are preserved), for speed if
> +  ;; nothing else.
> +  (with-temp-buffer
> +    (insert str)
> +    (goto-char (point-min))
> +    (while (search-forward "%s" nil t)
> +      (let ((obj (car objects)))
> +	(replace-match (if (stringp obj) obj (format "%s" obj))
> +		       nil t))
> +      (setq objects (cdr objects)))
> +    (gnus-replace-in-string (buffer-string) "%%" "%")))

Two issues come to my mind.  First of all, what happens if you have
"%%s" in the string?  I think that this function is buggy in that
case.  Secondly, converting the string to a buffer and then (for
gnus-replace-in-string) back to a string does not appear to be very
efficient.

Maybe you could search for the regexp "%[%s]", the look at
match-string (that's what actually matched the regex) and do
different things depending on that.  Does this make sense?

kai
-- 
~/.signature is: umop ap!sdn    (Frank Nobis)

Re: User format functions - text properies get lost (XEmacs)

[Paul Moore]

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

Kai.Grossjohann@CS.Uni-Dortmund.DE (Kai Großjohann) writes:

> Two issues come to my mind.  First of all, what happens if you have
> "%%s" in the string?  I think that this function is buggy in that
> case.  Secondly, converting the string to a buffer and then (for
> gnus-replace-in-string) back to a string does not appear to be very
> efficient.

You're right on both counts.

> Maybe you could search for the regexp "%[%s]", the look at
> match-string (that's what actually matched the regex) and do
> different things depending on that.  Does this make sense?

It does. I've updated the patch (attached) to do the following:

1. Adds a customisation variable gnus-make-format-preserve-properties,
   which allows the user to switch the behaviour off if they don't
   need/want it.
2. Rewrote the format replacement function to take into account your
   comments, and added support for pad widths (%10s), as they can
   occur if gnus-use-correct-string-widths is nil. I didn't do
   precision (%.5s) as that should never happen.

Otherwise, it does the same as the previous one.

Cheers,
Paul.

-- 
This signature intentionally left blank


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Revised patch to gnus-spec.el --]
[-- Type: text/x-patch, Size: 2535 bytes --]

--- gnus-spec.el.orig	Fri Oct 11 13:14:39 2002
+++ gnus-spec.el	Thu Oct 17 16:26:23 2002
@@ -35,6 +35,12 @@
   :group 'gnus-format
   :type 'boolean)
 
+(defcustom gnus-make-format-preserve-properties (featurep 'xemacs)
+  "*If non-nil, use a replacement `format' function which preserves
+text properties. This is only needed on XEmacs, as FSF Emacs does this anyway."
+  :group 'gnus-format
+  :type 'boolean)
+
 ;;; Internal variables.
 
 (defvar gnus-summary-mark-positions nil)
@@ -479,6 +485,41 @@
 		      (nth 1 sform)))))
 	 form)))
 
+
+(defun gnus-xmas-format (fstring &rest args)
+  "A version of `format' which preserves text properties.
+
+Required for XEmacs, where the built in `format' function strips all text
+properties from both the format string and any inserted strings.
+
+Only supports the format sequence %s, and %% for inserting
+literal % characters. A pad width and an optional - (to right pad)
+are supported for %s."
+  (let ((re "%%\\|%\\(-\\)?\\([1-9][0-9]*\\)?s")
+	(n (length args)))
+    (with-temp-buffer
+      (insert-string fstring)
+      (goto-char (point-min))
+      (while (re-search-forward re nil t)
+	(goto-char (match-end 0))
+	(cond
+	 ((string= (match-string 0) "%%")
+	  (delete-char -1))
+	 (t
+	  (if (null args)
+	      (error 'wrong-number-of-arguments #'my-format n fstring))
+	  (let* ((minlen (string-to-int (or (match-string 2) "")))
+		 (arg (car args))
+		 (str (if (stringp arg) arg (format "%s" arg)))
+		 (lpad (null (match-string 1)))
+		 (padlen (max 0 (- minlen (length str)))))
+	    (replace-match "")
+	    (if lpad (insert-char ?\  padlen))
+	    (insert str)
+	    (unless lpad (insert-char ?\  padlen))
+	    (setq args (cdr args))))))
+      (buffer-string))))
+
 (defun gnus-parse-simple-format (format spec-alist &optional insert)
   ;; This function parses the FORMAT string with the help of the
   ;; SPEC-ALIST and returns a list that can be eval'ed to return a
@@ -644,6 +685,13 @@
       ;; A single string spec in the end of the spec.
       ((string-match "\\`\\([^%]+\\)%[sc]\\'" fstring)
        (list (match-string 1 fstring) (car flist)))
+      ;; Only string (and %) specs (XEmacs only!)
+      ((and (featurep 'xemacs)
+	    gnus-make-format-preserve-properties
+	    (string-match
+	     "\\`\\([^%]*\\(%%\\|%-?\\([1-9][0-9]*\\)?s\\)\\)*[^%]*\\'"
+	     fstring))
+       (list (cons 'gnus-xmas-format (cons fstring (nreverse flist)))))
       ;; A more complex spec.
       (t
        (list (cons 'format (cons fstring (nreverse flist)))))))

Re: User format functions - text properies get lost (XEmacs)

[Kai Großjohann]

Paul Moore <gustav@morpheus.demon.co.uk> writes:

> It does. I've updated the patch (attached) to do the following:

Committed.  Thanks!

kai
-- 
~/.signature is: umop ap!sdn    (Frank Nobis)

Re: User format functions - text properies get lost (XEmacs)

[Paul Moore]

Kai.Grossjohann@CS.Uni-Dortmund.DE (Kai Großjohann) writes:

> Paul Moore <gustav@morpheus.demon.co.uk> writes:
>
>> It does. I've updated the patch (attached) to do the following:
>
> Committed.  Thanks!

Thank *you*.

-- 
This signature intentionally left blank