build: macOS fix background.svg #17311
Conversation
|
Please add a copyright notice. |
|
Before I push a new commit - Is there anything you think should change about the presentation? NOTE: The year and version number are dynamic.
|
contrib/macdeploy/background.svg
Outdated
There was a problem hiding this comment.
What I meant was a copyright for the SVG itself, like you see here
There was a problem hiding this comment.
ok - I will include the copyright header in the background.svg file.
I am pretesting my next commit
There was a problem hiding this comment.
<!-- kate: space-indent off; ... -->Is this an artifact from an old linting tool - or does this need to stay?
There was a problem hiding this comment.
It tells the Kate text editor to use tabs instead of spaces for indentation.
Unless you are switching the file to use spaces, I would prefer if it stays.
I did add the copyright notice to the background.svg file as well! |
Makefile.am
Outdated
There was a problem hiding this comment.
Please don't touch unmodified lines.
There was a problem hiding this comment.
I will correct this in the next commit.
There was a problem hiding this comment.
Cleaned up diff junk.
Makefile.am
Outdated
There was a problem hiding this comment.
Is sed -e or multiple arguments portable? (Alternatives are semicolons in a single argument, or multiple sed pipes)
There was a problem hiding this comment.
The semicolon syntax didn’t play well with the $(RSVG_CONVERT) piping - But I will take another look at it.
There was a problem hiding this comment.
As far as I know the -e substitution syntax is more portable than the semicolon syntax because it treats every substitution as a separate “session”. Then the final state of the file is piped to the rsvg-convert library.
There was a problem hiding this comment.
Cleaned up this code block so it is readable - but kept the multiple -e substitutions for clarity and easier changes in the future. I looked. into it and the -e substitution syntax is widely supported.
contrib/macdeploy/background.svg
Outdated
There was a problem hiding this comment.
Does XML allow comments outside the root element? (I don't know)
contrib/macdeploy/background.svg
Outdated
There was a problem hiding this comment.
Are these actually being changed?
There was a problem hiding this comment.
I kept the same values but technically the gradient is different.
There was a problem hiding this comment.
The goal is to keep this configuration simple. You may notice that in all of the transformations - I have intentionally made them whole numbers. It is easier to read and maintain but basically creates the same effect. Future maintainers may not have the same tools I used so I am trying to consider future maintainability in this commit.
There was a problem hiding this comment.
NOTE: I thought I was done - I added the copyright notice but left out the MIT license notice.
|
Why does this still use the "Tuffy" font family? Tuffy is not a cross platform font (and its under GPL). |
|
What's not cross-platform about it? And no, it isn't GPL... it's public domain with a catchall PD-equivalent license.
|
|
@luke-jr: Cross platform: you need to install the font in order to render the SVG correctly. There are a handful of fonts that are available on almost all platforms. IMO if we are going to use Tuffy, we should probably make sure its either covered/downloaded by the depends build or detected via configure. |
|
IIRC I chose Tuffy primarily because it looked similar to the prior bitmap image's font. I don't know any free font available on all platforms (and the non-free ones obviously aren't available on free-only platforms). Tuffy is the current font. Any addition to the build system IMO should be a separate PR - no need to go through it here. |
|
Tested 8a68941b70c95ef33e2f65865a96745da5721a44 on macOS 10.15.
Uploading 8a68941b70c95ef33e2f65865a96745da5721a44_background.tiff.zip… |
|
I think I found the issue. |
|
In a previous attempt at this fix I concluded that fonts-liberation may be a good alternative if (or when) it comes necessary. Now that I have a better understanding of the issue - we don't need to consider a new font right now. Adding this link for future reference: #17143 (comment) NOTE: fonts-liberation is actually a superior option long term - it should be considered if a branding refresh is ever on the table because we can use it across the board for software deployment as well as web font (it offers an entire array of variations including monospace). |
|
Liberation is OFL, which despite popular misconception is a non-free license. I don't see what's wrong with continuing to use Tuffy... |
|
Hi-Res working. It was a bit of a work around to fix the kerning issue.
Prelim test passed: |
|
But the copyright line is missing when building with Gitian (though why?): |
contrib/macdeploy/background.svg
Outdated
There was a problem hiding this comment.
This is missing the MIT license lines...
There was a problem hiding this comment.
It is in my next commit - The editor I use keeps over writing the copy right header. Kinda obtrusive. Thanks for the feedback.
Instead of hoping to pick a font that happens to be free and on all platfroms, what about picking a free font that looks best and then tracing the letters into paths, so that it works on all platforms? I know that this makes it harder to build Bitcoin Knots, because strings can no longer be simply replaced, but at some point we'd have to make trade-offs? |
|
Or maybe add the font only as a build dependency, not run-time dependency, and then have the build system trace it into a path. |
|
That sounds like an unreasonable trade-off. We already have Tuffy as a working build dependency... We can replace it with another similarly free font, but the method itself is working fine. |
|
The font discussion was centered around the idea of using a monospaced font to work around the kerning issue. This change eliminates the need to use a monospaced font - and Tuffy should be ok. No need to add a platform specific font dependency. |
Gitian builds
|
|
@luke-jr - I am not sure what else needs to be done or if this PR should be closed. |
fixes a text scaling issue with rsvg_convert in darwin build
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
This looks much nicer, but note that #19068 might remove the background altogether. |
|
Thanks for having a look here, but I'm going to close this. As mentioned, ideally this is something we just wont have to worry about in the future. #16836 also only applies to local builds, which means that there is nearly no-one affected by this. The gitian built images still look fine, and as far as I can tell, are already high-res? i.e this is the appearance of the 0.20.1 release on my machine:
|
8 participants
FIXES: #16836
and improve upon in a future version.
Normal Res
Hi Res
The svg files are valid markup. If the linter ignores these files - maybe we can re-add svg files back to the linter?