Font trait and parser #1042

corranwebster · 2021-11-30T12:20:33Z

This PR provides a PyfaceFont trait which is similar to the Font traits in TraitsUI and Enable, but based on the Pyface Font class.

This permits constructs like:

class Report(HasTraits):
    title_font = PyfaceFont("24 pt bold Helvetica sans-serif")
    body_font = PyfaceFont("Helvetica sans-serif") 

report = Report(
    title_font="24 pt bold expanded Helvetica sans-serif"
)
report.body_font = Font(
    family=['Helvetica', 'Arial', 'sans-serif'],
    weight='light',
)

The value held by the trait is always a Font object.

Compatibility with the TraitsUI and Enable font traits for string font descriptions is provided by a simple parser class which extends the parsers of those older traits to provide close, but not exact compatibility. The primary difference is that the new parser understands the additional attributes and values exposed by the Font class (eg. additional weight values, stretch, variants, etc.), but this means that it will interpret font names with those terms differently. The work around is to simply specify the font more precisely with a Font object if that level of precision is needed, or replace with a fully backwards compatible parser.

The parser used by the Trait can be easily switched with a more (or less) capable parser if desired.

To-do:

tests

Fixes #476 (Color traits are already done)

Other relevant issues: #609 (Font class PR), #520 (older attempt), #600 (there is more work to be done on the parsing issue before this can be closed)

mdickinson · 2022-01-04T15:52:00Z

pyface/util/font_parser.py

+VARIANTS = {'small-caps'}
+DECORATIONS = {'underline', 'strikethrough', 'overline'}
+NOISE = {'pt', 'point', 'px', 'family'}
+


Would it be possible to add an EBNF (or other) description of the grammar being parsed, either here or in the docs somewhere (or both)?

It's probably not worth it for this parser: it's more-or-less a "bag of words" designed to be backwards compatible with the other ad-hoc parsers in TraitsUI and Enable. It is probably a good idea to have the "words" documented somewhere.

Longer-term I do want a proper parser with a well-defined grammar, which is why the parser is designed to be able to be switched out.

Okay, then maybe just an informal description in the comments here? Otherwise there's no description of the intended behaviour outside the code itself.

I've expanded the parser docstring accordingly.

mdickinson · 2022-01-06T14:27:30Z

pyface/tests/test_ui_traits.py

+            )
+        )
+
+    def test_init_empty_string(self):


Need a different method name here.

mdickinson

LGTM; a couple of comments

mdickinson · 2022-01-06T14:38:22Z

pyface/ui_traits.py

+        if parser is not None:
+            self.parser = parser
+        if value is not None:
+            font = self.validate(None, None, value)


It looks as though this means that a validation error would be turned into a TraitError. Would it make sense to explicitly raise ValueError or TypeError (as appropriate) instead? The error message right now isn't as helpful as it could be:

>>> PyfaceFont(b"times new roman") Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/mdickinson/Enthought/ETS/pyface/pyface/ui_traits.py", line 212, in __init__ font = self.validate(None, None, value) File "/Users/mdickinson/Enthought/ETS/pyface/pyface/ui_traits.py", line 231, in validate self.error(object, name, value) File "/Users/mdickinson/.venvs/pyface/lib/python3.9/site-packages/traits/base_trait_handler.py", line 74, in error raise TraitError( traits.trait_errors.TraitError: None

There might be something that's worth looking into in Traits about why the error message isn't good, since calling validate with None object and None trait name is done in a few places. It feels like this code might need an extra branch for that situation: https://github.com/enthought/traits/blob/d22ce1f096e2a6f87c78d7f1bb5bf0abab1a18ff/traits/trait_errors.py#L66-L94

I'll convert this to a ValueError here for now.

mdickinson · 2022-01-06T14:46:26Z

pyface/util/font_parser.py

+        elif word in DECORATIONS:
+            decorations.add(word)
+        else:
+            if size is None:


It may not matter much, but from reading the code it looks as though there's an inconsistency between the first-one-wins handling for size and the last-one-wins handling for weight, stretch, etc.

I agree it's a bit weird. This parser is deliberately following the approach of the code here:
https://github.com/enthought/traitsui/blob/b7c38c7a47bf6ae7971f9ddab70c8a358647dd25/traitsui/qt4/font_trait.py#L92-L111
and here:
https://github.com/enthought/traitsui/blob/b7c38c7a47bf6ae7971f9ddab70c8a358647dd25/traitsui/wx/font_trait.py#L74-L91

The idea is that it should be possible to switch to this parser with a minimum of behaviour change; and then later we can have a "proper" well-defined parser which people can migrate to in a more controlled way.

mdickinson · 2022-01-06T14:51:53Z

Should PyfaceFont be added to pyface.api?

mdickinson · 2022-01-06T14:53:52Z

Not for this PR, but it may be useful to add an __eq__ method for Font. Otherwise, assigning the same string twice always generates an event:

