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

[Filippo Argiolas <filippo.argiolas@gmail.com>]

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