Font path #453
Merged
Font path #453
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the option to specify a path to the font.
Add the option to specify a path to the font.
Fixed argument name mismatch.
Fix inconsistent tabs and spaces
Change whitespace to try and pass automated checks.
Adding a space between comment and function call :/
Codecov Report
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
+ Coverage 93.61% 93.64% +0.02%
==========================================
Files 25 30 +5
Lines 5326 5868 +542
Branches 554 625 +71
==========================================
+ Hits 4986 5495 +509
- Misses 215 234 +19
- Partials 125 139 +14
Continue to review full report at Codecov.
|
Add test for "text" using fontPath keyword argument.
Trying Unix style path.
Try just the current directory.
Try pulling a valid path from elsewhere in the file.
Perhaps the call to os.path.join will allow the tests to run.
|
The implementation works on my machine. And I haven't been able reproduce the segfault at https://github.com/cactrot/cadquery/blob/00a1329c2ef7a17e94566a6a330780af05098483/tests/test_cadquery.py#L2746 on my machine. Not sure how to proceed, so I guess this is stalled for now. |
|
Thanks for the contribution @cactrot ! The segfault is reproducible across different platforms, so there is something systematic happening. Are you sure that the file Backtrace for future ref: |
Add font for testing makeText
Changed directory to point to test data.
|
Hoping https://fonts.google.com/specimen/Open+Sans#license is a reasonable choice for test data. |
|
OK, that's progress! Is some safety check for situations when the font is not found? |
|
It looks like the underlying implementation is already trying to do that. I suppose "os.path.exists" before the call might be a quick fix. Or, catching segfault with the "signal" module. |
|
We are not using PythonOCC anymore, but rather our own binding (OCP). It looks like a |
Check for non-existent file instead of trying to use it and possibly segfaulting.
Check for non-existent file instead of trying to use it and possibly segfaulting.
Fix type mismatch.
Code from PyOCC expected non-implemented function.
Inverted logic
|
I added a call to check_font before trying to use the file and coverage tests for a real and non-existent file. |
Use name based on the path for registering
adam-urbanczyk
approved these changes
Oct 11, 2020
|
@cactrot I reworked the font registration a bit - custom fonts use the path as their name now. Could you check if it still works well for you? As far as I can tell, this approach does work and will not overwrite existing fonts by accident. |
|
Just pulled the changes and ran the code. Still appears to work well for me. |
jmwright
approved these changes
Oct 14, 2020
|
Thanks again @cactrot ! Merging now since Adam has already approved. |
|
Thanks for the contribution @cactrot ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add the ability to specify a file path for the font.
Implements #198