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

add-face-text-property requires emacs 24.4 #1634

Closed
JohnLunzer opened this issue Jun 18, 2018 · 12 comments

Comments

@JohnLunzer
Copy link

commented Jun 18, 2018

Similarly to previously reported issue, ivy.el uses add-face-text-property which was introduced in 24.4, but ivy.el currently states that it requires 24.1.

I ran into an error when using ivy-rich's version of ivy-switch-buffer, when I type in a search pattern that doesn't exist. ivy-rich doesn't call add-face-text-property, so I believe ivy is the culprit.

It looks like in a couple places in ivy.el that there is a check for add-face-text-property and using an alternative when appropriate but inivy--insert-prompt it looks like it is being used without an alternative option. Though, I'm not entirely sure this is where I'm seeing my error.

Any advice on how I can eliminate all calls to add-face-text-property? the one in ivy--insert-prompt looks tricky because it says it's actually removing some property so I'm not sure I can just drop in font-lock-append-text-property as is being done elsewhere.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

As an aside, the docstring of ivy-display-style states:

This setting depends on `add-face-text-property' - a C function
available as of Emacs 24.5.  Fancy style will render poorly in
earlier versions of Emacs.

and its default value is predicated on (version< emacs-version "24.5"), but add-face-text-property was actually added in Emacs 24.4; see M-:(view-emacs-news "24.4")RET and commit 708e05f6d1b39313a63e34a5b4e1a16ae809ae25. (ivy) Installation also speaks of version 24.5, rather than 24.4.

@JohnLunzer

This comment has been minimized.

Copy link
Author

commented Jun 18, 2018

Well, against my better judgement I tried to modify the call to add-face-text-property inside ivy--insert-prompt to call ivy-add-face-text-property instead and it seems to work. My ivy-switch-buffer no longer breaks on no matches and recovers when I backspace to a matching string.

My exact call is:

(ivy-add-face-text-property (minibuffer-prompt-end) (line-end-position) 'ivy-prompt-match (current-buffer))

If somebody could test this change to ensure I'm not just hiding the error I would appreciate it.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

@JohnLunzer

I'm not entirely sure this is where I'm seeing my error.

You can get an error backtrace by first enabling M-xtoggle-debug-on-errorRET.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

@abo-abo Another thing that's confusing me is the function ivy--add-face:

(defun ivy--add-face (str face)
  "Propertize STR with FACE.
`font-lock-append-text-property' is used, since it's better than
`propertize' or `add-face-text-property' in this case."
  (require 'colir)
  (condition-case nil
      (progn
        (colir-blend-face-background 0 (length str) face str)
        (let ((foreground (face-foreground face)))
          (when foreground
            (add-face-text-property
             0 (length str)
             `(:foreground ,foreground)
             nil
             str))))
    (error
     (ignore-errors
       (font-lock-append-text-property 0 (length str) 'face face str))))
  str)

Its docstring claims that font-lock-append-text-property is preferred over add-face-text-property here, and yet the implementation uses the former only as a fallback for the latter. Which is correct, the docstring or the implementation?

@abo-abo abo-abo closed this in bb73d64 Jun 18, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 18, 2018

Thanks

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 18, 2018

@basil-conto

Its docstring claims that font-lock-append-text-property is preferred over add-face-text-property here, and yet the implementation uses the former only as a fallback for the latter. Which is correct, the docstring or the implementation?

It's kind of both, I think. add-face-text-property bit is only valid for faces with custom foregrounds, a rare case. And the docstring refers to using font-lock-append-text-property and not add-face-text-property again in the fallback case. Feel free to make the doc more clear, basically it says we should not try to call add-face-text-property twice instead of once. It's also possible to remove the confusing part of the doc altogether.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

@abo-abo

It's kind of both, I think. add-face-text-property bit is only valid for faces with custom foregrounds, a rare case. And the docstring refers to using font-lock-append-text-property and not add-face-text-property again in the fallback case. Feel free to make the doc more clear, basically it says we should not try to call add-face-text-property twice instead of once. It's also possible to remove the confusing part of the doc altogether.

Sorry, I don't understand what you mean.

  1. font-lock-append-text-property is only called if there is an error, most likely because add-face-text-property is not defined as a function
  2. The docstring says nothing about falling back on font-lock-append-text-property, but rather suggests that font-lock-append-text-property is the preferred function for propertising STR with FACE, for some reason.
  3. Why/when would add-face-text-property get called twice? If it were used both in the BODYFORM and HANDLERS of the condition-case?

What am I missing?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

A follow-up question is, if font-lock-append-text-property is only intended to be used when add-face-text-property is not defined as a function, then why don't we use ivy-add-face-text-property here?

basil-conto added a commit to basil-conto/swiper that referenced this issue Jun 18, 2018
Use compatibility shim for add-face-text-property
ivy.el (ivy--add-face): Minor simplification.
(ivy-add-face-text-property): Expand arglist to that of
add-face-text-property, but with the order of the last two args
flipped.  Move compatibility check from runtime to evaluation time.
Use font-lock-prepend-text-property instead of
font-lock-append-text-property when applicable.
(ivy-display-style):
doc/ivy.org (Installation, Defcustoms):
Mention Emacs 24.4 instead of 24.5.
counsel.el (counsel-git-grep-transformer):
ivy-overlay.el (ivy-display-function-overlay):
Replace add-face-text-property with ivy-add-face-text-property.

Re: abo-abo#1634
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

The docstring says nothing about falling back on font-lock-append-text-property, but rather suggests that font-lock-append-text-property is the preferred function for propertising STR with FACE, for some reason.

I think the reason is the historical deterioration of the docstring. Initially, no color blending was available and font-lock-append-text-property was the whole body.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2018

I think the reason is the historical deterioration of the docstring. Initially, no color blending was available and font-lock-append-text-property was the whole body.

OK, so should that sentence be removed completely then? Why font-lock-append-text-property instead of font-lock-prepend-text-property, which AIUI is closer to what add-face-text-property does with an APPEND argument of nil?

basil-conto added a commit to basil-conto/swiper that referenced this issue Jun 19, 2018
Use compatibility shim for add-face-text-property
ivy.el (ivy--add-face): Minor simplification.
(ivy-add-face-text-property): Expand arglist to that of
add-face-text-property, but with the order of the last two args
flipped.  Move compatibility check from runtime to evaluation time.
Use font-lock-prepend-text-property instead of
font-lock-append-text-property when applicable.
(ivy-append-face): Replace font-lock-append-text-property with
ivy-add-face-text-property.
(ivy-display-style):
doc/ivy.org (Installation, Defcustoms):
Mention Emacs 24.4 instead of 24.5.
counsel.el (counsel-git-grep-transformer):
ivy-overlay.el (ivy-display-function-overlay):
Replace add-face-text-property with ivy-add-face-text-property.

Re: abo-abo#1634
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

OK, so should that sentence be removed completely then?

Yes, seems like the best solution:)

OK, so should that sentence be removed completely then? Why font-lock-append-text-property instead of font-lock-prepend-text-property, which AIUI is closer to what add-face-text-property does with an APPEND argument of nil?

Ivy tries to show the candidates in the minibuffer with their existing text properties while adding its own text properties to highlight matches. It's not clear if an append or a prepend is more appropriate, because it's not clear if the original text properties or the highlights are more important for the user.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2018

Yes, seems like the best solution:)

OK, I'll add that to a future cleanup PR if no-one changes it before then.

Ivy tries to show the candidates in the minibuffer with their existing text properties while adding its own text properties to highlight matches. It's not clear if an append or a prepend is more appropriate, because it's not clear if the original text properties or the highlights are more important for the user.

OK, I'll leave it alone then. :)

basil-conto added a commit to basil-conto/swiper that referenced this issue Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.