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