syntax highlighting doesn't work with dark themes about plisp-mode HOT 11 CLOSED

Actually, it seems to work in light colored themes, but not dark ones.

from plisp-mode.

Sorry it's taken so long for me to get to this.

The issue is that currently, picolisp-mode.el only defines faces for contexts with a light background, e.,g.:

(defface picolisp-builtin-face
  '((((background light)) :foreground "Purple"))
  "Face for PicoLisp builtins."
  :group 'picolisp-faces)

Since i don't use dark backgrounds myself, i'm not sure what foreground colours might be suitable for those who do. Do you have any suggestions?

from plisp-mode.

My understanding is that generally a major mode will assign syntax to pre-defined faces that theming is already keyed into. These are the so-called font-lock faces: https://www.gnu.org/software/emacs/manual/html_node/elisp/Faces-for-Font-Lock.html

This way, the different syntax features are assigned to consistent faces across any theme and the colors will automatically work. It seems like you did quite a bit of work defining faces for this picolisp mode, so I'm not sure where you would prefer to go from here. I'd be happy to help, with what little free time I get :)

from plisp-mode.

Yeah, picolisp-mode makes use of the font-lock infrastructure: cf. the variable picolisp-font-lock-keywords (which references the various faces defined by picolisp-mode), the function picolisp--font-lock-syntactic-face-function (a no-op function for reasons described in its docstring), and the references to both of these in the definition of picolisp-mode itself (i.e. the call to define-derived-mode).

So the faces referenced by picolisp-font-lock-keywords have to be defined such that the font-lock infrastructure knows what colours to use when the background is dark, as in the case of a dark theme. At the moment, however, as i don't even use a theme, let alone a dark one, only the light case is specified (as per my earlier comment). Including the dark case would give us something like:

(defface picolisp-builtin-face
  '((((background light)) :foreground "Purple")
    (((background dark)) :foreground "Yellow"))
  "Face for PicoLisp builtins."
  :group 'picolisp-faces)

Thus, even using the font-lock infrastructure, we still need to decide which colours to use with a dark background. :-)

from plisp-mode.

I think rather than defining your own faces (which is fine too), the idea here is to match syntax to the faces such as these:

font-lock-function-name-face
font-lock-constant-face
font-lock-builtin-face
font-lock-comment-face
font-lock-string-face

These are then styled by themes so that each major mode can do syntax highlighting according to the user's theme. Then the major mode author isn't responsible for deciding on colours (unless they want to, of course ;) )

from plisp-mode.

I'm not setup at the moment to make a PR but I just tried changing some of the highlighting definitions like this:

(defvar picolisp-font-lock-keywords
  `(("\\_<\\(T\\|NIL\\)\\_>"
     (1 'font-lock-constant-face t))
    ("\\(\\*[[:alpha:]]+\\)"
     (1 'font-lock-variable-name-face t))
    ("\\(\\+[a-z]\\S-*\\)"
     (1 'font-lock-keyword-face t))
    ("\\(\\+[A-Z][[:alpha:]]*\\)"
     (1 'font-lock-keyword-face t))
    (,(concat "\\((\\)\\_<\\(" picolisp-builtins-regex "\\)\\_>")
     (1 'default t)
     (2 'font-lock-builtin-face t))
    ("(\\(_\\S-+\\)"
     (1 'font-lock-function-name-face t))
    ("(\\([[:alpha:]]\\S-+>\\s-\\)"
     (1 'font-lock-function-name-face t))
    ("\\(\".+?\"\\)"
     (1 'font-lock-string-face t))
    ("^.*?\\(#+.*\\)$"
     (1 'font-lock-string-face)))
  "Regexes for syntax-highlighting `picolisp-mode' buffers.")

Works with my current light and dark theme. Still needs a little work (seems like function names are usually highlighted with font-lock-function-name-face but there doesn't seem to be a regex for that).

You can add more faces that inherit from the base font lock faces for additional language features, so its possible you could tweak this to fit the regex syntax you had laid out better.

from plisp-mode.

Hm, okay, i see. Thanks for explaining! So if i'm understanding correctly, the picolisp-mode faces could inherit from the font-lock faces, e.g.

(defface picolisp-builtin-face
  '((t :inherit font-lock-builtin-face))
  "Face for PicoLisp builtins."
  :group 'picolisp-faces)

such that font-lock does the work by default - meaning that theming should "just work" - but users could still modify the picolisp-mode faces if they wanted to?

from plisp-mode.

I think so, I am pretty new to working on major modes myself, but from what I found while hacking on it seemed to be the case. I personally have never made custom faces for any syntax highlighting, though I have for little visual elements like the mode line. I think most major modes just use the built in font-lock faces and then will create custom faces for additional syntax that doesn't fit in the available categories. For example, here are some additional faces in the enhanced-ruby-mode package:

 (defface enh-ruby-regexp-face
   `((t :foreground ,(face-attribute font-lock-string-face :foreground)))
   "Face used to highlight the inside of regular expressions"
   :group 'enh-ruby)

 (defface enh-ruby-op-face
   `((t :foreground ,(erm-darken-color font-lock-keyword-face)))
   "Face used to highlight operators like + and ||"
   :group 'enh-ruby)

 (defface erm-syn-errline
   '((t (:box (:line-width 1 :color "red"))))
   "Face used for marking error lines."
   :group 'enh-ruby)

 (defface erm-syn-warnline
   '((t (:box (:line-width 1 :color "orange"))))
   "Face used for marking warning lines."
   :group 'enh-ruby))

These faces are used for the extra syntax in the Ruby language (there's quite a bit), but otherwise it doesn't customize the font-lock faces at all for the typical syntax. That function erm-darken-color just makes the color 20% darker than the face passed to it. So I guess those aren't exactly inheriting from the font-lock faces, but it is similar.

I'm not sure the best approach to have user customizable faces for a specific mode. Maybe the inheritance approach would work, or perhaps you could have a variable like picolisp-use-custom-faces and if it is non-nil, the code would use the faces that you've defined, otherwise would use the font-lock faces.

from plisp-mode.

*nod* Thanks for sharing your thoughts.

i'm not particularly attached to the colours i've chosen for the faces, and inheritance seems to me to be the most likely way to respect user preferences by default, whilst still allowing users to specifically customise picolisp-mode faces if they want or need. So i've just pushed the relevant changes. Could you please confirm that this results in syntax highlighting now working with dark themes for you?

from plisp-mode.

Yep. It works like a charm:

A few details I notice though... the text following the comment # isn't greyed out, which is usual for comment, and the # itself is orange. Also, usually the variable names following a declaration of a function or variable are highlighted in some way. But these are small details and can be taken up on another issue.

I might spend some time playing around with it, because despite Alex's preference for no syntax highlighting, there's some little details that could be nice... just looking at this screenshot for example, it could be nice to have the "@1" type of variables in strings/transient symbols be highlighted in some way.

from plisp-mode.

the text following the comment # isn't greyed out, which is usual for comment, and the # itself is orange.

How very odd! Here's how it looks for me:

As you can see, the entirety of comments are highlighted appropriately.

Also, usually the variable names following a declaration of a function or variable are highlighted in some way. But these are small details and can be taken up on another issue.

Yes, please feel free to open an issue for this.

it could be nice to have the "@1" type of variables in strings/transient symbols be highlighted in some way.

i agree - please feel free to open an issue for this also. :-)

from plisp-mode.