Allow customizing faces for queries and non-highlighted channels

[FrostyX]

At this point, we have a possibility to customize the color of a channel (in the
tracking segment of a modeline) where somebody mentioned our name. We use the
same face as for printing our name in the message itself
(circe-highlight-nick-face).

This is IMHO not sufficient because new personal messages are as important as
mentions in a channel and they can be easily missed when shown in the
default (for me gray) color. I am adding a support for this.

While I am at it, I am adding a possibility to customize the color of a channel,
that doesn't mention our name but has some new activity in it.

I understand that tracking-add-buffer allows adding buffers to
tracking-buffers with face and we may utilize this feature. I believe it makes
sense for what whatever it is currently used but I would prefer to have a
possibility to apply faces when rendering (in opposite to assigning a face when
some activity happens), hence tracking-get-face.

By default, I am setting the tracking-channel-face and tracking-query-face
to nil and therefore they are not going to be customized and a
backward-compatibility is going to be kept for everybody who doesn't care about
this feature. Personally, I am putting the following lines to my config.

(setq tracking-get-face-function #'circe-tracking-get-face)
(set-face-attribute 'circe-tracking-channel-face nil :foreground my/white)
(set-face-attribute 'circe-tracking-query-face nil :foreground my/blue)

[FrostyX]

Adding a screenshot, because why not :-)

[FrostyX]

I updated the PR, so now tracking uses defcustom thingy to decide what function should be used to get face for a buffer. Tracking implements a default function for this, that behaves exactly how it used to be before this PR. circe.el provides a more fancy function. I can put this in my config, to use the function I want

(setq tracking-get-face-function #'tracking-get-face)
(setq tracking-get-face-function #'circe-tracking-get-face)

I understand, that it would be ideal if circe picked the fancy one but I am not sure how to do so. Also, I guess the custom faces tracking-channel-face and tracking-query-face should now be defined in circe.el rather than tracking.el and their name prefixed with circe- right?

[FrostyX]

FTR we discussed this PR and made the first round of a review on #emacs-circe.
I am done with the changes and waiting for another review.

Thank you for all the feedback

[FrostyX]

bump, PTAL :-)

[wasamasa]

You can elide this line, if there's only a truthy condition, it evaluates to its result.

[FrostyX]

Thank you, I wasn't aware that it is possible. Updated.

[wasamasa]

Typo. It's unclear how this is supposed to work as there's no such variable.

[FrostyX]

Ah, sorry. It was supposed to be tracking-get-face from tracking.el. Updated.

[wasamasa]

Do I understand correctly, that tracking-get-face-function defaults to tracking-get-face, but can also be set to circe-tracking-get-face? If yes, it might make sense to mention that in the circe-tracking-get-face docstring, otherwise people will customize circe-tracking-*-face and won't see any effects from that.

I seem to recall from my original suggestion that tracking-get-face-function should be turned into a buffer-local variable and set from the various circe mode functions to circe-tracking-get-face. That way changes to the aforementioned faces would have immediate effect.

[wasamasa]

It seems redundant to specify buffer when buffer is current. Especially if the tracking-get-face call exploits that fact.

[FrostyX]

Thank you for another round of the review @wasamasa. Sorry it requires so much babysitting, I am not that proficient in lisp.

Do I understand correctly, that tracking-get-face-function defaults to tracking-get-face, but can also be set to circe-tracking-get-face? If yes, it might make sense to mention that in the circe-tracking-get-face docstring, otherwise people will customize circe-tracking-*-face and won't see any effects from that.

That is true, and you have a good point, thank you. Updated.

I seem to recall from my original suggestion that tracking-get-face-function should be turned into a buffer-local variable and set from the various circe mode functions to circe-tracking-get-face. That way changes to the aforementioned faces would have immediate effect.

The face changes have an immediate effect even now. Or is it possible that each one of us thinks something different by it? I can randomly evaluate these lines and the colors in my modeline immediately change

(set-face-attribute 'circe-tracking-query-face nil :foreground "#fb9fb1")
(set-face-attribute 'circe-tracking-query-face nil :foreground "#6fc2ef")

It seems redundant to specify buffer when buffer is current. Especially if the tracking-get-face call exploits that fact.

The reason why I do (with-current-buffer buffer there is because I need to figure out, what is the major-mode of the buffer that is passed into the function. If you recommend a different approach, we can surely change that. Just dropping the line isn't possible because then the colors would be based on the currently viewed buffer, which is not correct.

[wasamasa]

The face changes have an immediate effect even now. Or is it possible that each one of us thinks something different by it? I can randomly evaluate these lines and the colors in my modeline immediately change

(set-face-attribute 'circe-tracking-query-face nil :foreground "#fb9fb1")
(set-face-attribute 'circe-tracking-query-face nil :foreground "#6fc2ef")

That's fine then.

The reason why I do (with-current-buffer buffer there is because I need to figure out, what is the major-mode of the buffer that is passed into the function. If you recommend a different approach, we can surely change that. Just dropping the line isn't possible because then the colors would be based on the currently viewed buffer, which is not correct.

I don't mean with (with-current-buffer buffer ...) part, but the (get-text-property 0 'face buffer) one. If the buffer is current, you don't need to pass it to a function defaulting to the current buffer.

[FrostyX]

I don't mean with (with-current-buffer buffer ...) part, but the (get-text-property 0 'face buffer) one. If the buffer is current, you
don't need to pass it to a function defaulting to the current buffer.

Thank you @wasamasa, I understand what you are saying and it makes sense. Basically, these four options should all be equivalent in this context, right?

(get-text-property 0 'face buffer)
(get-text-property 0 'face (current-buffer))
(get-text-property 0 'face nil)
(get-text-property 0 'face)

Unfortunately only the first one works as expected, others give me

Error during redisplay: (eval (spaceline-ml-main)) signaled (args-out-of-range 0 0) [5 times]

Which I don't really understand. I tried checking whether the current buffer is really changed to the buffer value that is passed as an argument, like so

(print (format "%s vs %s" (current-buffer) buffer))

but yes, the values are equal. Do you understand why it behaves like this?

[wasamasa]

Thank you @wasamasa, I understand what you are saying and it makes sense. Basically, these four options should all be equivalent in this context, right?

(get-text-property 0 'face buffer)
(get-text-property 0 'face (current-buffer))
(get-text-property 0 'face nil)
(get-text-property 0 'face)

Correct. According to the docstring:

OBJECT should be a buffer or a string; if omitted or nil, it defaults
to the current buffer.

Unfortunately only the first one works as expected, others give me

Error during redisplay: (eval (spaceline-ml-main)) signaled (args-out-of-range 0 0) [5 times]

Please check whether this only happens with spaceline. If yes, then it might be an issue specific to it and you might need to dig into what exactly spaceline-ml-main does. Or get assistance from someone understanding it.