hniksic / emacs-htmlize

Convert buffer text and decorations to HTML.
192 stars 45 forks source link

fix: test that face is specified before computing its `inherit` attribute #41

Open vidbina opened 2 years ago

vidbina commented 2 years ago

Some themes may be defined such that 'unspecified is the car of head resulting to (face-attribute 'unspecified :inherit) to be evaluated at https://github.com/hniksic/emacs-htmlize/blob/dd27bc3f26efd728f2b1f01f9e4ac4f61f2ffbf9/htmlize.el#L1036 which raises the error "Invalid face: unspecified" since 'unspecified is an invalid face.

Since we don't control how themes are defined, I propose that we at least control that we don't end up in an error state in the htmlize logic because of an "incomplete" theme. This is done by expanding the test in the while expression to not just check that the head variable is set, but also checking that its car is not 'unspecified (thus eliminating us ending up in that error case).

TL;DR

DISCLAIMER: I do not fully understand how htmlize-face-size works BTW since I find the function difficult to reason about. I've ended up retrofitting it with a bunch of message calls while debugging my issue (yes, I'm "debugging by printf" like a shameless noob 🙊) and am really struggling with some of the state changes that are taking place. Changes to the face-list, head and tail variables are not always explicitly expressed as they, in some cases, happen through side-effects of setcdr and pop which complicate reasoning about this logic.

Looking at the implementation, I kept wondering how (htmlize-face-size face) relates to calling (face-attribute face :height nil t) which in my setup returns the same value. Please be so kind to shed some light on the difference between the two approaches to obtaining the face size. The initial assumption from my end was that this function perhaps exists for historic reasons, in which case we may be able to refactor this out for a simpler implementation (e.g.: just using face-attribute). Looking at the git blame, however; it seems like the htmlize-face-size implementation succeeded the face-attribute implementation which would lead me to believe that "historic reasons" may not quite apply here but I'm guessing that asking for context won't hurt (me at least).