Propertizing the minor-mode-alist

Propertizing the minor-mode-alist

[James Clark]

In the current CVS emacs, the default mode-line-format propertizes
minor-mode-alist like this:

     `(:propertize ("" minor-mode-alist)
		   help-echo "mouse-2: help for minor modes, mouse-3: minor mode menu"
		   local-map ,mode-line-minor-mode-keymap)

This causes a problem in nxml-mode.  One of the features of nxml-mode is
that it optionally does continuous background validation. The part of
nxml-mode that does this is implemented as minor-mode and it displays
the current validation status by adding an element using :eval to
minor-mode-alist.  When the status is invalid, it returns a propertized
string with local-map and help-echo properties, which allows the user to
click on the word Invalid in the mode-line and go to the first error
detected in the document.  This worked fine in 21.3 but no longer works
in the CVS version presumably because the properties nxml-mode that adds
are overridden by the :propertize in minor-mode-alist.

Perhaps there could be a new keyword :propertize-default that changes
the text properties of only those characters in the string that do not
already have a value for any of the specified properties. Or maybe this
should be the behavior of :propertize.   If you tell me how you would
like to fix this, I will be happy to have a go at implementing it.

James
-- 
To send me mail, replace auth-only by public in the from address. 

Re: Propertizing the minor-mode-alist

[Stefan]

> Perhaps there could be a new keyword :propertize-default that changes
> the text properties of only those characters in the string that do not
> already have a value for any of the specified properties.  Or maybe this
> should be the behavior of :propertize.

I think it makes sense to change the behavior of `:propertize' here.


        Stefan

Re: Propertizing the minor-mode-alist

[James Clark]

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

