Support variable-pitch face for plain text messages

[phil-s]

Closes #174.

[phil-s]

Marking as a draft pending more testing time, in case I spot any additional special cases.

[phil-s]

I wonder whether ement-room-shr-use-fonts should be renamed/aliased to something like ement-room-use-variable-pitch? With the changes in this PR the user option is no longer strictly for shr usage, and it might make it easier for users to find it via completion. (As far as this particular option is concerned, I'm not sure the user really needs to know that shr is involved.)

[alphapapa]

I wonder whether ement-room-shr-use-fonts should be renamed/aliased to something like ement-room-use-variable-pitch? With the changes in this PR the user option is no longer strictly for shr usage, and it might make it easier for users to find it via completion. (As far as this particular option is concerned, I'm not sure the user really needs to know that shr is involved.)

Seems like a good idea.

[phil-s]

Cool, I've now done that.

I'll need to deal with 'emote' messages too:

 (:content
  (body . "<message that was rendered in monospace>")
  (m.mentions)
  (msgtype . "m.emote"))
 (:origin-server-ts . 1701224341051)
 (:type . "m.room.message")

I'm not sure whether to extend the (equal msgtype "m.text") test, or if it's better to drop it entirely.

[phil-s]

I think dropping the test should be fine? Only the body event formatters for ?b and ?B call this code, and both are dealing with a value returned by ement-room--format-message-body... presumably at that point there's no reason for me to be acting conditionally on msgtype?

(I've now pushed a fixup commit to do this.)

[phil-s]

Ignore the following; it was my own fault.

---

Hmm, plain text messages in the Notifications buffer are ending up monospaced, but I'm not sure why.

In the notifications buffer describe-char shows me:

ftcrhb:-1ASC-Droid Sans Mono Dotted-regular-normal-normal-*-16-*-*-*-m-0-iso10646-1 (#x2B)

There are text properties here:
  event                [Show]
  face                 (:inherit (ement-room-mention ement-room-message-text shr-text))
  fontified            t
  line-prefix          [Show]
  room                 [Show]
  session              [Show]
  wrap-prefix          [Show]

Whereas in the room buffer the same event text is rendered in variable-pitch:

ftcrhb:-PfEd-DejaVu Sans-regular-normal-normal-*-18-*-*-*-*-0-iso10646-1 (#x2B)

There are text properties here:
  face                 (:inherit (ement-room-mention ement-room-message-text shr-text))
  fontified            t
  line-prefix          [Show]
  wrap-prefix          [Show]

[phil-s]

Well that last one was my own fault, and local to my config. It occurred to me to check face-remapping-alist to see if that explained what I was seeing, whereupon I realised that I'd previously overridden ement-room-mention in ement-notifications-mode-hook to avoid some unwanted highlighting in the Notifications buffer:

;; Don't highlight All Of The Things.
(face-remap-add-relative 'ement-room-mention :inherit 'default)

Which I've fixed via a more specific override:

;; Don't highlight All Of The Things.
;; Edit: Oooh, no, this isn't even valid:
;; (face-remap-add-relative 'ement-room-mention :background nil)
;; But this works:
(face-remap-add-relative
  'ement-room-mention :background (face-attribute 'default :background))

[phil-s]

I've tested a bunch of different things and not spotted any more issues, so this PR is no longer a Draft.

[phil-s]

Force-pushed to squash the prior fixup commit.

[phil-s]

Bah, and now I spot something else to tidy up...

[phil-s]

Have done more testing, and am happy with this now.

[alphapapa]

Thanks, I like how simple this patch is. :) How do you think we should describe it in the changelog?

[phil-s]

How do you think we should describe it in the changelog?

How about:

Option: ement-room-use-variable-pitch (previously named ement-room-shr-use-fonts) enables variable-pitch fonts for all message types. (This option previously supported formatted messages, but now works for plain text messages as well.) Note: users who have customized the ement-room-message-text face to be variable-pitch should revert that change, as it causes problems for formatted messages, and is no longer necessary.

[alphapapa]

Thanks. If you don't mind updating the changelog, then I'll merge this.

[phil-s]

As another micro optimisation, seeing as how the following won't ever vary in a given session:

(or (and (facep 'shr-text) 'shr-text)
    'variable-pitch))

I thought I might defconst ement-room-variable-pitch-face with that value.

[alphapapa]

As another micro optimisation, seeing as how the following won't ever vary in a given session:

(or (and (facep 'shr-text) 'shr-text)
    'variable-pitch))

I thought I might defconst ement-room-variable-pitch-face with that value.

Yeah, I think that makes sense. You might need to verify that shr is loaded first.

[phil-s]

You might need to verify that shr is loaded first.

That's all good -- ement-room.el has a (require 'shr).

[phil-s]

I'm investigating an issue which is probably connected to this in some way. Actual rendering appears fine, but something in my config is currently triggering a lot of face-attribute-related messages; so don't merge this in the interim, just in case there's a bug to fix.

Edit: Ok, I think I see what's going on, and it's something in my config entirely unconnected to this PR. I just need to do some more testing...

[phil-s]

Yep, that last comment/issue was totally unrelated. Nothing to see here!

But in case anyone is interested...

I was seeing floods of this in Messages:

Invalid face attribute :background nil
Invalid face reference: ement-room-mention
Invalid face attribute :inherit (ement-room-mention ement-room-message-text)

And it took me a stupidly long time to recognise the literal-ness of the first message.

;; Bad:
(face-remap-add-relative 'ement-room-mention :background nil)
;; Good:
(face-remap-add-relative 'ement-room-mention :background 'unspecified)

Edit: Hmm, in fact that prevented the errors but didn't actually suppress the background colour, so I went with this in the end:

(face-remap-add-relative
  'ement-room-mention :background (face-attribute 'default :background))

This is happening in ement-notifications-mode-hook. If you happen to know a cleaner way to suppress the background without explicitly setting a colour, I'd be keen to know.

Looking at (elisp)Face Attributes it seems that a slightly better value is 'reset which IIUC means exactly the same thing as (face-attribute 'default :background), but more succinctly. It seems to me that the 'unspecified value was just ignored by the face remapping process, rather than actually making the attribute unspecified. (Nope; reset just gets me "Invalid face attribute :background reset".)

Edit 2: And to demonstrate why setting a specific background is a pain, I subsequently ended up with the following in order to support switching themes (which I do depending on whether I'm working indoors or outdoors):

(let ((buf (current-buffer)))
  (cl-flet ((update-faces (&optional _theme)
              (with-current-buffer buf
                (face-remap-add-relative
                 'ement-room-mention
                 :background (face-attribute 'default :background)))))
    (update-faces)
    (add-hook 'enable-theme-functions #'update-faces nil :local)))

But then I simplified all that to:
(face-remap-add-relative 'ement-room-mention :inherit 'ement-room-message-text)

Edit 3: Nope, still not right! Latest attempt:

(setq-local face-remapping-alist nil)
(face-remap-add-relative 'ement-room-mention
                         :inherit '(variable-pitch-text default))

[phil-s]

AFAIK, this one is good to go.

[alphapapa]

I'd like to release v0.14 for tomorrow, so I'll merge this for v0.15 afterward, so it can be tested on master for a while.

[phil-s]

Rebased over current master.

[phil-s]

Rebased over current master.

[alphapapa]

Merged. Thanks for your work and your patience.