vm-summary-selected-face acting strange

[Arik]

vm-summary-selected face is acting in a strange way (after fix to Bug 615008). I usually use a face that is simply inverse-video and in previous versions it properly displayed the inverse of what the message was faced with earlier (for example cyan for new messages would be cyan background black text when selected). Now I get intermittent behavior, sometimes VM seems to select the proper inversed highlight, sometimes it goes to some sort of default maybe and gives me black background white foreground and sometimes it shows a combination of these two on one line.

I set it in my .vm file:
(set-face-attribute 'vm-summary-selected-face nil :inverse-video t :inherit nil)

which has worked in previous vm-versions exactly as I expect. But *most* of the time now, it somehow gets changed to (form a *Customize face* buffer):

((t (:inverse-video t :foreground "black" :background "grey85" :inherit nil)))

where sometimes it is white on black, and sometimes the inverse-video kicks in and it is white on green (or worse, white!) for instance depending on the summary face of the selected message.

[Uday Reddy (reddyuday)]

It is working perfectly for me. (Funny, when I tried it a few weeks
ago, inverse-video didn't work.)

The problem could be that your setting is getting overwritten when
vm-summary-faces is loaded. I will move the definitions to vm-vars
and hope that that will fix it.

[Tim Cross (tcross)]

Not sure if this will help or not, but my ~tcross/vm/custom branch has been modified so that all faces are 'real' defface definitions rather than a variable just holding a face name. For example, vm-summary-highlighted-face. I've set the default in my branch for both vm-summary-highlighted-face and vm-summary-selected face to inverse-video. Both appear to work correctly i.e. inverse video for the current message regardless of the value of vm-summary-enable-faces.

You might want to try out my branch as having the ability to modify face attributes via customize may be a useful way to experiment with various face configurations. My branch is currently in sync with VM trunk.

Note that I've not yet started any testing with xemacs, so there could be issues there.

[Uday Reddy (reddyuday)]

Tim Cross writes:

> Not sure if this will help or not, but my ~tcross/vm/custom branch has
> been modified so that all faces are 'real' defface definitions rather
> than a variable just holding a face name.

I don't think we can do that. Variables and faces are entirely
different beasts in elisp. So, changing one to the other will break
user customization.

[Tim Cross (tcross)]

Uday Reddy writes:
 > Tim Cross writes:
 >
 > > Not sure if this will help or not, but my ~tcross/vm/custom
 > > branch has been modified so that all faces are 'real' defface
 > > definitions rather than a variable just holding a face name.
 >
 > I don't think we can do that. Variables and faces are entirely
 > different beasts in elisp. So, changing one to the other will break
 > user customization.

Yes, it would mean that existing customization would change, but I'd argue this
can be justified. The current setup does not allow a user to customize the face
other than to set it to an already defined face. Making it a propper face
definition will allow much more standard and flexible customization.

Currently, vm-summary-highlight-face is a variable. It has a default of 'bold.
If the end user wants to change that default, they must use an existing face
name. In addition, they need to select a face that is defined when VM is
loaded, which is not always straight-forward as using list-faces-display or the
customizeation groups will list all faces defined at that time, including those
defined by packages which may not always be loaded when VM is started.
Furthermore, they are restricted to those faces i.e. cannot just modify a face
attribute, such as adding underline, italic, a different colour etc.

Leaving the definition as a variable holding a face name also means we now have
inconsistency in how we do things. Some faces are real faces and some faces are
jus variables holding face symbols/names. Some faces will be customizable in a
standard way and some will not. This does not lead to a good user experience.

While I agree with minimizing change and preserving backwards compatibility
where possible, I'd argu that we also need to balance this with improving VM
and making the end-user experience better. We have already made other changes
which are not backwards compatible. The change this causes is small and would
not mean complex or dificult modifications for anyones custom code. If they
have set the variable to a different face name, they would need to remove that
setting and then, if necessary, update the new face definition to meet their
tastes, which is easily done via the custom interface. Secondly, they would
need to modify any code that uses this variable. Generally, this involves
nothing more than quoting the variable to make it a symbol and possibly adding
an explicit test i.e. instead of

 (if vm-summary-highlight-face
   (....

you would do something like

 (if (facep 'vm-summary-highlight-face)
   (...

Of course, we would need to ensure this change is fully documented in the NEWS
file.

Tim

--
Tim Cross
<email address hidden>

There are two types of people in IT - those who do not manage what they
understand and those who do not understand what they manage.

[Uday Reddy (reddyuday)]

> Yes, it would mean that existing customization would change, but I'd
> argue this can be justified. The current setup does not allow a user
> to customize the face other than to set it to an already defined
> face. Making it a propper face definition will allow much more
> standard and flexible customization.

But the user can define a new face and set the variable to that face.
So, I don't see a limitation.

The VM documentation makes a big deal of the fact that VM uses face
variables rather than faces themselves. So, there is likely to be
quite a big hooplah if we make a change like this.

[Uday Reddy (reddyuday)]

Tim Cross writes:

> Currently, vm-summary-highlight-face is a variable. It has a default
> of 'bold. If the end user wants to change that default, they must
> use an existing face name.

I suppose you are more interested in what can be done via the
customize menus. In that case, why don't you define a face called
vm-summary-highlight and set the variable vm-summary-highlight-face to
that face? That way, nothing will break.

[Tim Cross (tcross)]

Uday Reddy writes:
 > > Yes, it would mean that existing customization would change, but
 > > I'd argue this can be justified. The current setup does not allow
 > > a user to customize the face other than to set it to an already
 > > defined face. Making it a propper face definition will allow much
 > > more standard and flexible customization.
 >
 > But the user can define a new face and set the variable to that
 > face. So, I don't see a limitation.
 >

True, but defining a face is not as easy as using the custom interface to
change the attributes of an existing face. Not all VM users are comfortable
with writing elisp and even those that are often have problems getting face
definitions correct.

 > The VM documentation makes a big deal of the fact that VM uses face
 > variables rather than faces themselves. So, there is likely to be
 > quite a big hooplah if we make a change like this.
 >

Except now VM uses both faces and face variables. This has the risk of causing
confusion. I'm already concerned there will be confusion with
vm-summary-highlight-face and vm-summary-selected face. In fact, long-term, we
probably should consider making vm-summary-selected always active (ie. not just
when you have vm-summary-enable-faces set to t and removing
vm-summary-highlight-face as they appear to do the same thing. This wold
simplify/reduce some of the code.

I will send a post to the newsgroup and vm-info list and see how
people feel about the change. Maybe that will provide us with better data to
guage the impact.

Tim

--
Tim Cross
<email address hidden>

There are two types of people in IT - those who do not manage what they
understand and those who do not understand what they manage.

[Tim Cross (tcross)]

Uday Reddy writes:
 > Tim Cross writes:
 >
 > > Currently, vm-summary-highlight-face is a variable. It has a
 > > default of 'bold. If the end user wants to change that default,
 > > they must use an existing face name.
 >
 > I suppose you are more interested in what can be done via the
 > customize menus. In that case, why don't you define a face called
 > vm-summary-highlight and set the variable vm-summary-highlight-face
 > to that face? That way, nothing will break.
 >

Yes, that is a good idea. It is a bit 'messy', but would provide a solution
that doesn't break backwards compatibility.

I do still think long-term we should consider getting rid of either
vm-summary-highlight-face or vm-summary-selected as they appear to do the same
thing. We probably need to make the distinction re: which is releant when clear
in the manual to help minimise the potential for confusion (if not done
already).

BTW, I notice you have removed the -face extension from the face symbols in
vm-summary-faces. Is this the convention we want followed for face symbols?

Tim

--
Tim Cross
<email address hidden>

There are two types of people in IT - those who do not manage what they
understand and those who do not understand what they manage.

[Tim Cross (tcross)]

BTW, I noticed some code like

(if (and selected vm-summary-highlight-face)
 (vm-summary-highlight-region
  (vm-su-start-of m) (point)
    vm-summary-highlight-face))

Given that users may get their config wrong, do you think we should really test
the value of vm-summary-highlight-face to ensure it is a legit face name i.e.

(if (and selected (facep vm-summary-highlight-face))

Tim

--
Tim Cross
<email address hidden>

There are two types of people in IT - those who do not manage what
they understand and those who do not understand what they manage.
--
Tim Cross
<email address hidden>

There are two types of people in IT - those who do not manage what they
understand and those who do not understand what they manage.

[Uday Reddy (reddyuday)]

On 8/15/2010 10:29 AM, Tim Cross wrote:

> Yes, that is a good idea. It is a bit 'messy', but would provide a solution
> that doesn't break backwards compatibility.

I agree that it is messy. That is always a problem with legacy stuff.

> I do still think long-term we should consider getting rid of either
> vm-summary-highlight-face or vm-summary-selected as they appear to do the same
> thing. We probably need to make the distinction re: which is releant when clear
> in the manual to help minimise the potential for confusion (if not done
> already).

Yes, at the moment, the VM manual makes a virtue out of using predefined faces.
  We should first rewrite the manual section encouraging people to
define/modify faces and pushing the face variables under the rug. Eventually,
at some point, the face variables would have lost their point and some users
will start complaining that they are messy. We can remove them then.

After 2003 or so, VM has been extremely stable, if not stagnant, and people
have come to depend on that stability. Only after we are able to recruit
enough new users will we be able to address its idiosyncracies.

[Uday Reddy (reddyuday)]

>
> Given that users may get their config wrong, do you think we should really test
> the value of vm-summary-highlight-face to ensure it is a legit face name i.e.
>
> (if (and selected (facep vm-summary-highlight-face))

Yes, it is a good idea.

Picking up on another issue you asked, faces shouldn't have names ending in
"-face", as prescribed in the elisp manual. Face variables normally do.