Implement "Link Metrics..." custom parameters #641
Conversation
There was a problem hiding this comment.
Thanks so much for this! I spoke to someone today who needed this functionality.
What you have here works fine, but it's a bit clumsy - a lot of repetition of the source-finding code. Instead, we could have a metrics_source method on a GSFontMaster object which returns the right master - either the master itself if no custom parameters are set, or the correct master/first master based on custom parameter.
Then you only need to do that search in the one place, and you can get the width information from self.font.glyphs[glyph.name].layers[master.metrics_source.id].width and the kerning information from self.font.masters[master_id].metrics_source.kerning.
Let me know if you want any help implementing that, or if you don't have time to make the changes, let me know and I'll do it.
|
I've finished off Jens' PR. Would be good for another pair of eyes to OK my changes. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
simoncozens
requested review from
simoncozens
and removed request for
simoncozens
Lib/glyphsLib/builder/glyph.py
Outdated
| width = metric_layer.width | ||
| else: | ||
| width = None | ||
| if layer.width != width: |
There was a problem hiding this comment.
you're only applying it if width is not None (or if metric_layer is true); the != may catch the case when width is None and layer.width is not None and trigger the logging debug message, when in fact you haven't applied anything because width was None. I think the logger.debug() call should be moved in the if metric_layer: branch after you override width = metric_layer.width
Lib/glyphsLib/builder/kerning.py
Outdated
|
|
||
| from .glyph import BRACKET_GLYPH_RE | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
where is this logger being used?
tests/link_metrics_test.py
Outdated
| return os.path.join(os.path.dirname(__file__), "data", "LinkMetrics.glyphs") | ||
|
|
||
|
|
||
| class UtilTest(unittest.TestCase): |
There was a problem hiding this comment.
you are using plain assert .. == ... like in pytest-style tests but then you're sublcassing from unittest.TestCase.. Either you subclass TestCase and then use unittest-style self.assertEqual(etc.) or you simply use pytest-style testing and remove the subclassing from unittest.TestCase. You can still group pytest tests as instance methods by creating the test class and naming it such that it starts with the string "Test" (see https://docs.pytest.org/en/reorganize-docs/new-docs/user/naming_conventions.html). We can configure pytest so that it also recognizes class names that ends with "Test" but it's simpler if you rename it TestUtil.
I prefer that you use pytest-style for newly added tests.
The test runner in pytest anyway and it understand both. But mixing ways the two like you do here is incorrect.
|
@jenskutilek the googlebot wants you to also reply "I consent" before it's happy |
|
Thanks, good catches. |
No, it's happy enough now, because @jenskutilek was the PR originator. |
yeah you're right |
|
Thank you @simoncozens for picking this up! |
If the custom parameters "Link Metrics To Master" or "Link Metrics To First Master" are present, this will apply glyph layer widths and kerning from the referenced source master.
Fixes #256.