Pyface Font class and Trait Type #520
Conversation
Codecov Report
@@ Coverage Diff @@
## master #520 +/- ##
==========================================
- Coverage 38.97% 37.86% -1.12%
==========================================
Files 487 477 -10
Lines 26798 26434 -364
Branches 4066 4040 -26
==========================================
- Hits 10445 10009 -436
- Misses 15897 15993 +96
+ Partials 456 432 -24
Continue to review full report at Codecov.
|
| FontDialog = toolkit_object("font_dialog:FontDialog") | ||
|
|
||
|
|
||
| def get_font(parent, font): |
There was a problem hiding this comment.
Shouldn't parent be passed to the actual dialog?
Another thing, I might be using this function incorrectly, but simply calling it with parent=None and any font, and then clicking on either of the dialog buttons freezes the application with qt. No dialog window is brought up at all if using wx.
And finally, this function isn't tested at all, so might be good to add some tests.
There was a problem hiding this comment.
Qt dialog now works fine and wx dialog is brought up. But now it's the wx dialog that freezes for quite some time after a button is clicked (either OK, Cancel). I'm guessing that it's related to the closing of the dialog window.
Co-authored-by: Ieva <ievacerny@users.noreply.github.com>
Co-authored-by: Ieva <ievacerny@users.noreply.github.com>
|
Assuming tests pass, this is ready for re-review. |
| def close(self): | ||
| font_data = self.control.GetFontData() | ||
| wx_font = font_data.GetChosenFont() | ||
| self.font = Font.from_toolkit(wx_font) |
There was a problem hiding this comment.
Clicking Cancel on the dialog raises this error:
>>> get_font(None, "Helvetica")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/icernyte/Documents/repos/pyface/pyface/font_dialog.py", line 36, in get_font
result = dialog.open()
File "/Users/icernyte/Documents/repos/pyface/pyface/i_dialog.py", line 168, in open
self.close()
File "/Users/icernyte/Documents/repos/pyface/pyface/ui/wx/font_dialog.py", line 46, in close
self.font = Font.from_toolkit(wx_font)
File "/Users/icernyte/Documents/repos/pyface/pyface/font.py", line 387, in from_toolkit
return cls(**toolkit_font_to_properties(toolkit_font))
File "/Users/icernyte/Documents/repos/pyface/pyface/ui/wx/font.py", line 162, in toolkit_font_to_properties
family = wx_family_to_generic_family[toolkit_font.GetFamily()]
wx._core.wxAssertionError: C++ assertion "IsOk()" failed at /Users/robind/projects/bb2/dist-osx-py36/build/ext/wxW
idgets/src/common/fontcmn.cpp(407) in GetFamily(): invalid font
There was a problem hiding this comment.
The fact that this logic is wrapped inside a GUI dialog and that wx testing utility is lacking means this conversion logic is not tested with wx.
We should be able to extract this and test without launching the dialog:
def to_font_data(font):
data = wx.FontData()
data.SetInitialFont(font.to_toolkit())
return data
def from_font_data(font_data):
wx_font = font_data.GetChosenFont()
return Font.from_toolkit(wx_font)
The dialog can call these functions instead.
Then the test will look like this:
from_font_data(to_font_data(Font()))
It'd be worth adding this test with the logic extracted from the dialog. And I can confirm I got the same error above.
There was a problem hiding this comment.
I'll have a play with this - I think it might require exposing (at least at the private level) methods for getting and setting the font, but I'm OK with that as they can then be used to push common code into the mixin.
There was a problem hiding this comment.
Thank you for this - I did not realize it was this substantial and I am sorry for the late review. It has taken me quite some time to finalize the review.
The ability to configure the various font options in a toolkit agnostic way is going to be very useful. The fact that Font can be subclassed means it should be possible for downstream applications to extend the capability (e.g. Qt's font support is quite rich!)
I think I got carried away by the text parsing logic and forgot the core of the feature should be about having a toolkit-agnostic layer for creating toolkit-specific font objects.
There are two ways to create a toolkit-agnostic (1) Font object: instantiating the object directly, or (2) via a text description. It is the second way where I have the most concerns. I believe the core functionality should not depend on the text parsing capability. The parsing should be a thin optional layer that can be added later. At the moment the design seems very much driven by what is possible or convenient in the text description. There are places where this is limiting the design and the extent of which Font can be capable of. It is also somewhat distracting... With a lot of efforts going into text parsing, there are not many tests to do with creating the toolkit specific objects. Could we focus on the conversion from the toolkit-agnostic Font (HasTraits) class to/from the toolkit specific object, and defer the text-parsing instantiation layer to a separate PR? I guess the text parsing feature won't block other work such as the data table view, but if it does, then we may have a bigger problem.
| 'extraexpanded': 800, | ||
| 'ultra-expanded': 900, | ||
| 'ultraexpanded': 900, | ||
| } |
There was a problem hiding this comment.
-
This applies to both
weightsandstretches: Could we stick to the ones specified in the CSS specification (font-stretch here and font-weight here)? If we can follow the existing specification strictly, then we won't have to document it: it is already documented. Chances are, the users might already be familiar with CSS. If we roll our own font property mini-language, we are on our own the maintain the it and we won't be able to make use of open-source CSS parsers/validators available in the public domain. -
On
stretches: These keywords have a well defined mapping to percentages in the web browser world, e.g. https://developer.mozilla.org/en-US/docs/Web/CSS/font-stretch and the values are different here. Where do the values here come from? -
On
stretches: What units are these numbers? If we follow Qt documentation, then stretch number should be percentage. Then"normal"should be mapped to100(as in the reference in mozilla.org)
There was a problem hiding this comment.
This looks like a miss on my part- I think somehow I had got into my head that stretches were using the 0 to 1000 values instead of percentages.
pyface/font.py
Outdated
| FontWeight = Map(weights, default_value='normal') | ||
|
|
||
| #: A trait for font stretch values. | ||
| FontStretch = Map(stretches, default_value='normal') |
There was a problem hiding this comment.
The synchronization of the key and mapped trait is one-directional. e.g. setting stretch will update stretch_, but setting stretch_ won't update stretch. This means one can only set stretch to one of the approved set of strings.
This limits usage, e.g. if "expanded" is too narrow but "extra-expanded" is too wide, one cannot set something in between. That would be a real shame because Qt allows any integer to be set. For weight, both Qt and wx accept any integer values.
Perhaps FontStretch should be implemented like FontSize, where it can support any number while also allow adaptation from text. It would be important to decide not to use Map early this determines whether downstream code will use stretch or stretch_ for setting the stretch numerical value.
Same applies to FontWeight
There was a problem hiding this comment.
I think I want this as a design choice. I certainly do for FontWeight.
pyface/font.py
Outdated
|
|
||
| styles = ['normal', 'italic', 'oblique'] | ||
|
|
||
| variants = ['small-caps', 'underline', 'strikethrough', 'overline'] |
There was a problem hiding this comment.
Could we use capital naming for module level constants such that it is less likely to accidentally mutate them, like this scenario:
def to_variants(...):
# variants = [] # forgot to create a new local list
variants.append(...) # does not fail because Python found it in the outer scope, now we are mutating the global state :(
return variants
There was a problem hiding this comment.
I'm personally in two minds about all caps for container module "constants". If they are intended to be mutated (eg. a registry) then I don't like all-caps. In this case it's probably fine (in fact, I should possibly make them tuples rather than lists.
|
|
||
| return font_to_toolkit_font(self) | ||
|
|
||
| def __str__(self): |
There was a problem hiding this comment.
Similar to the comment in from_description, this Font object may grow beyond the definition of CSS Font Property and this will be like inventing another mini-language. Likewise, if someone tries to extend this class, they will have to override this method as well. That seems unnecessary... why is this needed?
pyface/ui/qt4/font.py
Outdated
| weight = map_to_nearest(toolkit_font.weight(), qt_weight_to_weight) | ||
| stretch = map_to_nearest(toolkit_font.stretch(), qt_stretch_to_stretch) |
There was a problem hiding this comment.
If Font.weight and Font.stretch are integers then we don't need to approximate the values. I think it would preferable if the conversion is less lossy.
There was a problem hiding this comment.
I think I want font weights to stay multiples of 100: almost all typefaces outside of specialist typographical fonts are multiples of 100. Our use case is scientific apps, so I think we're good.
I'll assess the stretch situation separately as there is an underlying problem with the implementation, but even there I think we are constrained by what most fonts in the wild actually support.
These numbers look like the could be arbitrary values, but in practice they are not, and there will never be an actual difference in what appears on a screen or what font you get if you set weight to 501 instead of 500, for example.
| ) | ||
|
|
||
| # smoke test | ||
| toolkit_font = font.to_toolkit() |
There was a problem hiding this comment.
I think some toolkit-specific tests are sorely needed for checking the content of the font object. We may not be able to check the font family where the final value used changes from system to system, but we should able to check other statically computed properties.
| break | ||
| else: | ||
| family = wx.FONTFAMILY_DEFAULT | ||
| weight = weight_to_wx_weight[font.weight_] |
There was a problem hiding this comment.
If Font.weight is an integer, we can use wx.Font.SetNumericWeight and we will be able to use weights that are in between the presets.
Each of size, family, style, weight, underline can all be set after instantiating wx.Font.
| def close(self): | ||
| font_data = self.control.GetFontData() | ||
| wx_font = font_data.GetChosenFont() | ||
| self.font = Font.from_toolkit(wx_font) |
There was a problem hiding this comment.
The fact that this logic is wrapped inside a GUI dialog and that wx testing utility is lacking means this conversion logic is not tested with wx.
We should be able to extract this and test without launching the dialog:
def to_font_data(font):
data = wx.FontData()
data.SetInitialFont(font.to_toolkit())
return data
def from_font_data(font_data):
wx_font = font_data.GetChosenFont()
return Font.from_toolkit(wx_font)
The dialog can call these functions instead.
Then the test will look like this:
from_font_data(to_font_data(Font()))
It'd be worth adding this test with the logic extracted from the dialog. And I can confirm I got the same error above.
| 'variants': variant_set, | ||
| 'size': size, | ||
| } | ||
|
|
There was a problem hiding this comment.
The text-to-font conversion is unlikely going to support all use cases. e.g. How are we going to represent superscript or subscript in this text? CSS Font Property has a limited scope for good reasons: It is not possible to pack all font styling properties into a single line of text without making the text more and more cryptic. The shorthand supports a subset of most commonly used styling, and does not try to encapsulate everything.
That aside, assuming we do want some sort of text-to-font convenience layer, having a relaxed parser worries me.
This is a valid CSS font property: "small-caps bold italic 12px Georgia, serif". But this parser wrongly considers "px" to be one of the font families
>>> parse_font_description("small-caps bold italic 12px Georgia, serif")
{'family': ['px', 'Georgia', 'serif'],
'weight': 'bold',
'stretch': 'normal',
'style': 'italic',
'variants': {'small-caps'},
'size': 12.0}
Say this was used in production but it was fine maybe because the family is only a list of priority: The "px" font is not found so the toolkit tries the next one. And the developers would have adjusted the numeric value 12 such that it actually looks nice when the unit is pt. But this is clearly a parsing error so it has to be fixed, and should the fix raises an error or not? And if one day the description should support other font size unit (px, em, rem), existing code is going to see a different behaviour where the font size becomes too big or too small, and it would be difficult to track it down to this.
I think I'd be in favour of a strict parser that is easy to maintain, or have no parsing at all.
|
This Typography is fantastically fractal and we could imagine any number of things that we might like to support (eg. kerning options, different styles of numerals, etc.) many of which aren't even exposed in Qt, let alone Wx. I made a deliberate choice to target what CSS can express, as that seems good enough for almost everything we need (it's good enough for the web!), is approximately the intersection of what wxPython (at least in 4.1) and PyQt support, and additionally provides a standard textual representation for the font that is familiar to developers. The CSS textual representation is important because:
So it's quite deliberately limited. But its likely far more than what we actually need for real scientific computing use-cases. |
|
Changes I am planning to make based on all of this:
|
This is a step in the direction of sorting out #476
This PR provides:
Fontclass that roughly corresponds to the CSS font specificationPyfaceFonttrait type that holds such aFontinstance and auto-converts CSS font descriptions to those fontsFontobjects to and from the toolkit font specification objectsFontDialogand associated classes for bringing up a standard system font dialog to get aFontvalue.