search

bastibe / annotate.el

Annotate.el
Other
388 stars 20 forks source link

Ensure `face-attribute` is invoke on faces only #63

Closed willthefrog closed 4 years ago

willthefrog commented 4 years ago

somehow all-faces may contain inline face attributes or lists, and face-attribute will error out on non face objects

bastibe commented 4 years ago

Thank you for your pull request!

Out of curiosity, in which case is this bug triggered?

cage2 commented 4 years ago

Hello willthefrog!

Many thanks for your patch from me too!

Hello Bastian!

Thank you for your pull request!

I am able to confirm that this is a bug, i did not read the documentation properly when i wrote that portion of the code.

When i check the face of a portion of buffer to try to get the right positioning of the annotation i used: '(get-text-property changed-face-pos 'face)

I assumed this function returned a symbol (the face of the text) or nil: turned out i was wrong.

According to the documentation, the code above can returns a symbol, a plist or even list of symbols!

(this has been pointed out already from willthefrog)

https://www.gnu.org/software/emacs/manual/html_mono/elisp.html#Special-Properties

The patch proposed goes in the right direction but i think it is not enough: basically if we used '(remove-if-not #'facep ...)' we will skip faces that are not a symbol but lists, as facep return nil if the argument is not a named face.

I propose another patch that extend the one of this PR and try to take into account the different type the function 'get-text-property' returns.

Out of curiosity, in which case is this bug triggered?

I think i was able to reproduce the issue annotating a text surrounded by '~' in a ORG document, but, of course this is dependent, from the theme i am using.

Bye! C.

cage2 commented 4 years ago

Hello everyone!

I pushed a patch (#64), looking forward for your opinions/comments!

Bye! C.

willthefrog commented 4 years ago

I agree, this was only meant to be a quick fix to get annotations work again.

Closing now that it was superseded by #64.

BTW thanks a lot for this amazing project.

cage2 commented 4 years ago

Hello!

thanks a lot for this amazing project.

Happy that you find it useful! :)

We are near, i guess, to merge the patch. If both you and the maintainer agree i would like to mention you in the news file, are you OK? Is willthefrog OK as reference?

Bye! C.