Don't force graphviz diagrams to render in Courier

[ecstatic-morse]

I went to render a graphviz diagram for rust-lang/rustc-dev-guide#882, and found that they are now much harder to read on my 13 inch laptop monitor. Courier is very thin (for interesting historical reasons), so I'm not happy that we are overriding the system monospace font with it.

@richkadel Could you just use a VSCode extension that handles monospace correctly? This one works just fine. I'm surprised such a simple feature is broken, and I'd prefer not to bend over backwards to support its absence.

r? @richkadel (We're probably the only two people who care ATM)

[richkadel]

Sorry for the problems this seems to be causing, but reverting the change isn't a perfect solution either, I'm afraid.

I tried the extension you suggested, from EFanZh, and I think it has several deficiencies compared to the one I finally picked. (I think I've tried all of them?)

This is the one I use (by João Pinto): https://marketplace.visualstudio.com/items?itemName=joaompinto.vscode-graphviz

I see EFanZh's previewer doesn't handle the Courier font well. I agree with you there, but it seems like that may be a bug in the EFanZh extension.

The EFanZh extension has only 13k users, whereas Pinto's extension has 47K. Those aren't huge numbers, but Pinto does have over 3-times the number of users.

Pinto's extension lays out the Courier font graph perfectly, with the exact required sizes.

When I try EFanZh's extension and just use "monospace", the layout is close but it is still miscalculating the sizes, especially for longer labels, which extend beyond the borders of the nodes.

Plus, Pinto's extension includes dot-file syntax highlighting, and a convenient context menu to open the graph to the side.

The EFanZh extension has neither of these.

Would you be willing to try the Pinto extension? Would that work for you?

Or if not, we could add another option to allow the user to select an alternative font.

Regarding the choice of Courier, I only get Courier when the font is "monospace". The d3-graphviz renderer used by the extension I use, and some others, supports Courier (and substitutes Courier for monospace, but doesn't calculate the correct layout size if monospace is the first or only font).

I don't know if other fonts work.

It doesn't seem that "thin" to me, on my display, but if there's another option that works for everyone, I'm certainly not tied to Courier specifically. It just has to be a font that works (IMO).

[richkadel]

This seems to be a well-known limitation of d3-graphviz, when I Google it.

Here's one note, for example:

https://github.com/mdaines/viz.js/wiki/Caveats#fonts

When used in Viz.js, Graphviz only knows about font metrics for Courier, Times, Helvetica, and Arial, even though it will include other font names in its output if they are specified. When another font name is specified, Graphviz will fall back to using the metrics for Times. This can result in a label being drawn outside of its node

Every derivative product I see appears to solve the problem in the same way (with "Courier, monospace").

[ecstatic-morse]

I don't use any VSCode extensions. I use dot or xdot, both of which use pango and render everything correctly. If both plugins are broken (have you tried setting your system's monospace font to Courier for the less broken one btw?), we can configure this via an environment variable.

[richkadel]

Got it. I did test with dot as well, output to PDF.
dot.pdf

The font doesn't seem "thin" to me.

How are you using it?

[ecstatic-morse]

With -Zdump-mir-dataflow diagrams. You're free to use Courier if it works for you. It doesn't work for me. I would rather not hardcode the font and allow the system (and user) to choose. However, we can use an environment variable instead, since it seems anything using vis.js will be broken otherwise.

[richkadel]

Please take a look at PR #76794 and let me know if you have any feedback, or if this will give you the results you need.

Again, sorry for the disruption with this change.

Thanks!
Rich

[ecstatic-morse]

No worries. Obviously, it's just a minor annoyance, but I do rely on these diagrams quite frequently when something breaks.

[richkadel]

@ecstatic-morse - #76794 was successfully merged, so this PR can probably be closed, if you agree.

Thanks!

[bors]

☔ The latest upstream changes (presumably #76804) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author