[NonGNU ELPA] new package: eglot-inactive-regions

[NonGNU ELPA] new package: eglot-inactive-regions

[Filippo Argiolas]

Hi all,

a couple of weeks ago I submitted my clangd-inactive-regions package
NonGNU ELPA inclusion. Previous discussion led to renaming the package
to make it more general, so I am submitting it again.

For whom who missed it, it's a little Eglot extension to visually style
inactive preprocessor branches in c/cpp code in a LSP powered way.

You can find more at:
https://github.com/fargiolas/eglot-inactive-regions

This addresses some of the feedback received last time:

- renamed the package to eglot-inactive-regions so it's not limited to
  clangd support only

- added experimental support for ccls skippedRegions, so the rename
  actually makes sense

- turned it into a global minor mode (thanks Gerd!). Now once the mode
  is active globally it gets automatically enabled in an eglot managed
  buffer when an inactive region notification arrives from the LSP
  server

- moved all the user options to defcustoms

I hope this rename won't cause too much disruption to my users.
I think all the changes are for the best but let me know if I broke
anything in the process.

As always, feedback is very welcome!

Cheers,
Filippo

Re: [NonGNU ELPA] new package: eglot-inactive-regions

[Philip Kaludercic]

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

Filippo Argiolas <filippo.argiolas@gmail.com> writes:

> Hi all,
>
> a couple of weeks ago I submitted my clangd-inactive-regions package
> NonGNU ELPA inclusion. Previous discussion led to renaming the package
> to make it more general, so I am submitting it again.
>
> For whom who missed it, it's a little Eglot extension to visually style
> inactive preprocessor branches in c/cpp code in a LSP powered way.
>
> You can find more at:
> https://github.com/fargiolas/eglot-inactive-regions

Here are a few comments and alternatives that you might be interested
in:


