fix: test that face is specified before computing its inherit attribute
#41
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some themes may be defined such that ='unspecified= is the car of head resulting to ~(face-attribute 'unspecified :inherit)~ being evaluated which raises the error =Invalid face: unspecified= since unspecified is an invalid face. Since we don't control how themes are defined, we should at least control that we don't end up in an error state in the htmlize logic because of an "incomplete" theme. 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 (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 and in some changes 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. Perhaps this function 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, it seems like the htmlize-face-size implementation https://github.com/hniksic/emacs-htmlize/blame/master/htmlize.el#L1029-L1050 succeeded the face-attribute implementation https://github.com/emacs-mirror/emacs/blame/master/lisp/faces.el#L450-L487 which would lead me to believe that "historic reasons" may not quite apply here but asking for context won't hurt.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some themes may be defined such that
'unspecifiedis the car of head resulting to(face-attribute 'unspecified :inherit)to be evaluated atemacs-htmlize/htmlize.el
Line 1036 in dd27bc3
'unspecifiedis 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
whileexpression to not just check that theheadvariable 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-sizeworks BTW since I find the function difficult to reason about. I've ended up retrofitting it with a bunch ofmessagecalls 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 theface-list,headandtailvariables are not always explicitly expressed as they, in some cases, happen through side-effects ofsetcdrandpopwhich 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 usingface-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).