gonum / plot

A repository for plotting and visualizing data
BSD 3-Clause "New" or "Revised" License
2.74k stars 203 forks source link

vg/vgsvg: drop fontMap, use informations from font.Font #704

Closed sbinet closed 3 years ago

sbinet commented 3 years ago

This CL drops the use of the internal fontMap that was associating some pre-defined set of fonts with a set of SVG naming schemes (derived from PostScript). Instead, use the informations contained in plot/font.Font to derive the expected SVG font-family (and friends) font style string.

Updates #702.

Please take a look.

sbinet commented 3 years ago

PTAL @stippi2

sbinet commented 3 years ago

@kortschak what do you see?

here is what I get: screenshot

could it be that the Liberation fonts are not available to your browser?

codecov-commenter commented 3 years ago

Codecov Report

Merging #704 (1a37402) into master (bd0e370) will increase coverage by 0.05%. The diff coverage is 77.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   71.91%   71.96%   +0.05%     
==========================================
  Files          56       58       +2     
  Lines        4957     5230     +273     
==========================================
+ Hits         3565     3764     +199     
- Misses       1206     1272      +66     
- Partials      186      194       +8     
Impacted Files Coverage Δ
vg/vgsvg/vgsvg.go 64.68% <77.02%> (+4.10%) :arrow_up:
vg/vggio/context.go 100.00% <0.00%> (ø)
vg/vggio/vggio.go 67.37% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bd0e370...1a37402. Read the comment docs.

kortschak commented 3 years ago

Screenshot from 2021-06-15 07-11-45

sbinet commented 3 years ago

I get this with my browser: screenshot

(master is left.)

and this in the github rich-diff: screenshot

kortschak commented 3 years ago

It's entirely possible that the installed fonts impacts on this. There should probably be some documentation explaining how to ensure that fonts are available for SVG-rendered plots with this change. We're bound to get issues otherwise.

sbinet commented 3 years ago

you're probably right.

perhaps this could be addressed with embedding fonts? i.e.: https://github.com/gonum/plot/issues/703 and adding the proper documentation+example.

sbinet commented 3 years ago

@kortschak I have a CL ready to add fonts embedding to SVG canvases. should I add it to this PR or send another one (once that one is merged.)

kortschak commented 3 years ago

Are they independent? If they are, then looking at the PR adding embedding and then rebasing this only that once merged would be the best I think.

sbinet commented 3 years ago

they are independent. I'll send a PR.

sbinet commented 3 years ago

actually, it's a bit more intertwined than I remembered. (the coupling is in the naming taxonomy of fonts) it's #705.

sbinet commented 3 years ago

superseded by https://github.com/gonum/plot/pull/705