rougier / nano-modeline

GNU Emacs / N Λ N O Modeline
GNU General Public License v3.0
170 stars 29 forks source link

nano-modeline-faces vs faces #54

Open aaronjensen opened 1 year ago

aaronjensen commented 1 year ago

Hi, now that the simpler branch is merged, I've started to try to rebuild my modeline with the new toolkit. I was curious about the reason for nano-modeline-faces vs having separate named faces for each possible face. As a user who is needing to customize, what I have ended up doing is adding my own faces and using them. I've never had to do that before in any other package, so it's a bit cumbersome. Also, the way that the base face gets inherited in is clever, but also a bit surprising. I personally think the old way was more manageable as a consumer of the library, but I may very well be missing something.

alkurbatov commented 1 year ago

Hi, +1 to this issue. While I like the new branch pretty much, previously it was relatively simple to change foreground for secondary elements (line and column numbers), now it requires additional tinkering.

rougier commented 1 year ago

You can still use a direct face when designing a new mode. But the faces indirection make it easier to define active/inactive faces.

aaronjensen commented 1 year ago

There isn't always a direct face. See: https://github.com/rougier/nano-modeline/blob/a18780c277838983932623870752f0adddef0345/nano-modeline.el#L204

I'm not sure it makes anything easier, it just adds another layer of abstraction around the faces. It means I need to do two things (add an item to an alist and declare a new face), rather than one (change a face). It also makes is so that nano-modeline cannot be theme'd, because now there is configuration code and custom faces that must be set instead of just defining faces.

Could we have a face for each active/inactive state instead?

rougier commented 1 year ago

Problem is that when configuring the mode line, I need a "dynamic" face for applying active / inactive faces. But I agree it is far from ideal and I'm open to suggestions.

aaronjensen commented 1 year ago

What do you mean by "dynamic"? How is it any different from dynamically constructing the face name, e.g., nano-modeline-name-active-face? Isn't that what the old version did?

rougier commented 1 year ago

for example, this is the code for the buffer name. I just call the nano-modeline-face function that will take care of applying active or inactive face (using nameas a key). How would you write this instead?

(defun nano-modeline-buffer-name (&optional name)
  "Buffer name"

  (propertize
   (cond (name name)
         ((buffer-narrowed-p) (format"%s [narrow]" (buffer-name)))
         (t (buffer-name)))
   'face (nano-modeline-face 'name)))
aaronjensen commented 1 year ago

(nano-modeline-face 'name) would resolve to nano-modeline-name-active-face (or inactive). You would declare all permutations of faces.

I feel like I'm missing something here though? The keys aren't dynamic, right? They can't be anything, there's only a fixed set (at least declared by nano-modeline, obviously an end user could decide to add their own faces).

rougier commented 1 year ago

Yes, keys are not dynamic and they can be anything. They are used to find a corresponding face using the nano-modeline-faces and the current state (active / inactive).

Alternative would be to call nano-modeline-face with active-face and inactive face. Or to have a convention for naming faces (e.g. some-face-active some-face-inactive)

aaronjensen commented 1 year ago

Yeah, I think the naming convention makes the most sense. It gives users of the package a way to customize faces they are used to, and power users a utility and convention for additional faces.

Alternatively, you can do both—maintain the map but also declare faces for all of the default faces and states. In other words, rather than using bold, you would have an active name face declared and mapped. I’m just not sure that additional level of indirection buys us anything.

rougier commented 1 year ago

Ok, I'll create a branch to test. Not sure when though...