Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete paren-face customization. #6

Merged
merged 2 commits into from
Dec 5, 2013

Conversation

mswift42
Copy link
Contributor

@mswift42 mswift42 commented Dec 5, 2013

Delete paren-face customization.

I am using a theme with a light background and the parens are nearly invincible when set to color "honeydew4". I know how to customize this, but everyone using a light but not white background would have to do so.

Please see this
screenshot
it is especially hard to distinguish at first glance between ( and [ .

@greghendershott
Copy link
Owner

Thank you very much for preparing the pull-request.

However I'm reluctant to kill the whole feature.

Instead of deleting the ability to customize the paren colors, what if their default color were more sensible for light-but-not-white backgrounds? Is that something you could do, instead?


Longer version:

Personally, I love being able to de-emphasize the parens. Because normally I don't need/want to see them. I rely on auto-indent. And I rely on the ] key smartly inserting either ] or ) or } as needed to match.

People comfortable with lisps sometimes say that they hardly see the parens, in the same way people hardly see the spaces between words. I'm one of them. I really like having them be de-emphasized visually, too.

That's why I want to keep this feature. If the default color is awful for a lot of people on light-but-not-quite-white backgrounds, then let's fix that default color (instead of deleting the whole feature).


IIUC Emacs color customizations allow specifying colors for both light and dark backgrounds. I haven't taken advantage of that properly. If that's something you could help with, I'd be grateful, and I think that would also address your concern. (If you don't have time to do that: I don't have time either, right now, but when I do have time that's the way I would try to do this.)

@mswift42
Copy link
Contributor Author

mswift42 commented Dec 5, 2013

I've tried gray30 with five different themes, and it works for me, of course it's always a matter of taste.

I would like to look further into this when I can find the time, especially specifying colors for light and dark backgrounds.

@greghendershott
Copy link
Owner

So we could probably do something like:

(defconst racket-paren-face 'racket-paren-face)
(defface racket-paren-face
  '((((background dark))
     (:foreground "HoneyDew4"))
    (((background light))
     (:foreground "gray30")))
  "Face for parentheses () [] {}"
  :group 'racket)

Or, I guess another approach is define/use the face, but have it default to empty (inherit the default colors). I think that would be something like this:

(defconst racket-paren-face 'racket-paren-face)
(defface racket-paren-face
  '((t))
  "Face for parentheses () [] {}"
  :group 'racket)

@mswift42
Copy link
Contributor Author

mswift42 commented Dec 5, 2013

I really love the second approach. Out of the box it would work for everyone, and easy to customize for people who like to dim their parens.

(defface racket-paren-face
  (let ((fg (face-foreground 'default)))
    `((t (:foreground ,fg))))
  "Face for parentheses () [] {}"
  :group 'racket)

If you now do M-x customize-mode you only need to click on the default color to select a new one.

@greghendershott
Copy link
Owner

That looks great. Assuming that compiles and works cleanly for you, could you please push that to update the PR, and I'll merge it?

(Or let me know and I'll try to do same, later.)

Thanks again!

@greghendershott
Copy link
Owner

Oh you already did push that. Sorry. :)

greghendershott pushed a commit that referenced this pull request Dec 5, 2013
Default racket-paren-face to plain foreground face.
@greghendershott greghendershott merged commit c752ead into greghendershott:master Dec 5, 2013
@mswift42
Copy link
Contributor Author

mswift42 commented Dec 5, 2013

Thank you.

@greghendershott
Copy link
Owner

Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants