Kyle Meyer <k...@kyleam.com> writes: >> +(defmacro org--extended-face (attributes) >> + "Make face that extends beyond end of line. >> + >> +Up to Emacs 26, all faces extended beyond end of line; getting >> +the same behaviour starting with Emacs 27 requires :extend t." >> + `(nconc ,attributes (when (>= emacs-major-version 27) '(:extend t)))) > > Two minor thoughts, neither really important: > > * Style nit-pick: s/when/and/, as the return value is of interest.
OK; I didn't know 'when' had a "for side-effect only" connotation, I was using it as a shorthand for (if COND FORM nil). > ... the main thing I wonder is whether this kludge is necessary at all. > AFAICT unconditionally including :extend in the face spec doesn't seem > to bother earlier Emacs versions. Huh. Based on the discussion for bug#37774[1][2][3][4], I had assumed this kind of kludge would be necessary, but both Emacs 25.3 and 26.3 seem to evaluate and byte-compile the following snippet with no errors: #+begin_src elisp (defface foobar '((t (:extend t))) "Test extend in 26.3" :group 'org-faces) (custom-set-faces '(foobar ((t (:extend nil))) t)) #+end_src Obviously I'm all for removing this shim if it's not needed. From some light testing it looks like removing it breaks the customization widget, though? I'll post a revised patch as soon as someone can confirm or refute my observations. [1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#41 [2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#53 [3] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#71 [4] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#80 > Even if there is a reason to avoid > :extend on older versions, it's perhaps an overkill to add a > compatibility macro for just one spot. Right, that macro dates from an earlier patch, where I unconditionally set :extend t on a bunch of faces[5]. I agree that it's overkill for just org-block. [5] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42184#17 > If org--extended-face stays, org-face.el should be adjusted to load > org-compat.el. (`make single' flags this.) (Ugh, I actually got that right in earlier patches.) Thanks for the review. As I said, I'll post an updated patch as soon as someone confirms or refutes my impression that :extend messes with the Customize widget for Emacs <27.
