search

rougier / nano-modeline

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

Fix nano-modeline--base-face #53

Open aaronjensen opened 1 year ago

aaronjensen commented 1 year ago

Unless for some reason this function is supposed to return a single item list, I think this should be cadr, rather than cdr

rougier commented 1 year ago

Thansk, but I'm not sure this is correct. We want to keep the list such that it can be inherit inside a face

aaronjensen commented 1 year ago

I'm not sure I follow. The only usage of it is here:

https://github.com/rougier/nano-modeline/blob/master/nano-modeline.el#L259

If it appropriate to set a list of faces to the face property?

If so, maybe it's fine. I ran into some issues with it I thought, but it's likely they were actually caused by my other issue: https://github.com/rougier/nano-modeline/issues/54

aaronjensen commented 1 year ago

I think there was also an instance of the code where it would prepend a face to the faces, and you'd end up with a list like this: (some-face (the-base-face)), rather than (some-face the-base-face). Again, if that's valid then there's no issue here.

rougier commented 1 year ago

I'm not sure at all actually. I'll need to test your fix a bit first. One question though, there are other instances where I use the cdr (for example in nano-modeline-face but you did not fix this one. Any reason?

aaronjensen commented 1 year ago

I looked at it. I don't recall, but it was either used in a place that assumes it returns a list or it works against a cons and not a list, so the cdr is actually an item. The issue is the cdr of a list is a list, so it wasn't clear that that was actually the intent here.

rougier commented 1 year ago

Unfortunately, I don"t remember very well. I think one of the motivation was for the :inherit that can accept an arbitrary list of feature.