Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Helm odd highlighting with emacs-27 #2213

Closed
thierryvolpiatto opened this issue Oct 25, 2019 · 17 comments
Closed

Helm odd highlighting with emacs-27 #2213

thierryvolpiatto opened this issue Oct 25, 2019 · 17 comments
Labels

Comments

@thierryvolpiatto
Copy link
Member

Expected behavior

Same behavior as with emacs-26

Actual behavior (from emacs-helm.sh if possible, see note at the bottom)

Face are not extended to EOL due to emacs bug #37774

Same bug as magit #3986

Workaround: Use emacs-26 or use this code in init file:

;; Fix issue with the new :extend face attribute in emacs-27
;; Prefer to extend to EOL as in previous emacs.
(defun tv/extend-faces-matching (regexp)
  (cl-loop for f in (face-list)
           for face = (symbol-name f)
           when (and (string-match regexp face)
                     (eq (face-attribute f :extend t 'default)
                         'unspecified))
           do (set-face-attribute f nil :extend t)))

(when (fboundp 'set-face-extend)
  (with-eval-after-load "magit"
    (tv/extend-faces-matching "\\`magit"))
  (with-eval-after-load "helm"
    (tv/extend-faces-matching "\\`helm")))
@thierryvolpiatto
Copy link
Member Author

UPDATE: With last emacs-27 (16 December) the code above is only working on some faces, many faces in helm being now broken, for now don't know what to do to fix these faces (helm-header, symlinks etc...).

@thierryvolpiatto
Copy link
Member Author

I seems that now it is no more possible to add a face with :extend t to text displayed with a display property, this affect helm for sources with a header-name attribute, symlinks in HFF, Magit log etc...
The culprit commit in emacs-27 is:
dd3f2130cf2 * A better fix for extension of overlay string's faces

@thierryvolpiatto
Copy link
Member Author

Here a recipe:

(defun insert-string-with-display-prop (str disp)
  (let ((disp-str (concat str disp))
        ov)
    (insert str)
    (overlay-put (setq ov (make-overlay (point-at-bol) (point-at-eol)))
                 'display disp-str)
    (insert "\n")
    (add-face-text-property (overlay-start ov) (overlay-end ov)
                            '((:background "Blue")))))

(with-current-buffer (get-buffer-create "*test*")
  (insert-string-with-display-prop "foo" " (with more info)"))

With emacs-27 prior "dd3f2130cf2":
Capture d’écran_2019-12-16_09-43-30

After "dd3f2130cf2" there is no face, still can't provide a recipe with the face extending from eol to end of screen.

@thierryvolpiatto
Copy link
Member Author

Here screenshot with same code as above in emacs-27 master
Capture d’écran_2019-12-16_12-31-12
On the blue frame the output of describe-text-properties.

@thierryvolpiatto
Copy link
Member Author

Fixed now by commit 1e240a0ecdc in emacs master branch.

@deviantfero
Copy link

I'm still seeing this issue, I have to manually add :extend t in my theme to see lines properly in helm, should I have to do that from emacs 27 onwards?

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Jun 29, 2020 via email

@manuel-uberti
Copy link
Contributor

@deviantfero It could actually just depend on the theme you are using. For instance, I am using modus-operandi which extends the necessary Helm faces for Emacs 27.

@dgutov
Copy link

dgutov commented Jun 29, 2020

Helm should add :extend t to the faces that need it, then the themes wouldn't need to do that anymore.

And you should probably use the same trick as Magit here: https://github.com/magit/magit/blob/master/lisp/magit-diff.el#L492

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Jun 29, 2020 via email

@dgutov
Copy link

dgutov commented Jun 29, 2020

That ship has sailed, unfortunately.

But it's still better to do in one place, instead of putting it on theme authors' shoulders.

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Jun 29, 2020 via email

@thierryvolpiatto
Copy link
Member Author

Now fixed all face with :extend

@manuel-uberti
Copy link
Contributor

Thank you @thierryvolpiatto.

Also, FYI @protesilaos, you may want to update your themes.

@protesilaos
Copy link

Thank you @manuel-uberti! Will review this thread and act accordingly.

protesilaos added a commit to protesilaos/modus-themes that referenced this issue Jun 29, 2020
A recent commit in Helm adds the necessary ':extend' attribute to the
relevant faces (only relevant for >= Emacs27).  See
emacs-helm/helm@d19592f

This attribute is always retained regardless of the active theme, unless
it is explicitly overridden.  Meaning that support for it on the theme
side is no longer required.

Thanks to Manuel Uberti for bringing this issue to my attention:
emacs-helm/helm#2213
@thierryvolpiatto
Copy link
Member Author

Thanks, let me know if some faces are not working, I may have done errors when rewriting.

@manuel-uberti
Copy link
Contributor

I updated both Helm and the Modus themes this morning and everything seems to look good as expected. I'll report here if something's missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants