vm-summary-faces-alist format not quite right?

[Tim Cross]

I think this could be a bug, but it could just as well be a misunderstanding on my part. I'm still investigating.

I think there is a mismatch in some of the code in what it iexpects the format of vm-summary-faces-alist to be and what it actually is.

The doc string for the variable and its custom definition say

  "*Alist of virtual folder conditions and corresponding faces.
Order matters. The first matching one will be used as face.

See `vm-virtual-folder-alist' for a description of the conditions."
  :type '(repeat (cons (sexp) (face)))

However, the default definition is not a 'straight' alist i.e.
  '(((or (header "Priority: urgent")
   (header "Importance: high")
   (header "X-Priority: 1")
   (label "!")
   (label "\\flagged")
   (header "X-VM-postponed-data:"))
  vm-summary-high-priority-face)
 ((collapsed) vm-summary-collapsed-face)
 ((marked) vm-summary-marked-face)
 ((deleted) vm-summary-deleted-face)
 ((new) vm-summary-new-face)
 ((unread) vm-summary-unread-face)
 ((replied) vm-summary-replied-face)
 ((or (filed)
   (written)) vm-summary-saved-face)
 ((or (forwarded)
   (redistributed)) vm-summary-forwarded-face)
 ((edited) vm-summary-edited-face)
 ((outgoing) vm-summary-outgoing-face)
 ((expanded) vm-summary-expanded-face)
 ((any) vm-summary-default-face))

Note that this is a quoted list, not evaluated. The problem is that it appears the code treats it as a standard alist, with bits like

(setq prop (completing-read "Face name: "
       (mapcar (lambda (f)
           (list (format "%s" (caar f))))
         vm-summary-faces-alist)
       nil t "deleted")))

The mapcar in the above list will result in

(("or") ("collapsed") ("marked") ("deleted") ("new") ("unread") ("replied")
 ("or") ("or") ("edited") ("outgoing") ("expanded") ...)

Notice the "or" entries.

Later in the function, the value of prop is used to define a face name i.e.

        (face (intern (concat "vm-summary-" prop "-face")))

but 'vm-summary-or-face would not be a valid face name.

The function vm-summary-face-add also has what looks to be incorrect behavior. It has the following

  (let ((faces vm-summary-faces-alist)

and later

    (while faces
      (when (apply 'vm-vs-or msg (list (caar faces)))

with

      (setq faces (cdr faces)))))

to iterate through the list until vm-vs-or returns a true value

The potential set of values vm-vs-or could be called with would be

((or (header Priority: urgent) (header Importance: high) (header X-Priority: 1) (label !) (label \flagged) (header X-VM-postponed-data:)))
((collapsed))
((marked))
((deleted))
((new))
((unread))
((replied))
((or (filed) (written)))
((or (forwarded) (redistributed)))
((edited))
((outgoing))
((expanded))
((any))

Which doesn't look right. I stripped things down a little to try and see what was going on. I defined

(defun tx-vm-vs-or (m &rest selectors)
  (let ((result nil) selector arglist function)
 (while selectors
   (setq selector (car (car selectors))
   function (cdr (assq selector vm-virtual-selector-function-alist)))
   (message (format "Selector %s" (or selector "NULL")))
   (message (format "Function %s" (or function "NULL")))
   (setq arglist (cdr (car selectors)))
   (message (format "Arglist %s" (or arglist "NULL")))
   (setq selectors (cdr selectors)))))

and called it by (let ((faces vm-summary-faces-alist))

  (while faces
 (message "|%s|" (list (caar faces)))
 (tx-vm-vs-or "message data" (list (caar faces)))
 (setq faces (cdr faces))))

and got as a result

|((or (header Priority: urgent) (header Importance: high) (header X-Priority: 1) (label !) (label \flagged) (header X-VM-postponed-data:)))|
Selector (or (header Priority: urgent) (header Importance: high) (header X-Priority: 1) (label !) (label \flagged) (header X-VM-postponed-data:))
Function NULL
Arglist NULL
|((collapsed))|
Selector (collapsed)
Function NULL
Arglist NULL
|((marked))|
Selector (marked)
Function NULL
Arglist NULL
|((deleted))|
Selector (deleted)
Function NULL
Arglist NULL
|((new))|
Selector (new)
Function NULL
Arglist NULL
|((unread))|
Selector (unread)
Function NULL
Arglist NULL
|((replied))|
Selector (replied)
Function NULL
Arglist NULL
|((or (filed) (written)))|
Selector (or (filed) (written))
Function NULL
Arglist NULL
|((or (forwarded) (redistributed)))|
Selector (or (forwarded) (redistributed))
Function NULL
Arglist NULL
|((edited))|
Selector (edited)
Function NULL
Arglist NULL
|((outgoing))|
Selector (outgoing)
Function NULL
Arglist NULL
|((expanded))|
Selector (expanded)
Function NULL
Arglist NULL
|((any))|
Selector (any)
Function NULL
Arglist NULL

Note that all the function values returned from

     function (cdr (assq selector vm-virtual-selector-function-alist)))

appear to return nul for all possible selector values. The problem appears to be that the assq is getting a nested list i.e. ((any)) rather than 'any etc, so always fails to find a match, but I'm guessing here. At any rate, it doesn't seem to be doing what we want/expect.

Note also that there is a redundant bit of code in vm-vs-or i.e.

      (setq arglist (cdr (car selectors))
        arglist (cdr (car selectors))
         result (apply function m arglist)

So, even if the rest of this bug report is totally off the mark, at least it does point out one bit that should be removed!

Note that if desired, I'm happy to look at this, but others who are more familiar may be able to sort it out faster/quicker (assuming it is a real issue that is).

If you want me to look at it, just assign this bug report back to me.

[Tim Cross (tcross)]

I forgot to mention that I suspect changing the vm-summary-face-alist to be a simple alist would probably fix the problems. It would also make the definition of the custom format easier. This would remove the 'or' from the list of possible faces and would (I think) get rid of the nested values so that the assq would match for the search in vm-virtual-selector-function-alist. However, don't know what other impact such a change may have elsewhere.

[Uday Reddy (reddyuday)]

Are you saying that the caar of an alist should be a symbol? That is not strictly necessary. An alist is a list of cons-pairs, with each car being a "key" and cdr being the associated stuff.

Ideally, the car in this case should have been the face name and the cdr should have been the selector. But this is the way Rob F set it up originally. Should we change it?

Don't worry about the use of vm-vs-or. That is a red herring.

[Tim Cross (tcross)]

> Are you saying that the caar of an alist should be a symbol? That is
> not strictly necessary. An alist is a list of cons-pairs, with each car
> being a "key" and cdr being the associated stuff.
>

No. What I meant by 'standard' (probably should have said common or typical)
alist is that the car/key usually has a consistent structure. In this case, the
car is a sexp, but they have different structure. Taking the caar will return
'or' in some cases and 'collapsed', 'marked', 'new' etc in others. Personally,
I try to avoid abstractions like this as they require a deep awareness of the
possible structure of the key in order to use them reliably. When this level of
complexity is justified, I'd usually abstract it a bit further and use
functions to set/query/retrieve values.

This doesn't look quite right in some of the code I looked at. I can see that
returning 'collapsed' or 'marked' etc makes sense as this represents possible
states associated with a message, but what about 'or'. This certainly doesn't
seem correct in at least one bit of the code - in vm-summary-faces-hide, the
caar of each element in the alist is used to generate a list of possible face
names and 'or' will be included as an element in that list (more than once
BTW). It looks like the code assumes a simpler structure in which the caar
would always be a message state/attribute.

> Ideally, the car in this case should have been the face name and the cdr
> should have been the selector. But this is the way Rob F set it up
> originally. Should we change it?
>
Yes, a simpler structure would probably be more maintainable. However, I don't
yet fully understand how this alist is used, so I'm unwilling to just jump in
and go for change. However, as the current defcustom doesn't work and gives a
mismatch warning, I do need to get a handle on this, so I'm willing to look at
it if your OK with that.

What I suspect is that the code we have is some that Rob grew 'organically' as
he explored the problem of adding faces to the summary buffer. Now the basic
framework is there, we can probably simplify things considerably.

> Don't worry about the use of vm-vs-or. That is a red herring.

yes and no. The additional redundant bit of code is irrelevant. However, it
doesn't seem to be doing the right thing. If nothing else, it is trying to
lookup a selector function from vm-virtual-selector-function-alist using memq
where the first argument being passed is a list rather than an element, so it
will always return null as the car/keys of vm-summary-selector-function-alist
are all simple symbols.

Tim

>
> --
> vm-summary-faces-alist format not quite right?
> https://bugs.launchpad.net/bugs/614685
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in VM (View Mail) for Emacs: New
>
> Bug description:
> I think this could be a bug, but it could just as well be a misunderstanding on my part. I'm still investigating.
>
> I think there is a mismatch in some of the code in what it iexpects the format of vm-summary-faces-alist to be and what it actually is.
>
> The doc string for the variable and its custom definition say
>
>
> "*Alist of v...

[Uday Reddy (reddyuday)]

I will change the documentation to clarify what is meant by an alist here.

[Uday Reddy (reddyuday)]

vm-summary-faces-hide should have used cadr instead of caar.

Fixes made in revision 1172. Also edited the docstring.