[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

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

[Philip Kaludercic]

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

> 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?

The star-less version has been recently deprecated on the master branch.
And despite not using the variables in subsequent terms, I like to
imagine that if-let* makes more sense since the terms are still
explicitly evaluated in the order in which they are listed.
Furthermore, if you take a look at if-let, you will see that it is
implemented in terms of if-let* which means that the bindings remain
visible even if the name doesn't indicate that.

>> @@ -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.

Just double check that it works as intended.

>> @@ -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.

I would suspect that it would be easier and more efficient not to have
to think about having multiple separate properties.

>> @@ -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?

It compiles down to the same (byte)code, so there is no overhead

(macroexpand-all
  '(pcase foo
     ('one 1)
     ('two 2)))
;=>  (cond ((eq foo 'one) (let nil 1)) ((eq foo 'two) (let nil 2)))

and `pcase-exhaustive' raises an error earlier if the variable is in an
unexpected state.

>> @@ -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?

I would try something of the form like

  (if (fboundp 'new-function)
      (new-function ...)
    (old-function ...))

If on the other hand there has already been a new release of Eglot with
these commands, then just depend on that version and the issue would
resolve itself.

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

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

[Filippo Argiolas]

Philip Kaludercic <philipk@posteo.net> writes:

> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>
>> 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?
>
> The star-less version has been recently deprecated on the master branch.
> And despite not using the variables in subsequent terms, I like to
> imagine that if-let* makes more sense since the terms are still
> explicitly evaluated in the order in which they are listed.
> Furthermore, if you take a look at if-let, you will see that it is
> implemented in terms of if-let* which means that the bindings remain
> visible even if the name doesn't indicate that.
>
>>> @@ -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.
>
> Just double check that it works as intended.
>
>>> @@ -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.
>
> I would suspect that it would be easier and more efficient not to have
> to think about having multiple separate properties.

Removed that function altogether, just using
`next-single-property-change' seems good.

>>> @@ -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?
>
> It compiles down to the same (byte)code, so there is no overhead
>
> (macroexpand-all
>   '(pcase foo
>      ('one 1)
>      ('two 2)))
> ;=>  (cond ((eq foo 'one) (let nil 1)) ((eq foo 'two) (let nil 2)))
>
> and `pcase-exhaustive' raises an error earlier if the variable is in an
> unexpected state.
>
>>> @@ -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?
>
> I would try something of the form like
>
>   (if (fboundp 'new-function)
>       (new-function ...)
>     (old-function ...))
>
> If on the other hand there has already been a new release of Eglot with
> these commands, then just depend on that version and the issue would
> resolve itself.
>


Thanks again for the review and for the useful explanations. I pushed
your suggested changes to master. Will test it a bit and bump version if
everything seems ok.

Filippo

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

[Filippo Argiolas]

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

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>
>> I would try something of the form like
>>
>>   (if (fboundp 'new-function)
>>       (new-function ...)
>>     (old-function ...))
>>
>> If on the other hand there has already been a new release of Eglot with
>> these commands, then just depend on that version and the issue would
>> resolve itself.
>>
>
>
> Thanks again for the review and for the useful explanations. I pushed
> your suggested changes to master. Will test it a bit and bump version if
> everything seems ok.
>

Everything seems fine after the suggested changes.
Bumped version to 0.6.1, let me know if I can do anything else!

Thanks,
Filippo

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

[Filippo Argiolas]

Philip Kaludercic <philipk@posteo.net> writes:

> I would try something of the form like
>
>   (if (fboundp 'new-function)
>       (new-function ...)
>     (old-function ...))
>
> If on the other hand there has already been a new release of Eglot with
> these commands, then just depend on that version and the issue would
> resolve itself.
>

CC-ing João.

I pushed the suggested change but I still get the compile warnings about
deprecated functions. I'd like to keep supporting emacs-29, should I
just disregard the warning?

Sorry for the ignorance, how would the dependency on a specific eglot
version work now that it's in core?
Does it require the users to have additional repos enabled?

Sounds strange to me that we have macros to deprecate functions but no
mechanism to automatically switch to the new one if it's just a
rename. Am I missing something obvious?

Thanks!

Filippo

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

[Filippo Argiolas]

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

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> I would try something of the form like
>>
>>   (if (fboundp 'new-function)
>>       (new-function ...)
>>     (old-function ...))
>>
>> If on the other hand there has already been a new release of Eglot with
>> these commands, then just depend on that version and the issue would
>> resolve itself.
>>
>
> CC-ing João.
>
> I pushed the suggested change but I still get the compile warnings about
> deprecated functions. I'd like to keep supporting emacs-29, should I
> just disregard the warning?
>
> Sorry for the ignorance, how would the dependency on a specific eglot
> version work now that it's in core?
> Does it require the users to have additional repos enabled?
>
> Sounds strange to me that we have macros to deprecate functions but no
> mechanism to automatically switch to the new one if it's just a
> rename. Am I missing something obvious?
>
> Thanks!
>
> Filippo

How about something like:

  ;; fallback to deprecated eglot functions
  (when (version< emacs-version "30")
    (defalias 'eglot-uri-to-path 'eglot--uri-to-path)
    (defalias 'eglot-range-region 'eglot--range-region))

or even something similar with a check on eglot version?

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

[Philip Kaludercic]

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

> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> I would try something of the form like
>>>
>>>   (if (fboundp 'new-function)
>>>       (new-function ...)
>>>     (old-function ...))
>>>
>>> If on the other hand there has already been a new release of Eglot with
>>> these commands, then just depend on that version and the issue would
>>> resolve itself.
>>>
>>
>> CC-ing João.
>>
>> I pushed the suggested change but I still get the compile warnings about
>> deprecated functions. I'd like to keep supporting emacs-29, should I
>> just disregard the warning?
>>
>> Sorry for the ignorance, how would the dependency on a specific eglot
>> version work now that it's in core?
>> Does it require the users to have additional repos enabled?
>>
>> Sounds strange to me that we have macros to deprecate functions but no
>> mechanism to automatically switch to the new one if it's just a
>> rename. Am I missing something obvious?
>>
>> Thanks!
>>
>> Filippo
>
> How about something like:
>
>   ;; fallback to deprecated eglot functions
>   (when (version< emacs-version "30")
>     (defalias 'eglot-uri-to-path 'eglot--uri-to-path)
>     (defalias 'eglot-range-region 'eglot--range-region))
>
> or even something similar with a check on eglot version?

That can be dangerous if other packages do fboundp checks and infer too
much from that.  I would try to see if adding `declare-function's could
help suppress the warnings?