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.

Sign up for GitHub

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

Jump to bottom

Add -Zgraphviz_dark_mode and monospace font fix #76500

Merged
merged 2 commits into from
Sep 10, 2020
Merged

Add -Zgraphviz_dark_mode and monospace font fix #76500

merged 2 commits into from
Sep 10, 2020

Conversation

richkadel
Copy link
Contributor

@richkadel richkadel commented Sep 8, 2020 •
edited
Loading

Many developers use a dark theme with editors and IDEs, but this
typically doesn't extend to graphviz output.

When I bring up a MIR graphviz document, the white background is
strikingly bright. This new option changes the colors used for graphviz
output to work better in dark-themed UIs.

Screen Shot 2020-09-09 at 3 00 31 PM

Also fixed the monospace font for common graphviz renders (e.g., VS Code extensions), as described in #76500 (comment)

Before:
Screen Shot 2020-09-09 at 2 48 44 PM

Now with fix:
Screen Shot 2020-09-09 at 2 49 02 PM

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 8, 2020
@richkadel richkadel force-pushed the mir-graphviz-dark branch 4 times, most recently from a16f025 to c0c806f Compare September 8, 2020 23:53
@richkadel
Copy link
Contributor Author

@tmandry @wesleywiser ... FYI

@richkadel
Add -Zgraphviz_dark_mode
c19b237 
Many developers use a dark theme with editors and IDEs, but this
typically doesn't extend to graphviz output.

When I bring up a MIR graphviz document, the white background is
strikingly bright. This new option changes the colors used for graphviz
output to work better in dark-themed UIs.
@jyn514 jyn514 added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-pretty Area: Pretty printing (including `-Z unpretty`) B-unstable Blocker: Implemented in the nightly compiler and unstable. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 9, 2020
@richkadel
Copy link
Contributor Author

Note: I just realized how to fix the text width rendering issues (for example, see the attached image in the PR description).

All of the VS Code extensions for graphviz rendering have this problem. I noticed one of the renderers credits d3-graphviz for its rendering engine, and visiting that project, there's one example setting the fontname to a monospace font, but using the name Courier.

Apparently the node size is computed by the graphviz engine, which has access to a different set of fonts than the renderer (such as Chromium or Firefox), so the monospace font displays correctly in the renderer, but the engine doesn't have a font named monospace, and has to use its default (non-monospace) font to compute the (incorrect) node sizes.

I can change the fontname value to (I suggest) "Courier, monospace" (that order is required) so both names are still available (in case other engines or renders still need "monospace" as a backup fontname.

Since this is changing the same lines I've already changed for the dark mode option, I'm going to add a commit to this PR with this proposed change as well (to avoid the merge conflicts likely to happen with separate PRs).

@richkadel
Also fixed monospace font for d3-graphviz engine
f7aee33 
VS code graphviz extensions use d3-graphviz, which supports `Courier`
fontname but does not support `monospace`. This caused graphs to render
poorly because the text sizes were wrong.
@richkadel richkadel changed the title Add -Zgraphviz_dark_mode Add -Zgraphviz_dark_mode and monospace font fix Sep 9, 2020
@tmandry
Copy link
Member

tmandry commented Sep 9, 2020

Nice, this is great :). Thanks!!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 9, 2020

📌 Commit f7aee33 has been approved by tmandry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Sep 10, 2020
@tmandry
Rollup merge of rust-lang#76500 - richkadel:mir-graphviz-dark, r=tmandry
6e7d47d 
Add -Zgraphviz_dark_mode and monospace font fix

Many developers use a dark theme with editors and IDEs, but this
typically doesn't extend to graphviz output.

When I bring up a MIR graphviz document, the white background is
strikingly bright. This new option changes the colors used for graphviz
output to work better in dark-themed UIs.

<img width="1305" alt="Screen Shot 2020-09-09 at 3 00 31 PM" src="https://user-images.githubusercontent.com/3827298/92659478-4b9bff00-f2ad-11ea-8894-b40d3a873cb9.png">

Also fixed the monospace font for common graphviz renders (e.g., VS Code extensions), as described in rust-lang#76500 (comment)

**Before:**
<img width="943" alt="Screen Shot 2020-09-09 at 2 48 44 PM" src="https://user-images.githubusercontent.com/3827298/92658939-47231680-f2ac-11ea-97ac-96727e4dd622.png">

**Now with fix:**
<img width="943" alt="Screen Shot 2020-09-09 at 2 49 02 PM" src="https://user-images.githubusercontent.com/3827298/92658959-51451500-f2ac-11ea-9aae-de982d466d6a.png">
@tmandry tmandry mentioned this pull request Sep 10, 2020
Closed
@tmandry tmandry mentioned this pull request Sep 10, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2020
@bors
Auto merge of rust-lang#76558 - tmandry:rollup-bskim2r, r=tmandry
a1894e4 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#74787 (Move `rustllvm` into `compiler/rustc_llvm`)
 - rust-lang#76458 (Add drain_filter method to HashMap and HashSet)
 - rust-lang#76472 (rustbuild: don't set PYTHON_EXECUTABLE and WITH_POLLY cmake vars since they are no longer supported by llvm)
 - rust-lang#76497 (Use intra-doc links in `core::ptr`)
 - rust-lang#76500 (Add -Zgraphviz_dark_mode and monospace font fix)
 - rust-lang#76543 (Document btree's unwrap_unchecked)
 - rust-lang#76556 (Revert rust-lang#76285)

Failed merges:

r? `@ghost`
@bors bors merged commit ba6e2b3 into rust-lang:master Sep 10, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 10, 2020
@mtsr mtsr mentioned this pull request Mar 6, 2021
Merged
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
@tmandry
Rollup merge of #76500 - richkadel:mir-graphviz-dark, r=tmandry
6fa625e 
Add -Zgraphviz_dark_mode and monospace font fix

Many developers use a dark theme with editors and IDEs, but this
typically doesn't extend to graphviz output.

When I bring up a MIR graphviz document, the white background is
strikingly bright. This new option changes the colors used for graphviz
output to work better in dark-themed UIs.

<img width="1305" alt="Screen Shot 2020-09-09 at 3 00 31 PM" src="https://user-images.githubusercontent.com/3827298/92659478-4b9bff00-f2ad-11ea-8894-b40d3a873cb9.png">

Also fixed the monospace font for common graphviz renders (e.g., VS Code extensions), as described in rust-lang/rust#76500 (comment)

**Before:**
<img width="943" alt="Screen Shot 2020-09-09 at 2 48 44 PM" src="https://user-images.githubusercontent.com/3827298/92658939-47231680-f2ac-11ea-97ac-96727e4dd622.png">

**Now with fix:**
<img width="943" alt="Screen Shot 2020-09-09 at 2 49 02 PM" src="https://user-images.githubusercontent.com/3827298/92658959-51451500-f2ac-11ea-9aae-de982d466d6a.png">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-pretty Area: Pretty printing (including `-Z unpretty`) B-unstable Blocker: Implemented in the nightly compiler and unstable. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
7 participants
@richkadel @rust-highfive @tmandry @bors @nikomatsakis @jyn514 @rustbot