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