unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
@ 2013-10-28 21:04 Barry OReilly
  2013-10-28 21:38 ` Glenn Morris
  2013-10-29  1:38 ` Stefan Monnier
  0 siblings, 2 replies; 8+ messages in thread
From: Barry OReilly @ 2013-10-28 21:04 UTC (permalink / raw)
  To: 15745


[-- Attachment #1.1: Type: text/plain, Size: 234 bytes --]

It would be more convenient to customize
semantic-idle-symbol-highlight-face in the manner defface usually
allows. However, semantic-idle-symbol-highlight-face is currently
defined with defvar.

The attached patch implements the fix.

[-- Attachment #1.2: Type: text/html, Size: 279 bytes --]

[-- Attachment #2: semantic-face.diff --]
[-- Type: application/octet-stream, Size: 4026 bytes --]

diff --git a/lisp/cedet/ChangeLog b/lisp/cedet/ChangeLog
index 6862181..289280c 100644
--- a/lisp/cedet/ChangeLog
+++ b/lisp/cedet/ChangeLog
@@ -1,3 +1,12 @@
+2013-10-28  Barry O'Reilly  <gundaetiapo@gmail.com>
+
+       * semantic/idle.el (semantic-idle-symbol-highlight-face): Define
+       face using defface instead of defvar.
+       (semantic-idle-symbol-maybe-highlight)
+       (semantic-idle-local-symbol-highlight): Pass face as symbol
+       * pulse.el (pulse-momentary-highlight-overlay)
+       (pulse-momentary-highlight-region): Fix typo in doc
+
 2013-10-20  Johan Bockgård  <bojohan@gnu.org>
 
        * semantic/db-mode.el (global-semanticdb-minor-mode): Remove hooks
diff --git a/lisp/cedet/pulse.el b/lisp/cedet/pulse.el
index 268beed..4a3b501 100644
--- a/lisp/cedet/pulse.el
+++ b/lisp/cedet/pulse.el
@@ -180,7 +180,7 @@ Be sure to call `pulse-reset-face' after calling pulse."
 
 (defun pulse-momentary-highlight-overlay (o &optional face)
   "Pulse the overlay O, unhighlighting before next command.
-Optional argument FACE specifies the fact to do the highlighting."
+Optional argument FACE specifies the face to do the highlighting."
   (overlay-put o 'original-face (overlay-get o 'face))
   (add-to-list 'pulse-momentary-overlay o)
   (if (eq pulse-flag 'never)
@@ -237,7 +237,7 @@ Optional argument FACE specifies the face to do the highlighting."
 
 (defun pulse-momentary-highlight-region (start end &optional face)
   "Highlight between START and END, unhighlighting before next command.
-Optional argument FACE specifies the fact to do the highlighting."
+Optional argument FACE specifies the face to do the highlighting."
   (let ((o (make-overlay start end)))
     ;; Mark it for deletion
     (overlay-put o 'pulse-delete t)
diff --git a/lisp/cedet/semantic/idle.el b/lisp/cedet/semantic/idle.el
index d024e5d..221292a 100644
--- a/lisp/cedet/semantic/idle.el
+++ b/lisp/cedet/semantic/idle.el
@@ -830,8 +830,14 @@ turned on in every Semantic-supported buffer."
 ;; of all uses of the symbol that is under the cursor.
 ;;
 ;; This is to mimic the Eclipse tool of a similar nature.
-(defvar semantic-idle-symbol-highlight-face 'region
-  "Face used for highlighting local symbols.")
+(defface semantic-idle-symbol-highlight-face
+  '((((class color) (background dark))
+     ;; Put this back to something closer to black later.
+     (:background "gray20"))
+    (((class color) (background light))
+     (:background "gray90")))
+  "Face used for highlighting local symbols."
+  :group 'semantic-faces)
 
 (defun semantic-idle-symbol-maybe-highlight (tag)
   "Perhaps add highlighting to the symbol represented by TAG.
@@ -857,12 +863,12 @@ visible, then highlight it."
 		      (point) (get-buffer-window (current-buffer) 'visible))
 		 (if (< (semantic-overlay-end region) (point-at-eol))
 		     (pulse-momentary-highlight-overlay
-		      region semantic-idle-symbol-highlight-face)
+		      region 'semantic-idle-symbol-highlight-face)
 		   ;; Not the same
 		   (pulse-momentary-highlight-region
 		    (semantic-overlay-start region)
 		    (point-at-eol)
-		    semantic-idle-symbol-highlight-face))))
+		    'semantic-idle-symbol-highlight-face))))
 	     ))
 	  ((vectorp region)
 	   (let ((start (aref region 0))
@@ -882,7 +888,7 @@ visible, then highlight it."
 		   (pulse-momentary-highlight-region
 		    start (if (<= end (point-at-eol)) end
 			    (point-at-eol))
-		    semantic-idle-symbol-highlight-face)))
+		    'semantic-idle-symbol-highlight-face)))
 	       ))))
     nil))
 
@@ -913,7 +919,7 @@ Call `semantic-symref-hits-in-region' to identify local references."
 	   target (lambda (start end prefix)
 		    (when (/= start (car Hbounds))
 		      (pulse-momentary-highlight-region
-		       start end semantic-idle-symbol-highlight-face))
+		       start end 'semantic-idle-symbol-highlight-face))
 		    (semantic-throw-on-input 'symref-highlight)
 		    )
 	   (semantic-tag-start tag)
diff --git a/src/eval.c b/src/eval.c
index 1e0a63a..4258b68 100644

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
  2013-10-28 21:04 bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface Barry OReilly
@ 2013-10-28 21:38 ` Glenn Morris
  2013-10-28 22:36   ` Barry OReilly
  2013-10-29  1:38 ` Stefan Monnier
  1 sibling, 1 reply; 8+ messages in thread
From: Glenn Morris @ 2013-10-28 21:38 UTC (permalink / raw)
  To: Barry OReilly; +Cc: 15745


New face names should not end in -face.

The standard backwards-compatible way to make a change like this is:

1) Add:
(defface semantic-idle-symbol-highlight ...)

2) Change:
(defvar semantic-idle-symbol-highlight-face 'semantic-idle-symbol-highlight)

3) Mark semantic-idle-symbol-highlight-face the variable obsolete,
in favour of semantic-idle-symbol-highlight the face.

See eg diary-face.





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
  2013-10-28 21:38 ` Glenn Morris
@ 2013-10-28 22:36   ` Barry OReilly
  2013-10-29  2:29     ` Glenn Morris
  0 siblings, 1 reply; 8+ messages in thread
From: Barry OReilly @ 2013-10-28 22:36 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 15745


[-- Attachment #1.1: Type: text/plain, Size: 43 bytes --]

Thanks, how is the attached revised patch?

[-- Attachment #1.2: Type: text/html, Size: 83 bytes --]

[-- Attachment #2: semantic-face.diff --]
[-- Type: application/octet-stream, Size: 4528 bytes --]

diff --git a/lisp/cedet/ChangeLog b/lisp/cedet/ChangeLog
index 6862181..4e3a553 100644
--- a/lisp/cedet/ChangeLog
+++ b/lisp/cedet/ChangeLog
@@ -1,3 +1,13 @@
+2013-10-28  Barry O'Reilly  <gundaetiapo@gmail.com>
+
+       * semantic/idle.el (semantic-idle-symbol-highlight)
+       (semantic-idle-symbol-highlight-face): Define face with defface
+       and obsolete the replaced one defined with defvar.
+       (semantic-idle-symbol-maybe-highlight)
+       (semantic-idle-local-symbol-highlight): Pass face as symbol
+       * pulse.el (pulse-momentary-highlight-overlay)
+       (pulse-momentary-highlight-region): Fix typo in doc
+
 2013-10-20  Johan Bockgård  <bojohan@gnu.org>
 
        * semantic/db-mode.el (global-semanticdb-minor-mode): Remove hooks
diff --git a/lisp/cedet/pulse.el b/lisp/cedet/pulse.el
index 268beed..4a3b501 100644
--- a/lisp/cedet/pulse.el
+++ b/lisp/cedet/pulse.el
@@ -180,7 +180,7 @@ Be sure to call `pulse-reset-face' after calling pulse."
 
 (defun pulse-momentary-highlight-overlay (o &optional face)
   "Pulse the overlay O, unhighlighting before next command.
-Optional argument FACE specifies the fact to do the highlighting."
+Optional argument FACE specifies the face to do the highlighting."
   (overlay-put o 'original-face (overlay-get o 'face))
   (add-to-list 'pulse-momentary-overlay o)
   (if (eq pulse-flag 'never)
@@ -237,7 +237,7 @@ Optional argument FACE specifies the face to do the highlighting."
 
 (defun pulse-momentary-highlight-region (start end &optional face)
   "Highlight between START and END, unhighlighting before next command.
-Optional argument FACE specifies the fact to do the highlighting."
+Optional argument FACE specifies the face to do the highlighting."
   (let ((o (make-overlay start end)))
     ;; Mark it for deletion
     (overlay-put o 'pulse-delete t)
diff --git a/lisp/cedet/semantic/idle.el b/lisp/cedet/semantic/idle.el
index d024e5d..4bde8a5 100644
--- a/lisp/cedet/semantic/idle.el
+++ b/lisp/cedet/semantic/idle.el
@@ -830,8 +830,19 @@ turned on in every Semantic-supported buffer."
 ;; of all uses of the symbol that is under the cursor.
 ;;
 ;; This is to mimic the Eclipse tool of a similar nature.
-(defvar semantic-idle-symbol-highlight-face 'region
-  "Face used for highlighting local symbols.")
+(defface semantic-idle-symbol-highlight
+  '((((class color) (background dark))
+     ;; Put this back to something closer to black later.
+     (:background "gray20"))
+    (((class color) (background light))
+     (:background "gray90")))
+  "Face used for highlighting local symbols."
+  :group 'semantic-faces)
+(defvar semantic-idle-symbol-highlight-face 'semantic-idle-symbol-highlight)
+(define-obsolete-face-alias
+  'semantic-idle-symbol-highlight-face
+  'semantic-idle-symbol-highlight
+  "24.4")
 
 (defun semantic-idle-symbol-maybe-highlight (tag)
   "Perhaps add highlighting to the symbol represented by TAG.
@@ -857,12 +868,12 @@ visible, then highlight it."
                      (point) (get-buffer-window (current-buffer) 'visible))
                 (if (< (semantic-overlay-end region) (point-at-eol))
                     (pulse-momentary-highlight-overlay
-                     region semantic-idle-symbol-highlight-face)
+                     region 'semantic-idle-symbol-highlight)
                   ;; Not the same
                   (pulse-momentary-highlight-region
                    (semantic-overlay-start region)
                    (point-at-eol)
-                   semantic-idle-symbol-highlight-face))))
+                   'semantic-idle-symbol-highlight))))
             ))
          ((vectorp region)
           (let ((start (aref region 0))
@@ -882,7 +893,7 @@ visible, then highlight it."
                   (pulse-momentary-highlight-region
                    start (if (<= end (point-at-eol)) end
                            (point-at-eol))
-                   semantic-idle-symbol-highlight-face)))
+                   'semantic-idle-symbol-highlight)))
               ))))
     nil))
 
@@ -913,7 +924,7 @@ Call `semantic-symref-hits-in-region' to identify local references."
           target (lambda (start end prefix)
                    (when (/= start (car Hbounds))
                      (pulse-momentary-highlight-region
-                      start end semantic-idle-symbol-highlight-face))
+                      start end 'semantic-idle-symbol-highlight))
                    (semantic-throw-on-input 'symref-highlight)
                    )
           (semantic-tag-start tag)

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
  2013-10-28 21:04 bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface Barry OReilly
  2013-10-28 21:38 ` Glenn Morris
@ 2013-10-29  1:38 ` Stefan Monnier
  1 sibling, 0 replies; 8+ messages in thread
From: Stefan Monnier @ 2013-10-29  1:38 UTC (permalink / raw)
  To: Barry OReilly; +Cc: 15745

> -(defvar semantic-idle-symbol-highlight-face 'region
> -  "Face used for highlighting local symbols.")
> +(defface semantic-idle-symbol-highlight-face
> +  '((((class color) (background dark))
> +     ;; Put this back to something closer to black later.
> +     (:background "gray20"))
> +    (((class color) (background light))
> +     (:background "gray90")))
> +  "Face used for highlighting local symbols."
> +  :group 'semantic-faces)

Just define the new face to inherit from `region', so as to preserve the
old behavior:

(defface semantic-idle-symbol-highlight
  '((t :inherit region))
  "Face used for highlighting local symbols.")


        Stefan





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
  2013-10-28 22:36   ` Barry OReilly
@ 2013-10-29  2:29     ` Glenn Morris
  2013-10-29 15:56       ` Barry OReilly
  0 siblings, 1 reply; 8+ messages in thread
From: Glenn Morris @ 2013-10-29  2:29 UTC (permalink / raw)
  To: Barry OReilly; +Cc: 15745

Barry OReilly wrote:

> Thanks, how is the attached revised patch?

Not quite what I had in mind (taking something that was a variable and
turning it into a face alias won't work right, will it?).

-(defvar semantic-idle-symbol-highlight-face 'region
-  "Face used for highlighting local symbols.")
+(defface semantic-idle-symbol-highlight
+  '((((class color) (background dark))
+     ;; Put this back to something closer to black later.
+     (:background "gray20"))
+    (((class color) (background light))
+     (:background "gray90")))
+  "Face used for highlighting local symbols."
+  :group 'semantic-faces)

As has been said:

  (defface semantic-idle-symbol-highlight '((t (:inherit region))) ...)

+(define-obsolete-face-alias
+  'semantic-idle-symbol-highlight-face
+  'semantic-idle-symbol-highlight
+  "24.4")

Rather than the above, I meant:

  (make-obsolete-variable 'semantic-idle-symbol-highlight-face
    "customize the face `semantic-idle-symbol-highlight' instead" "24.4" 'set)

Then these bits:

-                     region semantic-idle-symbol-highlight-face)
+                     region 'semantic-idle-symbol-highlight)

are not necessary (or appropriate).





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
  2013-10-29  2:29     ` Glenn Morris
@ 2013-10-29 15:56       ` Barry OReilly
  2013-10-29 22:37         ` Glenn Morris
  0 siblings, 1 reply; 8+ messages in thread
From: Barry OReilly @ 2013-10-29 15:56 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 15745


[-- Attachment #1.1: Type: text/plain, Size: 1290 bytes --]

Thank you for your patience and guidance.

> Just define the new face to inherit from `region', so as to preserve
> the old behavior:
>
> (defface semantic-idle-symbol-highlight
>   '((t :inherit region))
>   "Face used for highlighting local symbols.")

It's weird that the region face would be used for this, because it has
nothing to do with the region. The fact that it can make me think I
have a region marked when I don't is a motivator for this bug report.
My primary concern is that I can customize it personally, so I revised
the patch to have the face inherit region.

> Not quite what I had in mind (taking something that was a variable
> and turning it into a face alias won't work right, will it?).

I probably misinterpreted your earlier tip. I looked off of
diary-face:

  (define-obsolete-face-alias 'diary-face 'diary "22.1")

I tested the last patch before sending it and it worked. Although that
patch used the defface variable and not the obsoleted one.

> Then these bits:
>
> -                     region semantic-idle-symbol-highlight-face)
> +                     region 'semantic-idle-symbol-highlight)
>
> are not necessary (or appropriate).

I don't know why using obsolete variables is most appropriate, but
I've done as requested in the revised attached patch.

[-- Attachment #1.2: Type: text/html, Size: 1539 bytes --]

[-- Attachment #2: semantic-face.diff --]
[-- Type: application/octet-stream, Size: 2654 bytes --]

diff --git a/lisp/cedet/ChangeLog b/lisp/cedet/ChangeLog
index 6862181..65f7330 100644
--- a/lisp/cedet/ChangeLog
+++ b/lisp/cedet/ChangeLog
@@ -1,3 +1,11 @@
+2013-10-28  Barry O'Reilly  <gundaetiapo@gmail.com>
+
+       * semantic/idle.el (semantic-idle-symbol-highlight)
+       (semantic-idle-symbol-highlight-face): Define face with defface
+       and obsolete the replaced one defined with defvar.
+       * pulse.el (pulse-momentary-highlight-overlay)
+       (pulse-momentary-highlight-region): Fix typo in doc
+
 2013-10-20  Johan Bockgård  <bojohan@gnu.org>
 
        * semantic/db-mode.el (global-semanticdb-minor-mode): Remove hooks
diff --git a/lisp/cedet/pulse.el b/lisp/cedet/pulse.el
index 268beed..4a3b501 100644
--- a/lisp/cedet/pulse.el
+++ b/lisp/cedet/pulse.el
@@ -180,7 +180,7 @@ Be sure to call `pulse-reset-face' after calling pulse."
 
 (defun pulse-momentary-highlight-overlay (o &optional face)
   "Pulse the overlay O, unhighlighting before next command.
-Optional argument FACE specifies the fact to do the highlighting."
+Optional argument FACE specifies the face to do the highlighting."
   (overlay-put o 'original-face (overlay-get o 'face))
   (add-to-list 'pulse-momentary-overlay o)
   (if (eq pulse-flag 'never)
@@ -237,7 +237,7 @@ Optional argument FACE specifies the face to do the highlighting."
 
 (defun pulse-momentary-highlight-region (start end &optional face)
   "Highlight between START and END, unhighlighting before next command.
-Optional argument FACE specifies the fact to do the highlighting."
+Optional argument FACE specifies the face to do the highlighting."
   (let ((o (make-overlay start end)))
     ;; Mark it for deletion
     (overlay-put o 'pulse-delete t)
diff --git a/lisp/cedet/semantic/idle.el b/lisp/cedet/semantic/idle.el
index d024e5d..c15b6bf 100644
--- a/lisp/cedet/semantic/idle.el
+++ b/lisp/cedet/semantic/idle.el
@@ -830,8 +830,14 @@ turned on in every Semantic-supported buffer."
 ;; of all uses of the symbol that is under the cursor.
 ;;
 ;; This is to mimic the Eclipse tool of a similar nature.
-(defvar semantic-idle-symbol-highlight-face 'region
+(defface semantic-idle-symbol-highlight
+  '((t :inherit region))
+  "Face used for highlighting local symbols."
+  :group 'semantic-faces)
+(defvar semantic-idle-symbol-highlight-face 'semantic-idle-symbol-highlight
   "Face used for highlighting local symbols.")
+(make-obsolete-variable 'semantic-idle-symbol-highlight-face
+    "customize the face `semantic-idle-symbol-highlight' instead" "24.4" 'set)
 
 (defun semantic-idle-symbol-maybe-highlight (tag)
   "Perhaps add highlighting to the symbol represented by TAG.

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
  2013-10-29 15:56       ` Barry OReilly
@ 2013-10-29 22:37         ` Glenn Morris
  2013-10-31  1:55           ` Barry OReilly
  0 siblings, 1 reply; 8+ messages in thread
From: Glenn Morris @ 2013-10-29 22:37 UTC (permalink / raw)
  To: Barry OReilly; +Cc: 15745

Barry OReilly wrote:

> It's weird that the region face would be used for this, because it has
> nothing to do with the region. The fact that it can make me think I
> have a region marked when I don't is a motivator for this bug report.

Wanting to change the default appearance is a separate issue! :)
You could ask cedet-devel.

> I probably misinterpreted your earlier tip. I looked off of
> diary-face:
>
>   (define-obsolete-face-alias 'diary-face 'diary "22.1")

Sorry, diary-face was a bad, confusing example for me to pick.
I was looking in diary-lib.el, not calendar.el.

> I've done as requested in the revised attached patch.

Looks fine to me.





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface
  2013-10-29 22:37         ` Glenn Morris
@ 2013-10-31  1:55           ` Barry OReilly
  0 siblings, 0 replies; 8+ messages in thread
From: Barry OReilly @ 2013-10-31  1:55 UTC (permalink / raw)
  Cc: 15745-done

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

Rev 114879

[-- Attachment #2: Type: text/html, Size: 71 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-10-31  1:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-28 21:04 bug#15745: [PATCH] Define semantic-idle-symbol-highlight-face with defface Barry OReilly
2013-10-28 21:38 ` Glenn Morris
2013-10-28 22:36   ` Barry OReilly
2013-10-29  2:29     ` Glenn Morris
2013-10-29 15:56       ` Barry OReilly
2013-10-29 22:37         ` Glenn Morris
2013-10-31  1:55           ` Barry OReilly
2013-10-29  1:38 ` Stefan Monnier

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).