[-- Attachment #2: Type: text/plain, Size: 9669 bytes --]

diff --git a/eglot-inactive-regions.el b/eglot-inactive-regions.el
index f28bb73..2f58d57 100644
--- a/eglot-inactive-regions.el
+++ b/eglot-inactive-regions.el
@@ -48,6 +48,7 @@
 (require 'eglot)
 (require 'color)
 (require 'font-lock)
+(eval-when-compile (require 'pcase))
 
 (defgroup inactive-regions nil
   "Eglot Inactive Regions."
@@ -56,7 +57,7 @@
 
 (defun eglot-inactive-regions--set-and-refresh (sym val)
   "Set custom variable SYM to value VAL and trigger a refresh of all active buffers."
-  (set sym val)
+  (set-default sym val)			;this is the default setter
   (when (fboundp 'eglot-inactive-regions-refresh-all)
     (eglot-inactive-regions-refresh-all)))
 
@@ -65,17 +66,15 @@
 Used to mix foreground and background colors and apply to the foreground
 face of the inactive region.  The lower the blending factor the more
 text will look dim."
-  :type '(float :tag "Opacity" :min 0.0 :max 1.0)
-  :set #'eglot-inactive-regions--set-and-refresh
-  :group 'inactive-regions)
+  :type '(float :tag "Opacity" :min 0.0 :max 1.0) ;:min and :max have no effect, but you can use :validate
+  :set #'eglot-inactive-regions--set-and-refresh)
 
 (defcustom eglot-inactive-regions-shading 0.08
   "Blending factor for the `shade-background' style.
 Used to mix background and foreground colors and shade inactive region
 background face.  The lower the more subtle shading will be."
   :type '(float :tag "Opacity" :min 0.0 :max 1.0)
-  :set #'eglot-inactive-regions--set-and-refresh
-  :group 'inactive-regions)
+  :set #'eglot-inactive-regions--set-and-refresh)
 
 (defcustom eglot-inactive-regions-style 'darken-foreground
   "Shading style to apply to the inactive code regions.
@@ -87,8 +86,7 @@ Allowed styles:
           (const :tag "Darken foreground" darken-foreground)
           (const :tag "Shade background" shade-background)
           (const :tag "Shadow" shadow-face))
-  :set #'eglot-inactive-regions--set-and-refresh
-  :group 'inactive-regions)
+  :set #'eglot-inactive-regions--set-and-refresh)
 
 (defface eglot-inactive-regions-shadow-face
   '((t (:inherit shadow)))
@@ -111,7 +109,6 @@ factor.  All other face attributes you can customize.")
 (define-minor-mode eglot-inactive-regions-mode
   "Minor mode to enable Eglot powered ifdef highlighting."
   :global t
-  :group 'inactive-regions
   (cond (eglot-inactive-regions-mode
          (eglot-inactive-regions--enable))
         (t
@@ -120,17 +117,15 @@ factor.  All other face attributes you can customize.")
 (defun eglot-inactive-regions--styles ()
   "Return a cons list of style names and styles."
   (let ((choices (cdr (get 'eglot-inactive-regions-style 'custom-type))))
-    (mapcar (lambda (opt)
-              (pcase opt (`(const :tag ,tag ,value) (cons tag value))))
+    (mapcar (pcase-lambda (`(const :tag ,tag ,value)) (cons tag value))
             choices)))
 
 (defun eglot-inactive-regions-set-style (style)
   "Interactively select a shading STYLE to render inactive code regions."
   (interactive
    (let* ((styles (eglot-inactive-regions--styles))
-          (names (mapcar #'car styles))
           (prompt "Set inactive regions shading style: ")
-          (name (completing-read prompt names)))
+          (name (completing-read prompt styles)))
      (list (cdr (assoc name styles)))))
   (setq eglot-inactive-regions-style style)
   (eglot-inactive-regions-refresh-all))
@@ -139,8 +134,8 @@ factor.  All other face attributes you can customize.")
   "Interactively set a new OPACITY value for inactive regions.
 Only applies to `darken-foreground' style."
   (interactive "nNew inactive region foreground color opacity [0-1.0]: ")
-  (unless (and (>= opacity 0.0) (<= opacity 1.0))
-    (error "Opacity should be between 0.0 and 1.0"))
+  (unless (<= 0 opacity 1)
+    (user-error "Opacity should be between 0.0 and 1.0"))
   (setq eglot-inactive-regions-opacity opacity)
   (eglot-inactive-regions-refresh-all))
 
@@ -148,7 +143,7 @@ Only applies to `darken-foreground' style."
   "Interactively set a new SHADING value for inactive regions.
 Only applies to `shade-background' style."
   (interactive "nNew inactive region background color shading [0-1.0]: ")
-  (unless (and (>= shading 0.0) (<= shading 1.0))
+  (unless (<= 0 shading 1)
     (error "Shading factor should be between 0.0 and 1.0"))
   (setq eglot-inactive-regions-shading shading)
   (eglot-inactive-regions-refresh-all))
@@ -157,13 +152,13 @@ Only applies to `shade-background' style."
   "Linearly interpolate between two colors.
 Blend colors FROM-COLOR and TO-COLOR with ALPHA interpolation
 factor."
-  (if-let ((from-rgb (color-name-to-rgb from-color))
-           (to-rgb (color-name-to-rgb to-color))
-           (alpha (min 1.0 (max 0.0 alpha))))
-      (apply 'color-rgb-to-hex
-             (cl-mapcar #'(lambda (a b) (+ (* a alpha) (* b (- 1.0 alpha))))
+  (if-let* ((from-rgb (color-name-to-rgb from-color))
+            (to-rgb (color-name-to-rgb to-color))
+            (alpha (min 1.0 (max 0.0 alpha))))
+      (apply #'color-rgb-to-hex
+             (cl-mapcar (lambda (a b) (+ (* a alpha) (* b (- 1.0 alpha))))
                         from-rgb to-rgb))
-      'unspecified))
+    'unspecified))
 
 (defun eglot-inactive-regions-cleanup ()
   "Clean up inactive regions."
@@ -197,7 +192,10 @@ If the correspondend \"eglot-inactive\" face doesn't not exist yet create it."
          (eglot-inactive-face (intern eglot-inactive-face-name))
          (eglot-inactive-doc (concat (face-documentation parent-face) doc-suffix)))
     (unless (facep eglot-inactive-face)
-      (eval `(defface ,eglot-inactive-face '((t nil)) ,eglot-inactive-doc)))
+      (custom-declare-face
+       eglot-inactive-face
+       '((t nil))
+       eglot-inactive-doc))
     (set-face-foreground eglot-inactive-face eglot-inactive-fg)
     eglot-inactive-face))
 
@@ -207,10 +205,14 @@ Some mode use `default' face for both generic keywords and
 whitespace while some other uses nil for whitespace.  Either way
 we don't want to include whitespace in fontification."
   (let* ((prev-face (get-text-property (point) 'face))
-         (_ (forward-char))
-         (next-face (get-text-property (point) 'face)))
+         (next-face (progn
+		      (forward-char)
+		      (get-text-property (point) 'face))))
     (while (and (eq prev-face next-face)
-                (not (thing-at-point 'whitespace)))
+		;; what are you trying to do here?  if you want to
+		;; check if you are not on whitespace, consider
+		;; something like (looking-at-p "[^[:space:]]").
+                (not (thing-at-point 'whitespace))) 
       (setq prev-face (get-text-property (point) 'face))
       (forward-char)
       (setq next-face (get-text-property (point) 'face)))))
@@ -280,16 +282,16 @@ Useful to update colors after a face or theme change."
     (dolist (range eglot-inactive-regions--ranges)
       (let ((beg (car range))
             (end (cdr range)))
-        (cond
-         ((eq eglot-inactive-regions-style 'darken-foreground)
+        (pcase-exhaustive eglot-inactive-regions-style
+         ('darken-foreground
           (with-silent-modifications
             (put-text-property beg end 'eglot-inactive-region t))
           (font-lock-flush))
-         ((eq eglot-inactive-regions-style 'shadow-face)
+         ('shadow-face
           (let ((ov (make-overlay beg end)))
             (overlay-put ov 'face 'eglot-inactive-regions-shadow-face)
             (push ov eglot-inactive-regions--overlays)))
-         ((eq eglot-inactive-regions-style 'shade-background)
+         ('shade-background
           (let ((ov (make-overlay beg (1+ end))))
             (overlay-put ov 'face 'eglot-inactive-regions-shade-face)
             (push ov eglot-inactive-regions--overlays))))))))
@@ -320,7 +322,7 @@ Useful to update colors after a face or theme change."
 
 (defun eglot-inactive-regions--handle-notification (uri regions)
   "Update inactive REGIONS for the buffer corresponding to URI."
-  (when-let* ((path (expand-file-name (eglot--uri-to-path uri)))
+  (when-let* ((path (expand-file-name (eglot--uri-to-path uri))) ;note that this function is deprecated!
               (buffer (find-buffer-visiting path)))
       (with-current-buffer buffer
         (when eglot-inactive-regions-mode
@@ -329,7 +331,8 @@ Useful to update colors after a face or theme change."
           (setq eglot-inactive-regions--ranges '())
           (cl-loop
            for r across regions
-           for (beg . end) = (eglot--range-region r)
+           for (beg . end) = (eglot--range-region r) ;this as well!
+	   collect (cons beg end) into eglot-inactive-regions--ranges
            do
            (push (cons beg end) eglot-inactive-regions--ranges))
           (eglot-inactive-regions-refresh)))))
@@ -338,7 +341,7 @@ Useful to update colors after a face or theme change."
   "Enable inactive code capabilities for SERVER."
   (let ((base (cl-call-next-method)))
     (when (cl-find "clangd" (process-command (jsonrpc--process server))
-                   :test #'string-match)
+                   :test #'string-search)
       (setf (cl-getf (cl-getf base :textDocument)
                      :inactiveRegionsCapabilities)
             '(:inactiveRegions t)))
@@ -354,7 +357,7 @@ Useful to update colors after a face or theme change."
   (_server (_method (eql textDocument/inactiveRegions))
            &key regions textDocument &allow-other-keys)
   "Listen to clangd inactiveRegions notifications."
-  (if-let ((uri (cl-getf textDocument :uri)))
+  (if-let* ((uri (cl-getf textDocument :uri)))
       (eglot-inactive-regions--handle-notification uri regions)))
 
 (provide 'eglot-inactive-regions)

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


If anything is unclear or I misunderstood something, just ask!

>
> This addresses some of the feedback received last time:
>
> - renamed the package to eglot-inactive-regions so it's not limited to
>   clangd support only
>
> - added experimental support for ccls skippedRegions, so the rename
>   actually makes sense
>
> - turned it into a global minor mode (thanks Gerd!). Now once the mode
>   is active globally it gets automatically enabled in an eglot managed
>   buffer when an inactive region notification arrives from the LSP
>   server
>
> - moved all the user options to defcustoms
>
> I hope this rename won't cause too much disruption to my users.
> I think all the changes are for the best but let me know if I broke
> anything in the process.
>
> As always, feedback is very welcome!
>
> Cheers,
> Filippo

Re: [NonGNU ELPA] new package: eglot-inactive-regions

[Filippo Argiolas]

Philip Kaludercic <philipk@posteo.net> writes:

> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>
>> Hi all,
>>
>> a couple of weeks ago I submitted my clangd-inactive-regions package
>> NonGNU ELPA inclusion. Previous discussion led to renaming the package
>> to make it more general, so I am submitting it again.
>>
>> For whom who missed it, it's a little Eglot extension to visually style
>> inactive preprocessor branches in c/cpp code in a LSP powered way.
>>
>> You can find more at:
>> https://github.com/fargiolas/eglot-inactive-regions
>
> Here are a few comments and alternatives that you might be interested
> in:

Thanks for the review, much appreciated!
Just a few comments below.

> @@ -65,17 +66,15 @@
>  Used to mix foreground and background colors and apply to the foreground
>  face of the inactive region.  The lower the blending factor the more
>  text will look dim."
> -  :type '(float :tag "Opacity" :min 0.0 :max 1.0)
> -  :set #'eglot-inactive-regions--set-and-refresh
> -  :group 'inactive-regions)
> +  :type '(float :tag "Opacity" :min 0.0 :max 1.0) ;:min and :max have no effect, but you can use :validate
> +  :set #'eglot-inactive-regions--set-and-refresh)

No idea how I came up with those, I was sure to have used another mode
as inspiration but it seems those are pure allucinations :)

> @@ -157,13 +152,13 @@ Only applies to `shade-background' style."
>    "Linearly interpolate between two colors.
>  Blend colors FROM-COLOR and TO-COLOR with ALPHA interpolation
>  factor."
> -  (if-let ((from-rgb (color-name-to-rgb from-color))
> -           (to-rgb (color-name-to-rgb to-color))
> -           (alpha (min 1.0 (max 0.0 alpha))))
> -      (apply 'color-rgb-to-hex
> -             (cl-mapcar #'(lambda (a b) (+ (* a alpha) (* b (- 1.0 alpha))))
> +  (if-let* ((from-rgb (color-name-to-rgb from-color))
> +            (to-rgb (color-name-to-rgb to-color))
> +            (alpha (min 1.0 (max 0.0 alpha))))
> +      (apply #'color-rgb-to-hex
> +             (cl-mapcar (lambda (a b) (+ (* a alpha) (* b (- 1.0 alpha))))
>                          from-rgb to-rgb))
> -      'unspecified))
> +    'unspecified))

Why the star variant if I don't need to bind variables sequentially? is
it just for future-proofness?

> @@ -197,7 +192,10 @@ If the correspondend \"eglot-inactive\" face doesn't not exist yet create it."
>           (eglot-inactive-face (intern eglot-inactive-face-name))
>           (eglot-inactive-doc (concat (face-documentation parent-face) doc-suffix)))
>      (unless (facep eglot-inactive-face)
> -      (eval `(defface ,eglot-inactive-face '((t nil)) ,eglot-inactive-doc)))
> +      (custom-declare-face
> +       eglot-inactive-face
> +       '((t nil))
> +       eglot-inactive-doc))
>      (set-face-foreground eglot-inactive-face eglot-inactive-fg)
>      eglot-inactive-face))

Nice, I always struggle with eval quoting, definitely better with your version.

> @@ -207,10 +205,14 @@ Some mode use `default' face for both generic keywords and
>  whitespace while some other uses nil for whitespace.  Either way
>  we don't want to include whitespace in fontification."
>    (let* ((prev-face (get-text-property (point) 'face))
> -         (_ (forward-char))
> -         (next-face (get-text-property (point) 'face)))
> +         (next-face (progn
> +		      (forward-char)
> +		      (get-text-property (point) 'face))))
>      (while (and (eq prev-face next-face)
> -                (not (thing-at-point 'whitespace)))
> +		;; what are you trying to do here?  if you want to
> +		;; check if you are not on whitespace, consider
> +		;; something like (looking-at-p "[^[:space:]]").
> +                (not (thing-at-point 'whitespace))) 
>        (setq prev-face (get-text-property (point) 'face))
>        (forward-char)
>        (setq next-face (get-text-property (point) 'face)))))

Idea here is to jump to the next face change or whitespace.  I believe I
wanted to avoid applying shaded faces to empty space.  Probably I could
use a mix of `next-single-property-change' and `looking-at-p'. It's old
code I never got to review, will take a better look in the next few
days.  Maybe there's no point of skipping whitespace after all.

> @@ -280,16 +282,16 @@ Useful to update colors after a face or theme change."
>      (dolist (range eglot-inactive-regions--ranges)
>        (let ((beg (car range))
>              (end (cdr range)))
> -        (cond
> -         ((eq eglot-inactive-regions-style 'darken-foreground)
> +        (pcase-exhaustive eglot-inactive-regions-style
> +         ('darken-foreground
>            (with-silent-modifications
>              (put-text-property beg end 'eglot-inactive-region t))
>            (font-lock-flush))
> -         ((eq eglot-inactive-regions-style 'shadow-face)
> +         ('shadow-face
>            (let ((ov (make-overlay beg end)))
>              (overlay-put ov 'face 'eglot-inactive-regions-shadow-face)
>              (push ov eglot-inactive-regions--overlays)))
> -         ((eq eglot-inactive-regions-style 'shade-background)
> +         ('shade-background
>            (let ((ov (make-overlay beg (1+ end))))
>              (overlay-put ov 'face 'eglot-inactive-regions-shade-face)
>              (push ov eglot-inactive-regions--overlays))))))))

Isn't pcase overkill if no complex pattern matching is involved?

> @@ -320,7 +322,7 @@ Useful to update colors after a face or theme change."
>  
>  (defun eglot-inactive-regions--handle-notification (uri regions)
>    "Update inactive REGIONS for the buffer corresponding to URI."
> -  (when-let* ((path (expand-file-name (eglot--uri-to-path uri)))
> +  (when-let* ((path (expand-file-name (eglot--uri-to-path uri)))
>  ;note that this function is deprecated!

I know, I believe I was even involved in deprecating it. At first I was
using the new version but a user forked the repo to make it work in 29.1
where both functions are still private.

What's the proper way to handle this without losing backwards
compatibility?

> If anything is unclear or I misunderstood something, just ask!

Thanks again!

Filippo