nashamri / spacemacs-theme

Light and dark theme for spacemacs that supports GUI and terminal
GNU General Public License v3.0
596 stars 113 forks source link

Update org-column face to use default height #183

Closed dankessler closed 3 years ago

dankessler commented 3 years ago

If using spacemacs-theme with spacemacs-theme-org-height set to t, then org-mode headings of level 1-3 will have non-standard heights so that higher level headlines "stick out" more, which is aesthetically pleasing. However, if one enables org-columns (with e.g., C-c C-x C-c), then things will be misaligned (see this issue on spacemacs repo), since the column headers are (likely) using the standard face height, but any level 1-3 headers that appear in the column view table will have differing heights.

The org-column face is overlaid on top of org-level-{1,2,3} but any unspecified lower attributes "shine through" and thus different rows of the "table" generated by org-columns have different height and so things don't line up properly.

This commit explicitly sets the :height: attribute of the org-column face to whatever the :height of the default face is.

dankessler commented 3 years ago

After using this fix for a few days, I think it's still better than the current behavior, but I was annoyed to discover that if changing the overall text scaling (which I assume changes the height of the default face), then the text in columns do not track the rescaling (although the column headers do) and things can once again get misaligned.

I might need to learn a bit more about how face properties work with multiple overlays and how they interact with text properties to come up with a more robust fix (although the manual says overlays trump text properties, so hopefully I can ignore that). From examining a high level headline with org-columns active, I see that it has both an overlay (with face ((:foreground "#4f97d7") (:family "Source Code Pro") org-column org-level-1) and text property with face (org-superstar-header-bullet org-level-1). As the manual suggests, let's assume that text property isn't doing anything since the overlay will have priority.

The way I read the documentation for overlays, specifically the face property, suggests that the result will be an "aggregate" of the faces listed; earlier ones have priority. If org-column has property :height set to 1.0 and org-level-1 has :height set to 1.3, I would expect that the resultant :height property is aggregated at 1.0 (since org-column comes earlier in the list). However, from a bit of experimentation, it seems that the effect height is actually the product: if I set org-column's :height to 1.1, I can make the heading slightly larger than before, or if I set it to 0.8 I can make it smaller than it was previously. This suggests aggregation of the :height attribute is happening through multiplication.

In my PR, I think I was exploiting the property that if :height is set to an integer value, and it is associated with a higher priority face, then it "wins." However, I was hardcoding the :height of org-column to whatever the default height was at startup, so if that value is later changed, org-column will not track it.

I think I've come up with a slightly more robust solution, wherein the org-column face has :height attribute set to an anonymous function that returns the default :height. If one changes the size of the default font (or rescales the entire frame in spacemacs), then org-column will adjust accordingly. However, if one uses text-rescaling (which uses face-remapping but leaves the default :height alone, then it doesn't track. So far I've been unsuccessful in my attempt to find some elisp I can call to return the buffer-local value of the default face's :height property (after adjusting for face-remap). I suppose it's possible to do this manually, but it seems like there's got to be a better way.

dankessler commented 3 years ago

After a bit more digging, I'm back with a new idea that also fixes behavior when text-scaling is used. If the org-column face has its :inherit property set to 'default, then it inherits all of the default face's attributes, which will include the integer-valued :height which will overwrite the relative height of org-level-1, and it appears that this is sufficiently "dynamic" and will look up remapping that happens with face-remap.

The downside is that the 'default face has several other properties set, e.g., weight is nil, and I can't figure out a way to selectively inherit just the height in a way that is sufficiently dynamic to track face-remap. As a result, other properties from org-level-1, like weight set to bold, also gets overwritten by default.

Probably the most delicate fix here is to only do this inheritance if the theme is configured to have variable heights for org-levels, since in this case it's better to clobber the properties in order to have a usable column view, whereas it won't change the behavior at all for users who already disable variable heights for org-levels.

nashamri commented 3 years ago

@dankessler Hello there, and thanks for your contribution and all of this investigative work :+1: