foreground menu bug

foreground menu bug

[Werner LEMBERG]

[emacs CVS 2005-06-12 with GTK]

If I mark a text region and select the menu entry

  Edit->Text Properties->Foreground Color->Other...

I can select a colour (say, red) and everything is fine.

After doing that, the colour `red' is added to the last submenu so
that it can be accessed as

  Edit->Text Properties->Foreground Color->red

Doing this on another region, it is *not* coloured.

IMHO, this is a bug.


    Werner

Re: foreground menu bug

[Mathias Dahl]

Werner LEMBERG <wl@gnu.org> writes:

> [emacs CVS 2005-06-12 with GTK]
>
> If I mark a text region and select the menu entry
>
>   Edit->Text Properties->Foreground Color->Other...
>
> I can select a colour (say, red) and everything is fine.
>
> After doing that, the colour `red' is added to the last submenu so
> that it can be accessed as
>
>   Edit->Text Properties->Foreground Color->red
>
> Doing this on another region, it is *not* coloured.
>
> IMHO, this is a bug.

I tested this the other day too and was confuzzled (confused +
puzzled). After a while I realized that it had to do woth
font-lock-mode overriding my coloring.

I don't think that this is a bug, but I'm not sure. It certainly is
puzzling for some users though.

Re: foreground menu bug

[Richard Stallman]

    I tested this the other day too and was confuzzled (confused +
    puzzled). After a while I realized that it had to do woth
    font-lock-mode overriding my coloring.

Perhaps those menu items should be disabled when Font Lock is enabled.
That way, people will see they're not supposed to work.

However, doing that in a simple way would have the wrong results for
Enriched mode buffers when Global Font Lock mode has been enabled.  So
the criterion "when font lock mode is enabled" is not exactly the
right criterion.  Perhaps it should be "in major modes that specify
something nontrivial for Font Lock to do", or "in such major modes
when Font Lock is enabled."

Would someone like to do this?

Re: foreground menu bug

[Lute Kamstra]

Mathias Dahl <brakjoller@gmail.com> writes:

> Werner LEMBERG <wl@gnu.org> writes:
>
>> [emacs CVS 2005-06-12 with GTK]
>>
>> If I mark a text region and select the menu entry
>>
>>   Edit->Text Properties->Foreground Color->Other...
>>
>> I can select a colour (say, red) and everything is fine.
>>
>> After doing that, the colour `red' is added to the last submenu so
>> that it can be accessed as
>>
>>   Edit->Text Properties->Foreground Color->red
>>
>> Doing this on another region, it is *not* coloured.
>>
>> IMHO, this is a bug.
>
> I tested this the other day too and was confuzzled (confused +
> puzzled). After a while I realized that it had to do woth
> font-lock-mode overriding my coloring.
>
> I don't think that this is a bug, but I'm not sure. It certainly is
> puzzling for some users though.

I think you and Werner are talking about different things.  You talk
about Font Lock mode overriding the text properties you set manually.
Werner's problem happens even when Font Lock mode is off.  The "red"
menu entry (that is created automatically when you set the foreground
text property once) simply doesn't work.  That's a bug.

Lute.

Re: foreground menu bug

[Richard Stallman]

    I think you and Werner are talking about different things.  You talk
    about Font Lock mode overriding the text properties you set manually.
    Werner's problem happens even when Font Lock mode is off.  The "red"
    menu entry (that is created automatically when you set the foreground
    text property once) simply doesn't work.  That's a bug.

If it simply doesn't work, that is definitely a bug too.

Could you investigate the bug?

Re: foreground menu bug

[Lute Kamstra]

Richard Stallman <rms@gnu.org> writes:

>     I think you and Werner are talking about different things.  You talk
>     about Font Lock mode overriding the text properties you set manually.
>     Werner's problem happens even when Font Lock mode is off.  The "red"
>     menu entry (that is created automatically when you set the foreground
>     text property once) simply doesn't work.  That's a bug.
>
> If it simply doesn't work, that is definitely a bug too.
>
> Could you investigate the bug?

I'll do that.

Lute.

Re: foreground menu bug

[Lute Kamstra]

Hi Werner,

> [emacs CVS 2005-06-12 with GTK]
>
> If I mark a text region and select the menu entry
>
>   Edit->Text Properties->Foreground Color->Other...
>
> I can select a colour (say, red) and everything is fine.
>
> After doing that, the colour `red' is added to the last submenu so
> that it can be accessed as
>
>   Edit->Text Properties->Foreground Color->red
>
> Doing this on another region, it is *not* coloured.
>
> IMHO, this is a bug.

Could you test this patch?

Lute.


