search

domtronn / spaceline-all-the-icons.el

A Spaceline Mode Line theme using All The Icons for Emacs
MIT License
235 stars 25 forks source link

Window Numbering error when using Square Icon Set #17

Closed domtronn closed 7 years ago

domtronn commented 7 years ago

(Quoted @jwintz #7)

using (setq spaceline-all-the-icons-icon-set-window-numbering (quote square)), breaks the modeline rendering when window number exceeds one: Invalid face attribute :background nil Invalid face attribute :foreground nil Error during redisplay: (eval (spaceline-ml-all-the-icons)) signaled (wrong-type-argument listp (:height 1.2 :inherit)) [24 times]

domtronn commented 7 years ago

I can't seem to reproduce this for windows greater than one? (see the below screenshot)

screen shot 2017-04-21 at 10 12 32

However, it does seem to throw errors when the window numbers reach 10, I'll look into a fix for this :)

jwintz commented 7 years ago

Just updated all of my melpa packages, including all-the-icons and spaceline-all-the-icons but the issue remains.

screen shot 2017-04-21 at 11 39 07

Works fine with other spaceline-all-the-icons-set-window-number values though :-(

jwintz commented 7 years ago

Reinstalled material-icon just to be sure and played with all-the-icons-window-number.

Error during redisplay: (eval (spaceline-ml-all-the-icons)) signaled (wrong-type-argument listp (:height 1.2 :inherit)) [16 times]

It comes from https://github.com/domtronn/spaceline-all-the-icons.el/blob/master/spaceline-all-the-icons-segments.el#L374-L398 where plist-put has wrong argument type.

Removing these two lines, I get the expected result !

Before:

screen shot 2017-04-21 at 19 09 09

After:

screen shot 2017-04-21 at 19 14 53

Do you mind a PR ?

domtronn commented 7 years ago

Hmm, I don't think this is the expected behaviour though... 😞

You need the plist-put ... :family in order to set the correct font family to display the icon with, otherwise you run the risk of the system choosing some other unicode character instead of the material-design icon.

I'm not sure why it won't let you do the plist-put though? What result do you get when you call

(plist-put '(:height 1.0 :inherit) :family "foo")

It shouldn't error and should return...

(:height 1.0 :family "foo" :inherit)

Basically, plist-put should allow list type arguments for the plist value (see the docs for plist-put)

Change value in PLIST of PROP to VAL. PLIST is a property list, which is a list of the form (PROP1 VALUE1 PROP2 VALUE2 ...). PROP is a symbol and VAL is any object. If PROP is already a property on the list, its value is set to VAL, otherwise the new PROP VAL pair is added. The new plist is returned; use ‘(setq x (plist-put x prop val))’ to be sure to use the new value. The PLIST is modified by side effects.

jwintz commented 7 years ago

Completely agree with you ! Just trying to isolate the bug.

Here is the result using C-x C-e in the scratch buffer. At least it is consistent with the previous one ;-)

screen shot 2017-04-21 at 19 31 55
domtronn commented 7 years ago

That's weird! It works for me 😅 What version of Emacs do you use and what's the result when you (describe-function 'plist-put)?

The alternative would be to replace the plist-puts with (setq face (append face '(:family ...))), much more verbose, but achieves the same thing :/

But you're right, it'd be nice to understand/isolate why this is happening

jwintz commented 7 years ago

As shown in the screenshot: 26.0.50. describe-function doesn't complain. I do not understand the behaviour then. My emacs-lisp fu is awful, but it seems like a syntax error when it comes to quoting the lists or so ...

screen shot 2017-04-21 at 19 37 31
domtronn commented 7 years ago

Interesting.. I'm still on Emacs 25.1, I'm going to install a copy of emacs-plus mentioned in your Prolusion ReadMe - Maybe they've changed the implementation/definition of plist-put?

That way I can confirm if it's the Emacs Version that's the problem 🙂 I'll let you know shortly 👍

jwintz commented 7 years ago

Looks like the list isn't changed in place, and that the result should be reafected. What about your plist-put description ? What about your emacs version ?

jwintz commented 7 years ago

I've looked for deprecation, but nothing is mentioned. Quite surprising for emacs API.

domtronn commented 7 years ago

I can't install emacs-plus because my XCode is out of date, and I can't upgrade it on this Mac at the moment... 😒 So yeah, I'd be happy with a PR to swap the plist-put for append, or I can pick it up later/tomorrow?

jwintz commented 7 years ago

There are 12 occurrences of plist-put. I can try and update them all (since my anzu-mode is working now ;-)).

Should I update them all ?

jwintz commented 7 years ago

And by the way, according to https://www.gnu.org/software/emacs/manual/html_node/elisp/Plist-Access.html, wouldn't it be more appropriate to setq face (plist-put ...) instead of using append ?

domtronn commented 7 years ago

That would be great if you've got time, otherwise I'll pick this up when I get some time tomorrow.

And yes, you'll need to do both... Because the setq isn't going to stop the error you're getting with incorrect argument type listp, so I would suggest changing all plist-put calls from

(plist-put 'face :property val)
;; to
(setq face (append '(:property val) 'face))

Bear in mind, that for values that need to be computed i.e. with a function call, you'll need to use backticks and commas to denote a list that has elements that need evaluating, for example

(setq face (append `(:family ,(all-the-icons-material-family)) 'face))