Toolkit-independent Font class #609

corranwebster · 2020-07-18T09:12:26Z

This is the ~~color~~ font class from #520 pulled out into a separate PR.

This does not include any of the string parsing, trait type, or dialog classes.

codecov-commenter · 2020-07-18T09:50:11Z

Codecov Report

Merging #609 into master will increase coverage by 0.31%.
The diff coverage is 83.82%.

@@            Coverage Diff             @@
##           master     #609      +/-   ##
==========================================
+ Coverage   38.97%   39.28%   +0.31%     
==========================================
  Files         487      490       +3     
  Lines       26798    27002     +204     
  Branches     4066     4110      +44     
==========================================
+ Hits        10445    10609     +164     
- Misses      15897    15921      +24     
- Partials      456      472      +16

Impacted Files	Coverage Δ
pyface/qt/QtGui.py	`81.48% <50.00%> (-9.00%)`	⬇️
pyface/ui/wx/font.py	`76.08% <76.08%> (ø)`
pyface/ui/qt4/font.py	`77.27% <77.27%> (ø)`
pyface/font.py	`95.34% <95.34%> (ø)`
pyface/ui/qt4/util/gui_test_assistant.py	`77.87% <0.00%> (-2.66%)`	⬇️
pyface/ui/wx/window.py	`74.69% <0.00%> (-2.41%)`	⬇️
pyface/wx/python_stc.py	`9.14% <0.00%> (-1.22%)`	⬇️

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 0f3202b...2e72b66. Read the comment docs.

rahulporuri

LGTM. Most of my comments are around documentation and a few questions.

pyface/qt/QtGui.py

pyface/font.py

rahulporuri · 2020-07-18T09:53:28Z

pyface/font.py