On Thu, 2004-09-16 at 21:00, Stefan wrote:
> > Perhaps there could be a new keyword :propertize-default that changes
> > the text properties of only those characters in the string that do not
> > already have a value for any of the specified properties.  Or maybe this
> > should be the behavior of :propertize.
> 
> I think it makes sense to change the behavior of `:propertize' here.

It turns out that the existing code already tries to merge the existing
properties of the string with the properties specified in :propertize,
but it doesn't work in this case for two reasons:

- the code gets the existing properties just by looking at the first
character of the string; in my case the first character is a space which
doesn't have any properties, i.e. my code does

	 (concat " "
		 (propertize "Invalid"
			     'help-echo "mouse-1: go to first error"
			     'local-map (make-mode-line-mouse-map
					 'mouse-1
					 'rng-mouse-first-error)))

- the code gives the new properties priority over the existing
properties

Attached is a patch that 

- when the first character has no properties, gets the properties from
the last character;

- gives priority to the existing properties over the new properties.

James
-- 
To send me mail, replace auth-only by public in the from address. 

[-- Attachment #2: mode-line.patch --]
[-- Type: text/x-patch, Size: 1428 bytes --]

--- src/xdisp.c.~1.908.~	2004-09-16 02:12:32.000000000 +0700
+++ src/xdisp.c	2004-09-17 09:17:14.626506584 +0700
@@ -15335,23 +15335,34 @@
 	    Lisp_Object oprops, aelt;
 	    oprops = Ftext_properties_at (make_number (0), elt);
 
+	    /* If the first character doesn't have any properties, try
+	       the last character instead, since user code might not
+	       propertize a leading space character. */
+	       
+	    if (NILP (oprops))
+	      {
+		int length;
+
+		length = XFASTINT (Flength (elt));
+		if (length > 0)
+		  oprops = Ftext_properties_at (make_number (length - 1), elt);
+	      }
+	      
+
 	    if (NILP (Fequal (props, oprops)) || risky)
 	      {
 		/* If the starting string has properties,
-		   merge the specified ones onto the existing ones.  */
+		   merge the specified ones onto the existing ones.
+		   Give priority to the existing one. */
 		if (! NILP (oprops) && !risky)
 		  {
-		    Lisp_Object tem;
-
-		    oprops = Fcopy_sequence (oprops);
-		    tem = props;
-		    while (CONSP (tem))
+		    props = Fcopy_sequence (props);
+		    while (CONSP (oprops))
 		      {
-			oprops = Fplist_put (oprops, XCAR (tem),
-					     XCAR (XCDR (tem)));
-			tem = XCDR (XCDR (tem));
+			props = Fplist_put (props, XCAR (oprops),
+					    XCAR (XCDR (oprops)));
+			oprops = XCDR (XCDR (oprops));
 		      }
-		    props = oprops;
 		  }
 
 		aelt = Fassoc (elt, mode_line_proptrans_alist);

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Propertizing the minor-mode-alist

[Stefan]

> Attached is a patch that

> - when the first character has no properties, gets the properties from
> the last character;

> - gives priority to the existing properties over the new properties.

Looks good to me.
At some point we may want to implement something like
font-lock-fillin-text-property (so it doesn't just look at the first or
last char but also works if there are various properties on various parts
of the string), but as a quick fi, it sounds right.  Can someone install it?


        Stefan

Re: Propertizing the minor-mode-alist

[Kim F. Storm]

Hi James,

I see you have signed legal papers for specific contributions to emacs,
but I cannot find a general assignment that covers past and future changes.

Combined with your other tiny patches that are already installed in
emacs, I think we need such legal papers before we can install further
patches from you.

But I may be mistaken -- Richard?


James Clark <jjc@auth-only.jclark.com> writes:

> On Thu, 2004-09-16 at 21:00, Stefan wrote:
>> > Perhaps there could be a new keyword :propertize-default that changes
>> > the text properties of only those characters in the string that do not
>> > already have a value for any of the specified properties.  Or maybe this
>> > should be the behavior of :propertize.
>> 
>> I think it makes sense to change the behavior of `:propertize' here.
>
> It turns out that the existing code already tries to merge the existing
> properties of the string with the properties specified in :propertize,
> but it doesn't work in this case for two reasons:
>
> - the code gets the existing properties just by looking at the first
> character of the string; in my case the first character is a space which
> doesn't have any properties, i.e. my code does
>
> 	 (concat " "
> 		 (propertize "Invalid"
> 			     'help-echo "mouse-1: go to first error"
> 			     'local-map (make-mode-line-mouse-map
> 					 'mouse-1
> 					 'rng-mouse-first-error)))
>
> - the code gives the new properties priority over the existing
> properties
>
> Attached is a patch that 
>
> - when the first character has no properties, gets the properties from
> the last character;
>
> - gives priority to the existing properties over the new properties.
>
> James
> -- 
> To send me mail, replace auth-only by public in the from address. 
>
> --- src/xdisp.c.~1.908.~	2004-09-16 02:12:32.000000000 +0700
> +++ src/xdisp.c	2004-09-17 09:17:14.626506584 +0700
> @@ -15335,23 +15335,34 @@
>  	    Lisp_Object oprops, aelt;
>  	    oprops = Ftext_properties_at (make_number (0), elt);
>  
> +	    /* If the first character doesn't have any properties, try
> +	       the last character instead, since user code might not
> +	       propertize a leading space character. */
> +	       
> +	    if (NILP (oprops))
> +	      {
> +		int length;
> +
> +		length = XFASTINT (Flength (elt));
> +		if (length > 0)
> +		  oprops = Ftext_properties_at (make_number (length - 1), elt);
> +	      }
> +	      
> +
>  	    if (NILP (Fequal (props, oprops)) || risky)
>  	      {
>  		/* If the starting string has properties,
> -		   merge the specified ones onto the existing ones.  */
> +		   merge the specified ones onto the existing ones.
> +		   Give priority to the existing one. */
>  		if (! NILP (oprops) && !risky)
>  		  {
> -		    Lisp_Object tem;
> -
> -		    oprops = Fcopy_sequence (oprops);
> -		    tem = props;
> -		    while (CONSP (tem))
> +		    props = Fcopy_sequence (props);
> +		    while (CONSP (oprops))
>  		      {
> -			oprops = Fplist_put (oprops, XCAR (tem),
> -					     XCAR (XCDR (tem)));
> -			tem = XCDR (XCDR (tem));
> +			props = Fplist_put (props, XCAR (oprops),
> +					    XCAR (XCDR (oprops)));
> +			oprops = XCDR (XCDR (oprops));
>  		      }
> -		    props = oprops;
>  		  }
>  
>  		aelt = Fassoc (elt, mode_line_proptrans_alist);
> _______________________________________________
> Emacs-devel mailing list
> Emacs-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/emacs-devel

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Propertizing the minor-mode-alist

[Richard Stallman]

    - the code gives the new properties priority over the existing
    properties

That is the intended behavior of the feature; I don't want to change
that.

    - when the first character has no properties, gets the properties from
    the last character;

That change is ok.  To make it really "right", it should set the specified
properties for the whole contents of the string, leaving other properties
unchanged.  This might actually simplify the code, but one needs to be
careful to get the right results when risky != 0.

Re: Propertizing the minor-mode-alist

[James Clark]

On Sat, 2004-09-18 at 06:23, Richard Stallman wrote:
>     - the code gives the new properties priority over the existing
>     properties
> 
> That is the intended behavior of the feature; I don't want to change
> that.

How would you suggest fixing the original problem (of a minor mode
wanting to make the string that it puts in the mode line clickable in a
way specific to that minor mode)? How about either

- another keyword (say :propertize-default) which gives the new
properties priority over the existing properties

- a way for the :propertize keyword to specify the priority of each new
property relative to the exisiting properties

?  The only other alternative I can think of is for mode-line-format not
to simply apply :propertize to minor-mode-alist, but instead to use
:eval and apply :propertize only to those minor modes that specify a
string without properties.

>     - when the first character has no properties, gets the properties from
>     the last character;
> 
> That change is ok.  To make it really "right", it should set the specified
> properties for the whole contents of the string, leaving other properties
> unchanged. 

I agree that would be better.

James
-- 
To send me mail, replace auth-only by public in the from address. 

Re: Propertizing the minor-mode-alist

[James Clark]

On Sat, 2004-09-18 at 06:23, Richard Stallman wrote:
>     - the code gives the new properties priority over the existing
>     properties
> 
> That is the intended behavior of the feature; I don't want to change
> that.

I haven't been able to think of a case where this behavior would be
useful.  Do you have something in mind?  It seems to be that, in the
context of the mode-line, the other way round would be more useful: if
the user has gone to the trouble of putting local-map or help-echo
properties on a string destined for the mode-line, then they won't want
those properties overridden by some containing :propertize element. If
you think of :propertize as being a declarative description of the
desired properties rather than as a function call, then given something
like

`(:propertize (propertize: ,(propertize s p1) ,p2) ,p3)

it's more natural and useful for p1 to have priority over p2 which
should in turn have priority over p3.

James

Re: Propertizing the minor-mode-alist

[Richard Stallman]

    Combined with your other tiny patches that are already installed in
    emacs, I think we need such legal papers before we can install further
    patches from you.

    But I may be mistaken -- Richard?

I think you are right.  However, as I've said, I would not want to
install this patch as given.  It changes a feature I'm not convinced
we should change.

Re: Propertizing the minor-mode-alist

[Stefan]

> `(:propertize (propertize: ,(propertize s p1) ,p2) ,p3)

Agreed,


        Stefan

Re: Propertizing the minor-mode-alist

[Richard Stallman]

    How would you suggest fixing the original problem (of a minor mode
    wanting to make the string that it puts in the mode line clickable in a
    way specific to that minor mode)?

I have somewhat of a bad feeling about this interface as a way of
doing things.  Users would not expect such an interface or look for
it, so the mode ought to provide something else.  What is that
other interface?

    - another keyword (say :propertize-default) which gives the new
    properties priority over the existing properties

:propertize gives its new priority over the existing properties
in the strings.  I think you mean giving the existing properties
priority.

Such a feature could certainly be implemented and made safe.
One must not simply copy and tweak the code for :propertize,
because the handling of risky needs to be thought about.
However, I tend to think that the right solution is to use
an interface that is more typical of Emacs features.

Re: Propertizing the minor-mode-alist

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

> Such a feature could certainly be implemented and made safe.
> One must not simply copy and tweak the code for :propertize,
> because the handling of risky needs to be thought about.
> However, I tend to think that the right solution is to use
> an interface that is more typical of Emacs features.

One could specially recognize the :priority property that is used for
overlay disambiguation.  This would have the disadvantage that one
could not use :propertize to lower the priority.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Propertizing the minor-mode-alist

[James Clark]

On Sun, 2004-09-19 at 05:55, Richard Stallman wrote:
>     How would you suggest fixing the original problem (of a minor mode
>     wanting to make the string that it puts in the mode line clickable in a
>     way specific to that minor mode)?
> 
> I have somewhat of a bad feeling about this interface as a way of
> doing things.  Users would not expect such an interface or look for
> it, so the mode ought to provide something else.  What is that
> other interface?

In this case, the command that is invoked by the mode-line click is also
available on a key and a menu.  I agree that clicking on the mode-line
shouldn't be the only way to invoke a particular function.

On the other hand, it's normal Emacs behavior that clicking on a word on
the mode-line does something useful, somehow related to that word.  In
this case, the word on the mode-line is "Invalid". (It changes from
"Valid" when the background validation discovers an error; it also says
e.g. "Validating:42%" when it's validated 42% of the buffer.)

The implementation of nxml-mode is split into a number of independent
parts to maximize the possibility of reuse in the future by other modes,
but things are set up so that a normal end-user doesn't need to know
about this.  Although the word "Invalid" comes from a part of nxml-mode
that, as it happens, is implemented as a minor mode, this is not
something that the average user would be aware of. In 21.3, if the user
mouses over "Invalid", they will see "mouse-1: go to first error".  With
the current CVS emacs, they will see "mouse-2: help for minor modes,
mouse-3: minor mode menu".  This doesn't seem an improvement to me. One
can argue about exactly what clicking on "Invalid" should do, but surely
it should be something that is related to the invalid status of the
buffer.

I was thinking I could avoid this whole issue by getting Invalid into
the mode-line using mode-line-process instead of minor-mode-alist, since
default-mode-line-format doesn't propertize mode-line-process.

>     - another keyword (say :propertize-default) which gives the new
>     properties priority over the existing properties
> 
> :propertize gives its new priority over the existing properties
> in the strings.  I think you mean giving the existing properties
> priority.

Yes, sorry, you're right.

> Such a feature could certainly be implemented and made safe.
> One must not simply copy and tweak the code for :propertize,
> because the handling of risky needs to be thought about.

The comment in the code says:

 If RISKY is nonzero, remove (disregard) any properties in any string
 we encounter, and ignore :eval and :propertize.

Obviously, :propertize-default would also be ignored, when RISKY is
non-zero.  Is there anything more to it than that?

Also, it one's going to do this, the code should probably be fixed at
the same time to do the right thing with existing properties that apply
to only a substring.

> However, I tend to think that the right solution is to use
> an interface that is more typical of Emacs features.

I think we can distinguish two issues here:

- Is something like :propertize-default a useful feature, which you
would be willing to install if somebody implemented it cleanly?

- What should clicking on "Invalid" in the mode-line in nxml-mode do?

James

Re: Propertizing the minor-mode-alist

[Richard Stallman]

    On the other hand, it's normal Emacs behavior that clicking on a word on
    the mode-line does something useful, somehow related to that word.

What it does is toggle the mode in question.  Moving point is
something very different.  That's the sort of command that is normally
found in the menu bar, never in the mode line, and certainly not among
the minor modes.

If this interface is a great practical convenience, maybe it is
important enough to justify the inconsistency.  But I'd expect that it
is not a great practical convenience, because the keystroke is more
efficient--isn't that so?

Is there a practical importance for this click interface,
other than that it was possible to implement?

Re: Propertizing the minor-mode-alist

[James Clark]

On Mon, 2004-09-20 at 07:05, Richard Stallman wrote:
>     On the other hand, it's normal Emacs behavior that clicking on a word on
>     the mode-line does something useful, somehow related to that word.
> 
> What it does is toggle the mode in question.

Clicking different words on the mode-line does a variety of different
things:

- mouse-1/mouse-3 on the buffer name goes to the previous/next buffer
- mouse-2 on the major mode name pops up a buffer with help on the major
mode
- mouse-2 on a minor mode gives help on that minor mode; mouse-3 pops up
a menu allowing individual mode
- mouse-2 on Narrow widens

> Moving point is
> something very different.  That's the sort of command that is normally
> found in the menu bar, never in the mode line, and certainly not among
> the minor modes.
> 
> If this interface is a great practical convenience, maybe it is
> important enough to justify the inconsistency.  But I'd expect that it
> is not a great practical convenience, because the keystroke is more
> efficient--isn't that so?

The fact that validation is handled as a separate minor mode is not
something a typical user is aware of; they perceive it as a feature of
the major mode. The default behavior of a minor mode menu is not really
appropriate in this case.  But that's my fault for getting the
invalidity indicator into the mode-line via minor-mode-alist; so I've
changed to a different approach.

I misspoke earlier: there's a keystroke for goto to next error, but not
for goto first error.  Obviously I could add one, but when you have a
complex major mode, it's hard to find mnemonic key bindings for all the
commands, and it's hard for the user to remember all of them (at least I
find it hard to remember them). I find the click interface convenient
because it's more efficient than the menu bar, but easier to remember
that a keystroke interface would be.

However, I take your point about moving point being a strange thing for
a mode-line click to do.  The underlying idea is that clicking on the
invalidity indicator ought to give information about the invalidity.
Moving point to the location  of the first error was an easy way to do
this, but perhaps a better way would be to pop up a buffer giving a
summary of all the errors in the buffer.  Do you think that would be
better?

James

Re: Propertizing the minor-mode-alist

[Kai Grossjohann]

James Clark <jjc@auth-only.jclark.com> writes:

> I misspoke earlier: there's a keystroke for goto to next error, but not
> for goto first error.  Obviously I could add one, but when you have a
> complex major mode, it's hard to find mnemonic key bindings for all the
> commands, and it's hard for the user to remember all of them (at least I
> find it hard to remember them). I find the click interface convenient
> because it's more efficient than the menu bar, but easier to remember
> that a keystroke interface would be.

nxml-mode could integrate with the next-error framework that has
recently been added to CVS.  That would make M-x first-error RET and
M-x next-error RET work.

It would also be cool if it could populate a *compilation*-like buffer
with the error messages -- it is convenient to navigate between errors
in compilation buffers.

Kai

Re: Propertizing the minor-mode-alist

[Richard Stallman]

    I misspoke earlier: there's a keystroke for goto to next error, but not
    for goto first error.

C-u C-x ` goes to the first error.  C-x ` goes to the first error
when you have not visited any errors.

If this can be made to do the job, I would definitely prefer it.

    However, I take your point about moving point being a strange thing for
    a mode-line click to do.  The underlying idea is that clicking on the
    invalidity indicator ought to give information about the invalidity.
    Moving point to the location  of the first error was an easy way to do
    this, but perhaps a better way would be to pop up a buffer giving a
    summary of all the errors in the buffer.  Do you think that would be
    better?

I would rather avoid ad-hoc mode line interfaces.

Re: Propertizing the minor-mode-alist

[James Clark]

On Wed, 2004-09-22 at 01:31, Richard Stallman wrote:
>     I misspoke earlier: there's a keystroke for goto to next error, but not
>     for goto first error.
> 
> C-u C-x ` goes to the first error.  C-x ` goes to the first error
> when you have not visited any errors.
> 
> If this can be made to do the job, I would definitely prefer it.

I like the idea of using a C-u prefix to make the next error command go
to the first error.  I can definitely use that.

I don't see a good way to make it work with C-x `.  nxml-mode presents
errors using a very different style of UI from compile.el.  It's more
like what you see in modern IDEs.  The closest analog in Emacs in
jit-lock.  The errors are detected automatically and continuously
without the user explicitly calling a compile function (although they
can turn it off if they want).  It works by installing an
after-change-function: things that can be done quickly are done there,
the rest is done with an idle-timer.  Modifications don't necessarily
cause a complete reparse; it works incrementally and after most
modifications is usually able to just reparse and revalidate a small
part of the buffer.  Detected errors are highlighted with an overlay
over the exact range of characters that is in error with the error
message as a tooltip.  The error messages are generally quite short and
and designed to make sense in the context of the highlighting of the
precise relevant range of characters rather than the whole line or just
a position in the line; they wouldn't be very meaningful out of that
context. The next error function moves around the current buffer between
the highlighted errors.

I don't see any way to make compile.el do this.  Also I didn't want to
rebind C-x ` to my next error function, because I think it will be
common to want to run external compiler-like programs on XML files (e.g.
an XSLT processor), and a user would want to be able to use C-x ` to
step through the errors from that. My error function is currently bound
to C-c C-n.  Without an arg, it always moves to the first error after
point.

James

Re: Propertizing the minor-mode-alist

[Stefan Monnier]

> I like the idea of using a C-u prefix to make the next error command go
> to the first error.  I can definitely use that.

> I don't see a good way to make it work with C-x `.  nxml-mode presents
> errors using a very different style of UI from compile.el.  It's more

M-x next-error is now "independent" from compile.el.
Of course, this is specific to Emacs-CVS, but you could use this new feature
by providing a next-error-function (that's the name of the new var
introduced to decouple next-error from compile.el).


        Stefan

Re: Propertizing the minor-mode-alist

[James Clark]

On Wed, 2004-09-22 at 20:23, Stefan Monnier wrote:
> > I like the idea of using a C-u prefix to make the next error command go
> > to the first error.  I can definitely use that.
> 
> > I don't see a good way to make it work with C-x `.  nxml-mode presents
> > errors using a very different style of UI from compile.el.  It's more
> 
> M-x next-error is now "independent" from compile.el.
> Of course, this is specific to Emacs-CVS, but you could use this new feature
> by providing a next-error-function (that's the name of the new var
> introduced to decouple next-error from compile.el).

This looks great.  Let me just check my understanding here.

The doc string talks about "the next error in the current buffer".  This
means the next error following the error that was last found my the
next-error-function, not the next error after point.  So to support the
next-error-function I need to maintain an additional bit of state saying
which error in my buffer, if any, is the current error for the purposes
of next error function; if my next-error-function is called with a
non-nil RESET, I change the current error to be the first error in the
buffer.  

If the user wanted to use next-error to run through the errors from an
XSLT processor, say, instead from nxml-mode, then they would make the
compilation buffer be the only one displayed in the frame before calling
next-error.

Have I misunderstood anything?

James

Re: Propertizing the minor-mode-alist

[Stefan Monnier]

> next-error-function, not the next error after point.  So to support the
> next-error-function I need to maintain an additional bit of state saying
> which error in my buffer, if any, is the current error for the purposes
> of next error function; if my next-error-function is called with a
> non-nil RESET, I change the current error to be the first error in the
> buffer.

It's your call.  It's not required.  You can just use `point'.
E.g. in diff-mode, I just used point IIRC.


        Stefan

Re: Propertizing the minor-mode-alist

[Richard Stallman]

How about using fonts to show people the errors?

Re: Propertizing the minor-mode-alist

[James Clark]

> How about using fonts to show people the errors?

It does this already.  It defines a face which it uses just for
highlighting errors; it defaults to red underlining.

One thing that I think would be a nice touch here is if emacs could do
wavy underlines.  It's just a little more visually distinctive than a
straight underline and slightly more suggestive of an error condition. 
Perhaps one could allow the underline face attribute to have a list
value like the box attribute can:

  :underline (:line-width WIDTH :color COLOR :style STYLE)

where style could be things like wavy, dotted, dashed etc.

James

Re: Propertizing the minor-mode-alist

[Richard Stallman]

    One thing that I think would be a nice touch here is if emacs could do
    wavy underlines.  It's just a little more visually distinctive than a
    straight underline and slightly more suggestive of an error condition. 
    Perhaps one could allow the underline face attribute to have a list
    value like the box attribute can:

The feature would be unproblematic, but now's not the time for
new features like that.  If someone wants to implement it, after
the next release, installing it would be no problem.