search

alphapapa / topsy.el

Simple sticky header showing definition beyond top of window
GNU General Public License v3.0
100 stars 6 forks source link

Remap `header-line` face to `topsy-header-line` in `topsy-mode` #16

Open roshanshariff opened 10 months ago

roshanshariff commented 10 months ago

Many themes change the header-line face to be similar to the faces used by the mode line. This is often not a good fit for showing a sticky header. To allow the Topsy face to be customized independently of the usual header line, this PR remaps the header-line face to a new topsy-header-line face when Topsy is active.

By default, the topsy-header-line face just inherits from default, so it is displayed just as the rest of the buffer text would be. To revert to the original behaviour of using the header-line face, you need to do

(custom-set-faces '(topsy-header-line ((t :inherit nil))))

This PR is the first in a series that splits out the functionality of #7.

alphapapa commented 10 months ago

Hello Roshan,

I think it's basically intended that the Topsy face start from default so that the text in it will resemble the buffer text that it's supposed to display. So I don't understand what problem this PR is intended to solve. Would you demonstrate with some screenshots please?

roshanshariff commented 10 months ago

Sure, if the header-line face is not themed then this patch has no effect. But many themes customize header-line to look like mode-line, including at least Prot's ef-themes and modus-themes, as well as doom-themes. For example, I'm using ef-themes which gives the header line a gray background, and I've also enabled an option that uses variable-pitch text for UI elements. Then the Topsy master branch looks like this:

without-remapping

With this patch, it looks like this:

with-remapping

alphapapa commented 10 months ago

Thanks for the screenshots. But I'm still not sure I fully understand.

  1. Is it really desirable for our header line to not have the background of the header-line face? I would expect that if it didn't, another user might file a bug report saying that it's not respecting the header-line face. It seems strange to me that you want our header line to look like just another line of window text, because that's not what it is (at least, not without the other patch that enables that behavior).
  2. If this is what we want to do, is remapping the face the best way to do it? Face remapping is very rarely necessary, and it adds much additional complexity (even though the changes to our code are not large, what's happening behind the scenes when using face remapping is much more complex, and can cause performance issues). Can't this be done without face remapping?
roshanshariff commented 10 months ago
  1. Is it really desirable for our header line to not have the background of the header-line face? I would expect that if it didn't, another user might file a bug report saying that it's not respecting the header-line face. It seems strange to me that you want our header line to look like just another line of window text, because that's not what it is (at least, not without the other patch that enables that behavior).

I mostly just wanted Topsy to use distinct faces that can be customized independently of header-line face. I made the face :inherit default because that made sense to me, but of course the default can be just a null face, in which case it'll look just like every other header line.

I think having such a face makes a lot of sense, because even though Topsy isn't just showing another buffer line, it is quite closely connected to the text in the buffer (it is jarring, for example, when the font family changes when it scrolls off the screen and appears in the header line).

In combination with the other patch, we end up with two faces, one used when showing a sticky header and another when showing a normal buffer line. The default styling of these faces is of course up for discussion. In the current state of the patches, normal buffer text is shown in topsy-header-line which inherits from default whereas sticky headers are shown with topsy-highlight which adds :weight bold :underline t.

  1. If this is what we want to do, is remapping the face the best way to do it? Face remapping is very rarely necessary, and it adds much additional complexity (even though the changes to our code are not large, what's happening behind the scenes when using face remapping is much more complex, and can cause performance issues). Can't this be done without face remapping?

Face remapping isn't strictly necessary; we could use add-face-text-property to add the Topsy faces to the header line string "behind" all the other faces. Unfortunately, we can't use the :propertize construct in the header-line-format because it just replaces the faces instead of appending a new one. So the header-line-format would get a little bit more complicated, because it would have to propertize the leading space that's used to align the header line, and then topsy--header-string would apply the same face to the string it returns. I can easily make this change if you think face remapping is problematic.

alphapapa commented 10 months ago

Let me say first that, as you can imagine, I'm trying to digest and review these new PRs while also keeping up with all the other things I need to do. And while you already had all these features working together, I'm trying to review both the design and implementation of each one. So rather than try to do them all, I'll start with one or two, and then we can work on the others.

For this one, I think it's okay to have a base face that can be customized, but I don't think we should use face remapping unless absolutely necessary. And by default, the face should look like the header-line.

Thanks.

roshanshariff commented 10 months ago

For this one, I think it's okay to have a base face that can be customized, but I don't think we should use face remapping unless absolutely necessary. And by default, the face should look like the header-line.

Sure, sounds good, thanks! I'll make the changes when I have some time.

I'm trying to digest and review these new PRs while also keeping up with all the other things I need to do.

Yeah, of course, and feel free to take as much time as you need. As you know, it took me two years to finally make these PRs, and since this is just a hobby project there's no need to hurry. I appreciate your work for the Emacs community!

alphapapa commented 10 months ago

Thanks. Agreed that there is no rush, and I was happy to see your followup even after 2 years. (My bug-tracker philosophy is that issues should remain open as long as relevant and unaddressed.) Cheers.