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

Emacs: Dark theme fixes and modernisation #1865

Merged
merged 2 commits into from Aug 8, 2018

Conversation

Projects
None yet
6 participants
@Wilfred
Copy link
Contributor

commented Jun 27, 2018

The Emacs mode in this repository has colors defined that don't work well in dark themes.

Before:

caml_before

After:

caml_after

This also affects packages like merlin, which use the styles from the caml-types.el file. In the screenshot above, I'm running the merlin-type-enclosing command.

I've also updated the face definitions to correspond to Emacs best practices. See the individual commit messages for more details.

(This is my first PR to ocaml itself, so please let me know if I've done anything silly!)

Emacs: Modernise font face definitions
make-face is a low-level primitive function that packages should not
be calling. Packages should use defface instead, which has existed
since at least Emacs 21.1 (released 2001).

By using defface, Emacs can find the definition of the face, and it's
easier to define different colours for light and dark themes. Also fix
spelling in docstrings.

Also remove fallback code for font-lock-type-face, as this variable
was created before 2000. Likewise font-lock-preprocessor-face, which
dates from 2003.
@pmetzger

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

Does it still work well with light themes?

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

It would be nice to have some feedback from other people who know about the current good practices for Emacs configuration. For example (but other volunteers please feel free to chime in), @monnier, @cpitclaudel, would you by chance be able to comment?

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

FWIW I'm using a dark theme and the only thing I had to change because of readability issues was:

(if window-system
    (progn
      (require 'caml-font)
      (set-face-foreground 'caml-font-doccomment-face "#cb4b16")))

but I guess the point of theming should be that you don't define the colors in the mode no ?

@cpitclaudel

This comment has been minimized.

Copy link

commented Jun 28, 2018

For example (but other volunteers please feel free to chime in), @monnier, @cpitclaudel, would you by chance be able to comment?

These change look reasonable to me. It's a mix of changing the code to use appropriate macros and adding a variant for dark background.

you don't define the colors in the mode no ?

The mode should provide a default, and themes can override it :)

@gasche

gasche approved these changes Jun 28, 2018

Copy link
Member

left a comment

I'm approving based on @cpitclaudel's review (thanks!), but I'll still wait a few more days, in case people have objections, before merging.

@Wilfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

Does it still work well with light themes?

Yep, colours with light themes will be unchanged by this PR.

@Wilfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

I see the CI is complaining that I haven't written a change entry. Should I write one for this editor tooling change? This change does not require users to do anything.

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 28, 2018

See our CONTRIBUTING.md#Changes guideline. This is your choice to make; there is no dumb contribution, so it would make sense for you to list your work in the Changes file and be credited for it. Also, it If you don't think it is worth the hassle, I can merge without a Changes entry -- I know that the rest of the CI passes, and we can tag this PR to exclude it from this test to get a green dot if we want.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

I think a Changes entry is appropriate here.
@Wilfred: do you mind adding one now? I'll be happy to merge after.

@Wilfred Wilfred force-pushed the Wilfred:emacs-font-face-improvements branch from bd1e150 to 720f720 Aug 8, 2018

@Wilfred

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

@trefis okey-doke, I've updated Changes. Let me know if it needs fiddling in any way :)

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

The credits in parentheses should be on their own line, and it would be nice to credit your reviewer:

- GPR#1865: support dark themes in Emacs, and clean up usage of
  deprecated Emacs APIs
  (Wilfred Hughes, review by Clément Pit-Claudel)
Emacs: Improve colors on dark themes
Previously, some of the background colors were close to white, making
pale text difficult to read.
@Wilfred

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

Woops! Fixed.

@Wilfred Wilfred force-pushed the Wilfred:emacs-font-face-improvements branch from 720f720 to e0a7ea2 Aug 8, 2018

@gasche gasche merged commit b3fa8af into ocaml:trunk Aug 8, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Wilfred Wilfred deleted the Wilfred:emacs-font-face-improvements branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.