>>> from pyface.ui_traits import PyfaceFont
>>> from traits.api import HasStrictTraits
>>> class A(HasStrictTraits):
...     foo = PyfaceFont()
... 
>>> a = A()
>>> a.observe(print, "foo")
>>> a.foo = "10 pt helvetica"
TraitChangeEvent(object=<__main__.A object at 0x12f19a540>, name='foo', old=Font(decorations=TraitSetObject(), family=['default'], size=12.0, stretch=100.0, style='normal', variants=TraitSetObject(), weight='normal'), new=Font(decorations=TraitSetObject(), family=['helvetica'], size=10.0, stretch=100.0, style='normal', variants=TraitSetObject(), weight='normal'))
>>> a.foo = "10 pt helvetica"
TraitChangeEvent(object=<__main__.A object at 0x12f19a540>, name='foo', old=Font(decorations=TraitSetObject(), family=['helvetica'], size=10.0, stretch=100.0, style='normal', variants=TraitSetObject(), weight='normal'), new=Font(decorations=TraitSetObject(), family=['helvetica'], size=10.0, stretch=100.0, style='normal', variants=TraitSetObject(), weight='normal'))
>>> a.foo = "10 pt helvetica"
TraitChangeEvent(object=<__main__.A object at 0x12f19a540>, name='foo', old=Font(decorations=TraitSetObject(), family=['helvetica'], size=10.0, stretch=100.0, style='normal', variants=TraitSetObject(), weight='normal'), new=Font(decorations=TraitSetObject(), family=['helvetica'], size=10.0, stretch=100.0, style='normal', variants=TraitSetObject(), weight='normal'))
>>>

corranwebster · 2022-01-07T09:16:29Z

Should PyfaceFont be added to pyface.api?

Not yet - but we should probably have an issue for this.

corranwebster · 2022-01-07T09:26:49Z

Not for this PR, but it may be useful to add an __eq__ method for Font.

I had written one at one point, but had decided against because I didn't want to break hashing - which I'm not sure how important that actually is, since I can't think of a specific situation where I'd want a font as the keys of a dictionary or have a set of them.

mdickinson · 2022-01-07T09:31:21Z

decided against because I didn't want to break hashing

Couldn't we just do the usual thing and implement __hash__ as well, in a way that's compatible with __eq__?

corranwebster · 2022-01-07T11:01:51Z

Couldn't we just do the usual thing and implement __hash__ as well, in a way that's compatible with __eq__?

But the Font class is mutable, so I don't see a way of doing that safely. And I want the Font class to be mutable so it can sit behind editors which allow users to tweak individual attributes one at a time.

corranwebster · 2022-01-07T11:05:01Z

Actually, thinking about this, having a trait change on the same string being passed in is probably the correct behaviour, because you are getting a new Font object even if it is equivalent to the old one.

corranwebster · 2022-01-07T11:08:06Z

Not yet - but we should probably have an issue for this.

Done: #1071

mdickinson · 2022-01-07T11:57:16Z

Actually, thinking about this, having a trait change on the same string being passed in is probably the correct behaviour, because you are getting a new Font object even if it is equivalent to the old one.

Yep, agreed - if a Font instance is intentionally mutable, then two currently-identical Font objects are not interchangeable. (I don't much like the mutability, though.)

Initial check-in of font trait and simple parser.

8a391a4

corranwebster marked this pull request as draft November 30, 2021 12:21

corranwebster added 3 commits December 20, 2021 15:13

Merge branch 'main' into enh/font-trait-and-parser

715bb70

Tests for simple parser.

12bf8e2

Add tests for PyfaceFont trait; fix default size in parser.

bd7f3dd

corranwebster marked this pull request as ready for review December 21, 2021 09:05

corranwebster modified the milestone: Release 7.4.0 Dec 21, 2021

corranwebster requested a review from rahulporuri December 21, 2021 09:44

corranwebster mentioned this pull request Dec 21, 2021

Font dialog #1060

Merged

mdickinson reviewed Jan 4, 2022

View reviewed changes

corranwebster added 2 commits January 4, 2022 16:58

Added detailed description of allowed terms to parser docstring.

d1d2977

Merge branch 'main' into enh/font-trait-and-parser

5cead25

mdickinson reviewed Jan 6, 2022

View reviewed changes

pyface/tests/test_ui_traits.py Outdated

)

)

def test_init_empty_string(self):

Copy link

Member

mdickinson Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a different method name here.

Fix test method name.

6b81950

mdickinson approved these changes Jan 6, 2022

View reviewed changes

Raise ValueError on bad default value.

70520f2

corranwebster merged commit c12dbd5 into main Jan 7, 2022

corranwebster deleted the enh/font-trait-and-parser branch January 7, 2022 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font trait and parser #1042

Font trait and parser #1042

corranwebster commented Nov 30, 2021 •

edited

Loading

mdickinson Jan 4, 2022

corranwebster Jan 4, 2022

mdickinson Jan 4, 2022

corranwebster Jan 4, 2022

mdickinson Jan 6, 2022

mdickinson left a comment

mdickinson Jan 6, 2022

corranwebster Jan 7, 2022

mdickinson Jan 6, 2022

corranwebster Jan 7, 2022

mdickinson commented Jan 6, 2022

mdickinson commented Jan 6, 2022

corranwebster commented Jan 7, 2022

corranwebster commented Jan 7, 2022

mdickinson commented Jan 7, 2022

corranwebster commented Jan 7, 2022

corranwebster commented Jan 7, 2022

corranwebster commented Jan 7, 2022

mdickinson commented Jan 7, 2022

Font trait and parser #1042

Font trait and parser #1042

Conversation

corranwebster commented Nov 30, 2021 • edited Loading

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

mdickinson 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

mdickinson commented Jan 6, 2022

mdickinson commented Jan 6, 2022

corranwebster commented Jan 7, 2022

corranwebster commented Jan 7, 2022

mdickinson commented Jan 7, 2022

corranwebster commented Jan 7, 2022

corranwebster commented Jan 7, 2022

corranwebster commented Jan 7, 2022

mdickinson commented Jan 7, 2022

corranwebster commented Nov 30, 2021 •

edited

Loading