+# Note: we don't support 'medium' as an alias for weight 500 because it
+# conflicts with the usage of 'medium' as an alias for a 12pt font in the CSS
+# specification for font attributes.
+WEIGHTS.update({


Is there a resource which points out how/why this map is defined the way it is?

this and other maps which define similar associations

These come from the CSS spec, Qt and Wx constant names, true/open type font conventions, and in some cases (originally) from "traditional" values from the typesetting world (eg. the font weights date back to the fonts used in phototypesetting systems).

The CSS spec is probably the simplest comprehensive reference.

Looks like we have some extra weights here compared to the CSS spec: https://www.w3.org/TR/css-fonts-3/#propdef-font-weight

More specifically, these are the values from CSS: normal | bold | bolder | lighter | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900
Where do the other values come from?

As mentioned above, this wasn't just from the CSS spec - in particular it should at least include the mappings that Enable has here https://github.com/enthought/enable/blob/db84efa4d031fd641ebe097301a56764634ce21d/kiva/fonttools/_constants.py#L90-L105
as well as the names that Qt uses for weight constants: https://doc.qt.io/qt-5/qfont.html#Weight-enum
and the names wx uses: https://wxpython.org/Phoenix/docs/html/wx.FontWeight.enumeration.html
and the OpenType font weights: https://docs.microsoft.com/en-us/typography/opentype/spec/os2#usweightclass

More info can be found here: https://bigelowandholmes.typepad.com/bigelow-holmes/2015/07/on-font-weight.html

This is all very fuzzy because the names predate computerized fonts and most font families don't supply all weights.

pyface/ui/qt4/font.py

pyface/ui/wx/font.py

pyface/font.py

pyface/tests/test_font.py

corranwebster · 2020-07-20T11:47:28Z

I'd rather not - the round-tripping test was going for "cheap-and-cheerful", but if it's not generic then it's not particularly useful.

Better in that case is to have toolkit-specific tests that allow more fine-grained tests for what works in each permutation of back-ends.

rahulporuri · 2020-07-20T11:48:56Z

There seem to be a few review comments from #520 for the changes made in this PR which don't seem to be addressed -

kitchoi · 2020-07-21T14:37:04Z

pyface/font.py

+    'extraexpanded': 150,
+    'ultra-expanded': 200,
+    'ultraexpanded': 200,
+}


kitchoi · 2020-07-21T14:48:29Z

pyface/ui/qt4/font.py

+    QFont.ExtraLight: 'extra-light',
+    QFont.Light: 'light',
+    QFont.Normal: 'normal',
+    QFont.Medium: 'medium',


Isn't this "medium" not allowed in the weight trait?
In fact, could we map all of these to "100", "200" and so on? Then QFont.Medium will be mapped to "500", which is allowed.

Getting back to this after a year, since this is just the descriptions with no attempt at font-name parsing, 'medium' should be allowed both here and in the weight trait as there is no conflict with other uses of medium at this level.

corranwebster · 2021-10-05T12:40:14Z

There seem to be a few review comments from #520 for...

Looking through these, I have checked off the ones that have been addressed. Of the others:

keeping weights to round numbers for now (we can always allow any number later) because:
- CSS 2.x (which is roughly what we are modelling right now) only supports round values
- Qt's numeric weights are non-linear from 0 to 99 rather than 0-1000, which is awkward
- most of the time it will get rounded to something, and most fonts we will encounter in scientific computing will have round weight values
- I'd need to write a new trait type to support it, rather than relying on Map
- it feels very YAGNI to me right now
there is an issue with specifying multiple font families on Qt < 5.13 (we simply take the first specified, plus the first default family as a backup). Making this work like other systems is complex (eg. Enable, where we have a whole subsystem for this) or require the app to be running. Again, if we need it we can do it, but this feels like overkill for an old version of Qt when new versions of Qt just do what we want. It is possible we could pull the Enable font system into Pyface, but that also doesn't feel right.
I want a simple human-readable string representation (having it CSS-ish is nice, because it is likely to be understood) because the end-goal of this is for things like font editors in TraitsUI or Enable, and we want to be able to display the selected font unambiguously. It could be a separate method or function, but __str__ feels OK and I am not terribly worried about subclassing (again, YAGNI here, I think)
we can't do round-tripping tests for the toolkit objects because of things like the issue with Qt < 5.13, but we have smoke tests and I'm adding a couple more tests for things that should work independent of toolkit.

rahulporuri

LGTM with a couple of minor comments

pyface/font.py

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

corranwebster added 4 commits July 18, 2020 10:10

Tooolkit-independent Font class.

46eb66e

Clean-up and additional tests.

7a8e11f

Add forward-support for pyqt5 font weights in pyqt4

e6a20c9

Remove unused import.

b7135e1

rahulporuri approved these changes Jul 18, 2020

View reviewed changes

rahulporuri reviewed Jul 18, 2020

View reviewed changes

pyface/tests/test_font.py Outdated Show resolved Hide resolved

corranwebster added 2 commits July 20, 2020 11:43

Revisions based on PR review.

e01b3b0

Remove round-trip tests.

2e72b66

This comment has been minimized.

Sign in to view

kitchoi reviewed Jul 21, 2020

View reviewed changes

kitchoi changed the title ~~Tooolkit-independent Font class~~ Toolkit-independent Font class Aug 4, 2020

corranwebster added this to the Release 7.4.0 milestone Sep 28, 2021

corranwebster added 2 commits September 28, 2021 11:58

Merge branch 'main' into feat/font-class

e17c520

Update copyright dates.

cb77a42

corranwebster added 2 commits October 5, 2021 14:20

Updates to font class based on recent development.

4fa974a

Can't trust fonts installed on CI system, so don't test those.

3e756e3

corranwebster requested a review from rahulporuri October 11, 2021 11:59

rahulporuri approved these changes Oct 12, 2021

View reviewed changes

pyface/font.py Outdated Show resolved Hide resolved

pyface/font.py Outdated Show resolved Hide resolved

corranwebster and others added 2 commits October 12, 2021 11:10

Apply suggestions from code review

02eee8b

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

Fix order or operations.

af7abfe

corranwebster merged commit 8a67099 into main Oct 12, 2021

corranwebster deleted the feat/font-class branch October 12, 2021 10:23

corranwebster mentioned this pull request Dec 21, 2021

Font trait and parser #1042

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolkit-independent Font class #609

Toolkit-independent Font class #609

corranwebster commented Jul 18, 2020 •

edited

Loading

codecov-commenter commented Jul 18, 2020 •

edited

Loading

rahulporuri left a comment

rahulporuri Jul 18, 2020

rahulporuri Jul 18, 2020

corranwebster Jul 20, 2020

kitchoi Jul 21, 2020

kitchoi Jul 21, 2020

corranwebster Jul 15, 2021

This comment has been minimized.

corranwebster commented Jul 20, 2020

rahulporuri commented Jul 20, 2020 •

edited by corranwebster

Loading

kitchoi Jul 21, 2020 •

edited

Loading

kitchoi Jul 21, 2020

corranwebster Jul 15, 2021

corranwebster commented Oct 5, 2021

rahulporuri left a comment

Toolkit-independent Font class #609

Toolkit-independent Font class #609

Conversation

corranwebster commented Jul 18, 2020 • edited Loading

codecov-commenter commented Jul 18, 2020 • edited Loading

Codecov Report

rahulporuri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

corranwebster commented Jul 20, 2020

rahulporuri commented Jul 20, 2020 • edited by corranwebster Loading

kitchoi Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corranwebster commented Oct 5, 2021

rahulporuri left a comment

Choose a reason for hiding this comment

corranwebster commented Jul 18, 2020 •

edited

Loading

codecov-commenter commented Jul 18, 2020 •

edited

Loading

rahulporuri commented Jul 20, 2020 •

edited by corranwebster

Loading

kitchoi Jul 21, 2020 •

edited

Loading