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

Change colors to be more readable in light themes #10890

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Sep 6, 2024

Some of the text colors used by dune tend not to be readable on default light-themes terminals. Of course it will always be possible to configure your terminal so that some colors are too light/dark to read, but some of the preset themes on macos are unable to display some colors commonly used by dune.

The three types of text in question are Ok seen below as the green program names in the output of dune build --display=short, Details seen below as the dim text on the right side of the output of dune build --display=short, and Hint seen below as the inverted colors or italic text displaying the word "Hint" in the output of dune pkg validate.

The light theme is MacOS Terminal's "Novel" theme, the dark theme is MacOS Terminal's "Basic" theme, and the purple themes are "Catpuccin Mocha" themes of WezTerm (top) and Emacs (bottom).

@leostera you've mentioned issues with being able to read the output of dune on light themes a couple of times. Can you share the terminals and themes where you run into this issue and confirm whether this change fixes it.

Before:
Screenshot 2024-09-06 at 19 11 06

After:
Screenshot 2024-09-06 at 19 13 36

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No preference towards any particular colors. I do think we more empirical input from some more devs. The new scheme should work by default in a wide variety setups.

This definitely deserves a CHANGES entry

@gridbugs
Copy link
Collaborator Author

gridbugs commented Sep 9, 2024

Tried this out on a few more terminals. Here are the default themes for rxvt-unicode, xterm, emacs and vscode before and after this change. I suspect most people change their themes from the defaults, especially for the first 3 whose default theme is a white background, but I think it's important for dune to be useable in the default themes for these terminals regardless. In vscode (which defaults to a dark theme) dune is readable before and after this change, but I included it to demonstrate that I didn't break readability in this case.

Before:
dune-old1

After:
dune-new1

@gridbugs
Copy link
Collaborator Author

gridbugs commented Sep 9, 2024

Here's how it looked before and after on the default terminal theme in ubuntu. Again it was fine before this change, but just demonstrating that it remains fine after this change:

Before:
dune-old

After:
dune-new

@gridbugs gridbugs force-pushed the readable-colors-on-light-themes branch from e115f25 to eef7335 Compare September 9, 2024 04:26
Copy link
Collaborator

@maiste maiste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is displayed nicely on my side with foot term with both Catppuccin-mocha and Catppuccin-latte 👍

Nice colors! 😄

@Leonidas-from-XIV
Copy link
Collaborator

Also looks ok with GNOME Terminal with default GNOME color scheme
Screenshot from 2024-09-09 18-43-49

Some of the text colors used by dune tend not to be readable on
default light-themes terminals. Of course it will always be possible
to configure your terminal so that some colors are too light/dark to
read, but some of the preset themes on macos are unable to display
some colors commonly used by dune.

Signed-off-by: Stephen Sherratt <[email protected]>
@gridbugs gridbugs force-pushed the readable-colors-on-light-themes branch from eef7335 to cd94a2b Compare September 10, 2024 01:43
@rgrinberg
Copy link
Member

@nojb could you try this on windows?

@nojb
Copy link
Collaborator

nojb commented Sep 10, 2024

@nojb could you try this on windows?

Sure; a bit busy at the moment, but should be able to do it in the next day or so.

@nojb
Copy link
Collaborator

nojb commented Sep 11, 2024

@nojb could you try this on windows?

Sure; a bit busy at the moment, but should be able to do it in the next day or so.

I tried with several of the built-in themes of Windows Terminal; seems fine to me:

image

image

image

image

image

image

@rgrinberg rgrinberg added this to the 3.17.0 milestone Sep 11, 2024
@rgrinberg rgrinberg merged commit c50d37b into ocaml:main Sep 11, 2024
27 checks passed
anmonteiro pushed a commit to anmonteiro/dune that referenced this pull request Nov 17, 2024
Some of the text colors used by dune tend not to be readable on
default light-themes terminals. Of course it will always be possible
to configure your terminal so that some colors are too light/dark to
read, but some of the preset themes on macos are unable to display
some colors commonly used by dune.

Signed-off-by: Stephen Sherratt <[email protected]>
maiste added a commit to maiste/opam-repository that referenced this pull request Nov 25, 2024
CHANGES:

