GNU bug report logs - #41810
28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix

Previous Next

Full log

Message #23 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 41810 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Sun, 21 Jun 2020 19:32:17 +0100

Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:

> OK, here is a patch that I think should be good to push, tested against
> Emacs 28 and 26.3.

Thanks, just some minor comments from me.

> +(defun adaptive-wrap--prefix-face (fcp beg end)
> +  (or (get-text-property 0 'face fcp)
> +      ;; If the last character is a newline and has a face that
> +      ;; extends beyond EOL, assume that this face spans the whole
> +      ;; line and apply it to the prefix to preserve the "block"
> +      ;; visual effect.
> +      ;; NB: the face might not actually span the whole line: see for
> +      ;; example removed lines in diff-mode, where the first character
> +      ;; has the diff-indicator-removed face, while the rest of the
> +      ;; line has the diff-removed face.
> +      (when (= (char-before end) ?\n)
> +        (let ((eol-face (get-text-property (1- end) 'face)))

Is it guaranteed that (< (point-min) end (1+ (point-max)))?
Otherwise = and get-text-property will barf.

> +          (when (and eol-face (adaptive-wrap--face-extends eol-face))
> +            eol-face)))))

Nit: Can't the when+and be replaced with a single and?

> +(defun adaptive-wrap--prefix (fcp)
> +  (let ((fcp-len (string-width fcp)))
> +    (cond
> +     ((= 0 adaptive-wrap-extra-indent)
> +      fcp)
> +     ((< 0 adaptive-wrap-extra-indent)
> +      (concat
> +       fcp
> +       (make-string adaptive-wrap-extra-indent
> +                    (if (< 0 fcp-len)
> +                        (string-to-char (substring fcp -1))
> +                      ?\ ))))

Please change this to ?\s regardless of whether the second patch is
installed.

> +     ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
> +      (substring fcp
> +                 0
> +                 (+ adaptive-wrap-extra-indent fcp-len)))
> +     (t
> +      ""))))

> Open questions:
>
> - The (or … (when … (let … (when (and …))))) chain looks clumsy but I
>   don't really know how to improve it off the top of my head.  Maybe a
>   when-let or two would help?  That'd mean requiring Emacs 25.1 though.

Apart from the redundant when, I think it's fine.  If you really want
to shave off some forms you can write e.g.

  (defun adaptive-wrap--prefix-face (fcp beg end)
    (or (get-text-property 0 'face fcp)
        (let ((face (and (= (char-before end) ?\n)
                         (get-text-property (1- end) 'face))))
          (and face (adaptive-wrap--face-extends face) face))))

or

  (defun adaptive-wrap--prefix-face (fcp beg end)
    (cond ((get-text-property 0 'face fcp))
          ((= (char-before end) ?\n)
           (let ((face ...))
             (and face (adaptive-wrap--face-extends face) face)))))

Thanks,

-- 
Basil

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.