Index: lisp/facemenu.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/facemenu.el,v
retrieving revision 1.77
diff -c -r1.77 facemenu.el
*** lisp/facemenu.el	8 Mar 2005 21:47:03 -0000	1.77
--- lisp/facemenu.el	20 Jun 2005 08:24:46 -0000
***************
*** 358,367 ****
  			 (region-beginning))
  		     (if (and mark-active (not current-prefix-arg))
  			 (region-end))))
!   (unless (color-defined-p color)
!     (message "Color `%s' undefined" color))
!   (facemenu-add-new-color color 'facemenu-foreground-menu)
!   (facemenu-add-face (list (list :foreground color)) start end))
  
  ;;;###autoload
  (defun facemenu-set-background (color &optional start end)
--- 358,365 ----
  			 (region-beginning))
  		     (if (and mark-active (not current-prefix-arg))
  			 (region-end))))
!   (facemenu-add-face (facemenu-add-new-color color 'facemenu-foreground-menu)
! 		     start end))
  
  ;;;###autoload
  (defun facemenu-set-background (color &optional start end)
***************
*** 382,391 ****
  			 (region-beginning))
  		     (if (and mark-active (not current-prefix-arg))
  			 (region-end))))
!   (unless (color-defined-p color)
!     (message "Color `%s' undefined" color))
!   (facemenu-add-new-color color 'facemenu-background-menu)
!   (facemenu-add-face (list (list :background color)) start end))
  
  ;;;###autoload
  (defun facemenu-set-face-from-menu (face start end)
--- 380,387 ----
  			 (region-beginning))
  		     (if (and mark-active (not current-prefix-arg))
  			 (region-end))))
!   (facemenu-add-face (facemenu-add-new-color color 'facemenu-background-menu)
! 		     start end))
  
  ;;;###autoload
  (defun facemenu-set-face-from-menu (face start end)
***************
*** 406,412 ****
  		     (if (and mark-active (not current-prefix-arg))
  			 (region-end))))
    (barf-if-buffer-read-only)
-   (facemenu-get-face face)
    (if start
        (facemenu-add-face face start end)
      (facemenu-add-face face)))
--- 402,407 ----
***************
*** 641,654 ****
        (setq face-list (cdr face-list)))
      (nreverse active-list)))
  
- (defun facemenu-get-face (symbol)
-   "Make sure FACE exists.
- If not, create it and add it to the appropriate menu.  Return the SYMBOL."
-   (let ((name (symbol-name symbol)))
-     (cond ((facep symbol))
- 	  (t (make-face symbol))))
-   symbol)
- 
  (defun facemenu-add-new-face (face)
    "Add FACE (a face) to the Face menu.
  
--- 636,641 ----
***************
*** 708,754 ****
  	     (define-key menu key (cons name function))))))
    nil) ; Return nil for facemenu-iterate
  
! (defun facemenu-add-new-color (color &optional menu)
    "Add COLOR (a color name string) to the appropriate Face menu.
! MENU should be `facemenu-foreground-menu' or
! `facemenu-background-menu'.
  
  This is called whenever you use a new color."
!   (let* (name
! 	 symbol
! 	 docstring
! 	 function menu-val key
! 	 (color-p (memq menu '(facemenu-foreground-menu
! 			       facemenu-background-menu))))
!     (unless (stringp color)
!       (error "%s is not a color" color))
!     (setq name color
! 	  symbol (intern name))
! 
      (cond ((eq menu 'facemenu-foreground-menu)
  	   (setq docstring
  		 (format "Select foreground color %s for subsequent insertion."
! 			 name)))
  	  ((eq menu 'facemenu-background-menu)
  	   (setq docstring
  		 (format "Select background color %s for subsequent insertion."
! 			 name))))
      (cond ((facemenu-iterate ; check if equivalent face is already in the menu
  	    (lambda (m) (and (listp m)
  			     (symbolp (car m))
  			     (stringp (cadr m))
  			     (string-equal (cadr m) color)))
  	    (cdr (symbol-function menu))))
! 	  (t   ; No keyboard equivalent.  Figure out where to put it:
! 	   (setq key (vector symbol)
! 		 function 'facemenu-set-face-from-menu
! 		 menu-val (symbol-function menu))
! 	   (if (and facemenu-new-faces-at-end
! 		   (> (length menu-val) 3))
! 	       (define-key-after menu-val key (cons name function)
! 		 (car (nth (- (length menu-val) 3) menu-val)))
! 	     (define-key menu key (cons name function))))))
!   nil) ; Return nil for facemenu-iterate
  
  (defun facemenu-complete-face-list (&optional oldlist)
    "Return list of all faces that look different.
--- 695,738 ----
  	     (define-key menu key (cons name function))))))
    nil) ; Return nil for facemenu-iterate
  