### Fixed

- Show the context name for errors happening in non-default contexts.
  (ocaml/dune#10414, fixes ocaml/dune#10378, @jchavarri)

- Correctly declare dependencies of indexes so that they are rebuilt when
  needed. (ocaml/dune#10623, @voodoos)

- Don't depend on coq-stdlib being installed when expanding variables
  of the `coq.version` family (ocaml/dune#10631, fixes ocaml/dune#10629, @gares)

- Error out if no files are found when using `copy_files`. (ocaml/dune#10649, @jchavarri)

- Re_export dune-section private library in the dune-site library stanza,
  in order to avoid failure when generating and building sites modules
  with implicit_transitive_deps = false. (ocaml/dune#10650, fixes ocaml/dune#9661, @MA0100)

- Expect test fixes: support multiple modes and fix dependencies when there is
  a custom runner (ocaml/dune#10671, @vouillon)

- In a `(library)` stanza with `(extra_objects)` and `(foreign_stubs)`, avoid
  double linking the extra object files in the final executable.
  (ocaml/dune#10783, fixes ocaml/dune#10785, @nojb)

- Map `(re_export)` library dependencies to the `exports` field in `META` files,
  and vice-versa. This field was proposed in to
  https://discuss.ocaml.org/t/proposal-a-new-exports-field-in-findlib-meta-files/13947.
  The field is included in Dune-generated `META` files only when the Dune lang
  version is >= 3.17.
  (ocaml/dune#10831, fixes ocaml/dune#10830, @nojb)

- Fix staged pps preprocessors on Windows (which were not working at all
  previously) (ocaml/dune#10869, fixes ocaml/dune#10867, @nojb)

- Fix `dune describe` when an executable is disabled with `enabled_if`.
  (ocaml/dune#10881, fixes ocaml/dune#10779, @moyodiallo)

- Fix an issue where C stubs would be rebuilt whenever the stderr of Dune was
  redirected. (ocaml/dune#10883, fixes ocaml/dune#10882, @nojb)

- Format long lists in s-expressions to fill the line instead of formatting
  them in a vertical way (ocaml/dune#10892, fixes ocaml/dune#10860, @nojb)

- Fix the URL opened by the command `dune ocaml doc`. (ocaml/dune#10897, @gridbugs)

- Fix the file referred to in the error/warning message displayed due to the
  dune configuration version not supporting a particular configuration
  stanza in use. (ocaml/dune#10923, @H-ANSEN)

- Fix `enabled_if` when it uses `env` variable. (ocaml/dune#10936, fixes ocaml/dune#10905, @moyodiallo)

- Fix exec -w for relative paths with --root argument (ocaml/dune#10982, @gridbugs)

- Do not ignore the `(locks ..)` field in the `test` and `tests` stanza
  (ocaml/dune#11081, @rgrinberg)

- Tolerate files without extension when generating merlin rules.
  (ocaml/dune#11128, @anmonteiro)

### Added

- Make Merlin/OCaml-LSP aware of "hidden" dependencies used by
  `(implicit_transitive_deps false)` via the `-H` compiler flag. (ocaml/dune#10535, @voodoos)

- Add support for the -H flag (introduced in OCaml compiler 5.2) in dune
  (requires lang versions 3.17). This adaptation gives
  the correct semantics for `(implicit_transitive_deps false)`.
  (ocaml/dune#10644, fixes ocaml/dune#9333, ocsigen/tyxml#274, ocaml/dune#2733, ocaml/dune#4963, @MA0100)

- Add support for specifying Gitlab organization repositories in `source`
  stanzas (ocaml/dune#10766, fixes ocaml/dune#6723, @H-ANSEN)

- New option to control jsoo sourcemap generation in env and executable stanza
  (ocaml/dune#10777, fixes ocaml/dune#10673, @hhugo)

- One can now control jsoo compilation_mode inside an executable stanza
  (ocaml/dune#10777, fixes ocaml/dune#10673, @hhugo)

- Add support for specifying default values of the `authors`, `maintainers`, and
  `license` stanzas of the `dune-project` file via the dune config file. Default
  values are set using the `(project_defaults)` stanza (ocaml/dune#10835, @H-ANSEN)

- Add names to source tree events in performance traces (ocaml/dune#10884, @jchavarri)

- Add `codeberg` as an option for defining project sources in dune-project
  files. For example, `(source (codeberg user/repo))`. (ocaml/dune#10904, @nlordell)

- `dune runtest` can now run individual tests with `dune runtest mytest.t`
  (ocaml/dune#11041, @Alizter).

- Wasm_of_ocaml support (ocaml/dune#11093, @vouillon)

- Add a `coqdep_flags` field to the `coq` field of the `env` stanza, and to the `coq.theory` stanza, allowing to configure `coqdep` flags.
  (ocaml/dune#11094, @rlepigre)

### Changed

- Remove all remnants of the experimental `patch-back-source-tree`. (ocaml/dune#10771,
  @rgrinberg)

- Change the preset value for author and maintainer fields in the
  `dune-project` file to encourage including emails. (ocaml/dune#10848, @punchagan)

- Tweak the preset value for tags in the `dune-project` file to hint at topics
  not having a special meaning. (ocaml/dune#10849, @punchagan)

- Change some colors to improve readability in light-mode terminals
  (ocaml/dune#10890, @gridbugs)

- Forward the linkall flag to jsoo in whole program compilation as well (ocaml/dune#10935, @hhugo)

- Configurator uses `pkgconf` as pkg-config implementation when available
  and forwards it the `target` of `ocamlc -config`. (ocaml/dune#10937, @pirbo)

- Enable Dune cache by default. Add a new Dune cache setting
  `enabled-except-user-rules`, which enables the Dune cache, but excludes
  user-written rules from it. This is a conservative choice that can avoid
  breaking rules whose dependencies are not correctly specified. This is the
  current default. (ocaml/dune#10944, ocaml/dune#10710, @nojb, @ElectreAAS)

- Do not add `dune` dependency in `dune-project` when creating projects with
  `dune init proj`. The Dune dependency is implicitely added when generating
  opam files (ocaml/dune#11129, @Leonidas-from-XIV)
maiste added a commit to maiste/opam-repository that referenced this pull request Nov 27, 2024
CHANGES:

### Fixed

- Show the context name for errors happening in non-default contexts.
  (ocaml/dune#10414, fixes ocaml/dune#10378, @jchavarri)

- Correctly declare dependencies of indexes so that they are rebuilt when
  needed. (ocaml/dune#10623, @voodoos)

- Don't depend on coq-stdlib being installed when expanding variables
  of the `coq.version` family (ocaml/dune#10631, fixes ocaml/dune#10629, @gares)

- Error out if no files are found when using `copy_files`. (ocaml/dune#10649, @jchavarri)

- Re_export dune-section private library in the dune-site library stanza,
  in order to avoid failure when generating and building sites modules
  with implicit_transitive_deps = false. (ocaml/dune#10650, fixes ocaml/dune#9661, @MA0100)

- Expect test fixes: support multiple modes and fix dependencies when there is
  a custom runner (ocaml/dune#10671, @vouillon)

- In a `(library)` stanza with `(extra_objects)` and `(foreign_stubs)`, avoid
  double linking the extra object files in the final executable.
  (ocaml/dune#10783, fixes ocaml/dune#10785, @nojb)

- Map `(re_export)` library dependencies to the `exports` field in `META` files,
  and vice-versa. This field was proposed in to
  https://discuss.ocaml.org/t/proposal-a-new-exports-field-in-findlib-meta-files/13947.
  The field is included in Dune-generated `META` files only when the Dune lang
  version is >= 3.17.
  (ocaml/dune#10831, fixes ocaml/dune#10830, @nojb)

- Fix staged pps preprocessors on Windows (which were not working at all
  previously) (ocaml/dune#10869, fixes ocaml/dune#10867, @nojb)

- Fix `dune describe` when an executable is disabled with `enabled_if`.
  (ocaml/dune#10881, fixes ocaml/dune#10779, @moyodiallo)

- Fix an issue where C stubs would be rebuilt whenever the stderr of Dune was
  redirected. (ocaml/dune#10883, fixes ocaml/dune#10882, @nojb)

- Fix the URL opened by the command `dune ocaml doc`. (ocaml/dune#10897, @gridbugs)

- Fix the file referred to in the error/warning message displayed due to the
  dune configuration version not supporting a particular configuration
  stanza in use. (ocaml/dune#10923, @H-ANSEN)

- Fix `enabled_if` when it uses `env` variable. (ocaml/dune#10936, fixes ocaml/dune#10905, @moyodiallo)

- Fix exec -w for relative paths with --root argument (ocaml/dune#10982, @gridbugs)

- Do not ignore the `(locks ..)` field in the `test` and `tests` stanza
  (ocaml/dune#11081, @rgrinberg)

- Tolerate files without extension when generating merlin rules.
  (ocaml/dune#11128, @anmonteiro)

### Added

- Make Merlin/OCaml-LSP aware of "hidden" dependencies used by
  `(implicit_transitive_deps false)` via the `-H` compiler flag. (ocaml/dune#10535, @voodoos)

- Add support for the -H flag (introduced in OCaml compiler 5.2) in dune
  (requires lang versions 3.17). This adaptation gives
  the correct semantics for `(implicit_transitive_deps false)`.
  (ocaml/dune#10644, fixes ocaml/dune#9333, ocsigen/tyxml#274, ocaml/dune#2733, ocaml/dune#4963, @MA0100)

- Add support for specifying Gitlab organization repositories in `source`
  stanzas (ocaml/dune#10766, fixes ocaml/dune#6723, @H-ANSEN)

- New option to control jsoo sourcemap generation in env and executable stanza
  (ocaml/dune#10777, fixes ocaml/dune#10673, @hhugo)

- One can now control jsoo compilation_mode inside an executable stanza
  (ocaml/dune#10777, fixes ocaml/dune#10673, @hhugo)

- Add support for specifying default values of the `authors`, `maintainers`, and
  `license` stanzas of the `dune-project` file via the dune config file. Default
  values are set using the `(project_defaults)` stanza (ocaml/dune#10835, @H-ANSEN)

- Add names to source tree events in performance traces (ocaml/dune#10884, @jchavarri)

- Add `codeberg` as an option for defining project sources in dune-project
  files. For example, `(source (codeberg user/repo))`. (ocaml/dune#10904, @nlordell)

- `dune runtest` can now run individual tests with `dune runtest mytest.t`
  (ocaml/dune#11041, @Alizter).

- Wasm_of_ocaml support (ocaml/dune#11093, @vouillon)

- Add a `coqdep_flags` field to the `coq` field of the `env` stanza, and to the `coq.theory` stanza, allowing to configure `coqdep` flags.
  (ocaml/dune#11094, @rlepigre)

### Changed

- Remove all remnants of the experimental `patch-back-source-tree`. (ocaml/dune#10771,
  @rgrinberg)

- Change the preset value for author and maintainer fields in the
  `dune-project` file to encourage including emails. (ocaml/dune#10848, @punchagan)

- Tweak the preset value for tags in the `dune-project` file to hint at topics
  not having a special meaning. (ocaml/dune#10849, @punchagan)

- Change some colors to improve readability in light-mode terminals
  (ocaml/dune#10890, @gridbugs)

- Forward the linkall flag to jsoo in whole program compilation as well (ocaml/dune#10935, @hhugo)

- Configurator uses `pkgconf` as pkg-config implementation when available
  and forwards it the `target` of `ocamlc -config`. (ocaml/dune#10937, @pirbo)

- Enable Dune cache by default. Add a new Dune cache setting
  `enabled-except-user-rules`, which enables the Dune cache, but excludes
  user-written rules from it. This is a conservative choice that can avoid
  breaking rules whose dependencies are not correctly specified. This is the
  current default. (ocaml/dune#10944, ocaml/dune#10710, @nojb, @ElectreAAS)

- Do not add `dune` dependency in `dune-project` when creating projects with
  `dune init proj`. The Dune dependency is implicitely added when generating
  opam files (ocaml/dune#11129, @Leonidas-from-XIV)
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.

5 participants