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

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

Closed
wants to merge 4 commits into from

Conversation

sbinet
Copy link
Member

@sbinet sbinet commented Jun 14, 2021

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
Copy link
Member Author

sbinet commented Jun 14, 2021

PTAL @stippi2

@sbinet sbinet requested a review from kortschak June 14, 2021 10:01
Copy link
Member

@kortschak kortschak left a comment

Choose a reason for hiding this comment

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

This looks like it has broken text alignment; the width of text in the test golden values is greater and so now the text is not centred and extends beyond the bounds of the plot.

vg/vgsvg/vgsvg.go Outdated Show resolved Hide resolved
vg/vgsvg/vgsvg.go Outdated Show resolved Hide resolved
@sbinet
Copy link
Member Author

sbinet commented Jun 14, 2021

@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
Copy link

codecov-commenter commented Jun 14, 2021

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%) ⬆️
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
Copy link
Member

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

@sbinet
Copy link
Member Author

sbinet commented Jun 15, 2021

I get this with my browser:
screenshot

(master is left.)

and this in the github rich-diff:
screenshot

@kortschak
Copy link
Member

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
Copy link
Member Author

sbinet commented Jun 15, 2021

you're probably right.

perhaps this could be addressed with embedding fonts?
i.e.: #703 and adding the proper documentation+example.

@sbinet
Copy link
Member Author

sbinet commented Jun 18, 2021

@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
Copy link
Member

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
Copy link
Member Author

sbinet commented Jun 18, 2021

they are independent.
I'll send a PR.

@sbinet
Copy link
Member Author

sbinet commented Jun 18, 2021

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

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 gonum#702.
@sbinet
Copy link
Member Author

sbinet commented Jul 1, 2021

superseded by #705

@sbinet sbinet closed this Jul 1, 2021
@sbinet sbinet deleted the issue-702-svg branch July 5, 2021 08:11
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.

None yet

3 participants