! (defun facemenu-add-new-color (color menu)
    "Add COLOR (a color name string) to the appropriate Face menu.
! MENU should be `facemenu-foreground-menu' or `facemenu-background-menu'.
! Create the appropriate face and return it.
  
  This is called whenever you use a new color."
!   (let (symbol docstring)
!     (unless (color-defined-p color)
!       (error "Color `%s' undefined" color))
      (cond ((eq menu 'facemenu-foreground-menu)
  	   (setq docstring
  		 (format "Select foreground color %s for subsequent insertion."
! 			 color)
! 		 symbol (intern (concat "fg:" color)))
! 	   (set-face-foreground (make-face symbol) color))
  	  ((eq menu 'facemenu-background-menu)
  	   (setq docstring
  		 (format "Select background color %s for subsequent insertion."
! 			 color)
! 		 symbol (intern (concat "bg:" color)))
! 	   (set-face-background (make-face symbol) color))
! 	  (t (error "MENU should be `facemenu-foreground-menu' or `facemenu-background-menu'")))
      (cond ((facemenu-iterate ; check if equivalent face is already in the menu
  	    (lambda (m) (and (listp m)
  			     (symbolp (car m))
  			     (stringp (cadr m))
  			     (string-equal (cadr m) color)))
  	    (cdr (symbol-function menu))))
! 	  (t	; No keyboard equivalent.  Figure out where to put it:
! 	   (let ((key (vector symbol))
! 		 (function 'facemenu-set-face-from-menu)
! 		 (menu-val (symbol-function menu)))
! 	     (if (and facemenu-new-faces-at-end
! 		      (> (length menu-val) 3))
! 		 (define-key-after menu-val key (cons color function)
! 		   (car (nth (- (length menu-val) 3) menu-val)))
! 	       (define-key menu key (cons color function))))))
!     symbol))
  
  (defun facemenu-complete-face-list (&optional oldlist)
    "Return list of all faces that look different.

Re: foreground menu bug

[Lute Kamstra]

Richard Stallman <rms@gnu.org> writes:

>     I tested this the other day too and was confuzzled (confused +
>     puzzled). After a while I realized that it had to do woth
>     font-lock-mode overriding my coloring.
>
> Perhaps those menu items should be disabled when Font Lock is enabled.
> That way, people will see they're not supposed to work.
>
> However, doing that in a simple way would have the wrong results for
> Enriched mode buffers when Global Font Lock mode has been enabled.  So
> the criterion "when font lock mode is enabled" is not exactly the
> right criterion.  Perhaps it should be "in major modes that specify
> something nontrivial for Font Lock to do", or "in such major modes
> when Font Lock is enabled."
>
> Would someone like to do this?

I'll look into it.

Lute.

Re: foreground menu bug

[Mathias Dahl]

Richard Stallman <rms@gnu.org> writes:

>     I tested this the other day too and was confuzzled (confused +
>     puzzled). After a while I realized that it had to do woth
>     font-lock-mode overriding my coloring.
>
> Perhaps those menu items should be disabled when Font Lock is enabled.
> That way, people will see they're not supposed to work.
>
> However, doing that in a simple way would have the wrong results for
> Enriched mode buffers when Global Font Lock mode has been enabled.  So
> the criterion "when font lock mode is enabled" is not exactly the
> right criterion.  Perhaps it should be "in major modes that specify
> something nontrivial for Font Lock to do", or "in such major modes
> when Font Lock is enabled."

Or, it could just warn the user that the highlighting will not be
visible since font lock is enabled?

Re: foreground menu bug

[Richard M. Stallman]

    Or, it could just warn the user that the highlighting will not be
    visible since font lock is enabled?

It would be possible, but why be so complicated?
This change seems to do the right job.

*** facemenu.el	08 Mar 2005 22:24:04 -0500	1.77
--- facemenu.el	25 Jun 2005 18:19:29 -0400	
***************
*** 162,167 ****
--- 162,168 ----
    "Menu keymap for faces.")
  ;;;###autoload
  (defalias 'facemenu-face-menu facemenu-face-menu)
+ (put 'facemenu-face-menu 'menu-enable '(facemenu-enable-faces-p))
  
  ;;;###autoload
  (defvar facemenu-foreground-menu
***************
*** 171,176 ****
--- 172,178 ----
    "Menu keymap for foreground colors.")
  ;;;###autoload
  (defalias 'facemenu-foreground-menu facemenu-foreground-menu)
+ (put 'facemenu-foreground-menu 'menu-enable '(facemenu-enable-faces-p))
  
  ;;;###autoload
  (defvar facemenu-background-menu
***************
*** 180,185 ****
--- 182,192 ----
    "Menu keymap for background colors.")
  ;;;###autoload
  (defalias 'facemenu-background-menu facemenu-background-menu)
+ (put 'facemenu-background-menu 'menu-enable '(facemenu-enable-faces-p))
+ 
+ ;;; Condition for enabling menu items that set faces.
+ (defun facemenu-enable-faces-p ()
+   (not (and font-lock-mode font-lock-defaults)))
  
  ;;;###autoload
  (defvar facemenu-special-menu

Re: foreground menu bug

[Lute Kamstra]

"Richard M. Stallman" <rms@gnu.org> writes:

>     Or, it could just warn the user that the highlighting will not be
>     visible since font lock is enabled?
>
> It would be possible, but why be so complicated?
> This change seems to do the right job.

You asked if someone would like to work on this and I said I would.
In the meantime I've written a functional equivalent patch.  It's a
bit a waste of resources to do double work.

Lute.

Re: foreground menu bug

[Richard M. Stallman]

    You asked if someone would like to work on this and I said I would.
    In the meantime I've written a functional equivalent patch.  It's a
    bit a waste of resources to do double work.

Sorry.  I came across the message again, and didn't remember we
had discussed it, so it seemed easiest simply to do the job.

Re: foreground menu bug

[Mathias Dahl]

"Richard M. Stallman" <rms@gnu.org> writes:

>     Or, it could just warn the user that the highlighting will not be
>     visible since font lock is enabled?
>
> It would be possible, but why be so complicated?
> This change seems to do the right job.

Assuming the user notices that we disabled the menu item, your fix
might work. What about users who use M-x apropos or similar to find
commands and then used enter the commands with M-x, how would they
know that this comment is currently "disabled", or not meant to be
used?

Also, what does a disabled menu item tell the user? He might not
understand that it is because font lock is enabled. He might wonder if
he is doing something wrong seeing that menu item disabled.

I think this is a bad idea which just avoids solving the real
problem. If we disable the menu item, which is one way for the user to
execute the command, we should also disabled the other ways to execute
that command. Or make the command warn the user in some way.

Re: foreground menu bug

[Werner LEMBERG]

> > After doing that, the colour `red' is added to the last submenu so
> > that it can be accessed as
> >
> >   Edit->Text Properties->Foreground Color->red
> >
> > Doing this on another region, it is *not* coloured.
> >
> > IMHO, this is a bug.
> 
> Could you test this patch?

The CVS from today is just fine.  Thanks!


    Werner

Re: foreground menu bug

[Richard M. Stallman]

    Also, what does a disabled menu item tell the user? He might not
    understand that it is because font lock is enabled. He might wonder if
    he is doing something wrong seeing that menu item disabled.

This makes no sense.  Menu items are disabled when they are not
applicable; users know that.

    Assuming the user notices that we disabled the menu item, your fix
    might work. What about users who use M-x apropos or similar to find
    commands and then used enter the commands with M-x, how would they
    know that this comment is currently "disabled", or not meant to be
    used?

It might be a good idea to do that in addition.  What do you think
of this patch?

*** facemenu.el	27 Jun 2005 16:24:59 -0400	1.79
--- facemenu.el	27 Jun 2005 22:24:24 -0400	
***************
*** 611,617 ****
  						  self-insert-face
  						(list self-insert-face)))
  				 face)
! 	      self-insert-face-command this-command)))))
  
  (defun facemenu-active-faces (face-list &optional frame)
    "Return from FACE-LIST those faces that would be used for display.
--- 611,619 ----
  						  self-insert-face
  						(list self-insert-face)))
  				 face)
! 	      self-insert-face-command this-command))))
!   (unless (facemenu-enable-faces-p)
!     (message "Font-lock mode will override any faces you set in this buffer")))
  
  (defun facemenu-active-faces (face-list &optional frame)
    "Return from FACE-LIST those faces that would be used for display.

Re: foreground menu bug

[Mathias Dahl]

"Richard M. Stallman" <rms@gnu.org> writes:

>     Also, what does a disabled menu item tell the user? He might not
>     understand that it is because font lock is enabled. He might wonder if
>     he is doing something wrong seeing that menu item disabled.
>
> This makes no sense.  Menu items are disabled when they are not
> applicable; users know that.

You are right, that did not make much sense. I guess what I meant was
that when the user sees a disabled menu item, he does not necessarily
understand *why* it is disabled and how to enable it (in this case
disable font lock). I suggest documenting this in some place so that
he can find out why it is disabled and how to enable it.

>     Assuming the user notices that we disabled the menu item, your fix
>     might work. What about users who use M-x apropos or similar to find
>     commands and then used enter the commands with M-x, how would they
>     know that this comment is currently "disabled", or not meant to be
>     used?
>
> It might be a good idea to do that in addition.  What do you think
> of this patch?
>
> *** facemenu.el	27 Jun 2005 16:24:59 -0400	1.79
> --- facemenu.el	27 Jun 2005 22:24:24 -0400	
> ...

I think it looks good. That message would have helped me.