PR: Add font properties handling #52

goanpeca · 2020-01-02T00:09:34Z

This should add support for font properties and shorthand handling.

I have some questions I left with TODO on the code.

I also created a new file fonts.py, since I think we will need to put more font stuff on the future.

freakboy3742

A good start here. I've raised a few questions; the biggest is around the handling of the font_family property (which I think is going to need it's own special property declaration to be managed well)

colosseum/constants.py

colosseum/validators.py

colosseum/declaration.py

colosseum/constants.py

colosseum/fonts.py

colosseum/validators.py

colosseum/declaration.py

tests/test_validators.py

goanpeca · 2020-01-02T21:50:56Z

Another question: Should the setter of font receive a tuple of values where the last item might be a list? or have the setter(self, *args) (does this even work for setters :-p ?) ? Should we have several options?

# String
node.style.font = '12px/10px palatino, serif'


# # Which one should we provide, enforce besides string?

# Implicit tuple
node.style.font = '12px/10px', 'palatino'
node.style.font = 'normal', 'normal', 'normal', '12px/10px', 'palatino'

# Tuple
node.style.font = ('normal', 'normal', 'normal', '12px/10px', 'palatino')

# List
node.style.font = ['normal', 'normal', 'normal', '12px/10px', 'palatino']

# List and string for family
node.style.font = ['normal', 'normal', 'normal', '12px/10px', 'palatino, serif']

# List for all
node.style.font = ['normal', 'normal', 'normal', '12px/10px', ['palatino, serif']]

# Dictionary
node.style.font = {'size': '12px', 'family': 'palatino'}
node.style.font = {'size': '12px', 'family': 'palatino, serif'}
node.style.font = {'size': '12px', 'family': ['palatino', 'serif']}

goanpeca · 2020-01-03T06:44:52Z

Hi @freakboy3742 made the fixes as suggested and simplify the parsing (no more regex! yei).

Added a new list validated property and completed tests for font_family and font shorthand. Left a couple of questions as TODO:

I could add validation of system fonts by querying for available fonts or I could leave that for a separate PR

colosseum/constants.py

freakboy3742

More good improvements; still some work to be done.

The new font identification stuff is great; however, as I've flagged inline, we proably want to reverse the lookup (i.e., investigate if a font name is valid on request, rather than pre-computing a list of all valid fonts). It also adds some new testing requirements that we need to make sure we've got in CI.

Other than that, the big requirement is more testing. The font parsing in particular is a complex piece; I'd expect to see a successful and failing parse case for each font attribute in each possible position, as well as a comprehensive set of cases for the comma-splitting part of the font name parsing.

.github/workflows/ci.yml

colosseum/constants.py

colosseum/fonts.py

tests/test_declaration.py

colosseum/fonts.py

goanpeca · 2020-01-11T00:13:31Z

The new font identification stuff is great; however, as I've flagged inline, we proably want to reverse the lookup (i.e., investigate if a font name is valid on request, rather than pre-computing a list of all valid fonts). It also adds some new testing requirements that we need to make sure we've got in CI.

I have added an automatic way to add the needed fonts so users do not really need to follow any special instruction and I added a FontDatabase class that checks for fonts on demand and caches them if found so I think this solves your initial concerns

Other than that, the big requirement is more testing. The font parsing in particular is a complex piece; I'd expect to see a successful and failing parse case for each font attribute in each possible position, as well as a comprehensive set of cases for the comma-splitting part of the font name parsing.

Added more tests for all the cases you suggested.

…nd order

freakboy3742

Some more good improvement here. A few more comments inline.

colosseum/declaration.py

.github/workflows/ci.yml

colosseum/fonts.py

colosseum/validators.py

tests/test_parser.py

tests/utils.py

goanpeca · 2020-01-16T03:10:42Z

@freakboy3742 added the corrections and updted CI to use pytest

goanpeca · 2020-01-18T01:18:50Z

I gave it a bit of thought and I don't know if it is worth it to use pytest. I could just use permutations instead of generating over a thousand different tests. Instructions become less simple and pytest takes longer to run. Thoughts @freakboy3742 ?

freakboy3742

More solid improvements; still a bit more to go.

In particular, the FontShorthand handling (and, more generally, the approach we take for "shorthand" properties) needs some work

It strikes me that we've got two different purposes for FontShorthand. There's a storage class that is "a font" (i.e., this is "italic bold 14pt Helvetica") that can be easily mapped to an actual platform font instance; and there's the validation and storage mechanism for the individual configuration points that make up that definition. Asking for "the font" should give you a single object; however, that doesn't necessarily mean that's how it should be stored. This becomes most apparent in the tests you've added for FontShorthand.__repr__ - a font declaration of "normal normal normal..." ... isn't normal. :-)

I'm wondering if the best approach here might be to make the underlying base properties (e.g., font_family) the storage point for each individual property; with the "shorthand" properties able to either compose the underlying properties into a string representation, or generate an "object" representation as needed.

Regarding adopting pytest - my general preference is to have more, finer granularity tests. When you have monolithic tests, seeing a test failure doesn't tell you how widespread the problem actually is. The worst outcome for me is when a test fails, you make a fix, and the same test fails, but three lines later. For me, that means you've got 2 different tests; it's vastly preferable to have 2 failures in that situation. Having 1000 tests isn't a problem in my book. If it's causing a problem with test runtime, I'd be interested to know if the pytest-xdist extension helps.

Also - could I ask that you don't click mark as resolved if it's a question/issue that I've raised? On a long-running ticket like this one, it's difficult for me to tell which pieces of feedback you've addressed and which ones you haven't.

freakboy3742 · 2020-01-18T02:59:21Z

colosseum/fonts.py

+    @staticmethod
+    def fonts_path(system=False):
+        """Return the path for cross platform user fonts."""
+        if sys.platform == 'win32':


The fact that there are multiple instances of this platform check is mildly concerning. It feels like there's an opportunity for a base "FontDatabase" class with platform-specific subclasses, with a single package-level platform check to determine which class actually gets instantiated when you ask for the FontDatabase.

freakboy3742 · 2020-01-18T03:27:16Z

colosseum/declaration.py

+                try:
+                    if getattr(self, '_%s' % name, None):
+                        non_default.append((name, getattr(self, name)))
+                except AttributeError:


Is this exception handler still needed if the getattr on L623 has a default value?

Hmm nope, you are correct.

freakboy3742 · 2020-01-18T03:30:36Z

colosseum/declaration.py

+            # Shorthand values are constructed on demand since they depend on
+            # other properties, we have to use getattr(self, name) instead of
+            # getattr(self, '_%s' % name)
+            if name in shorthand_properties:


If we need this kind of logic check, then I'd argue the abstraction has failed. L617 should be a relatively simple "iterate over all the properties and if the value isn't the default, store it".

freakboy3742 · 2020-01-18T03:41:39Z

colosseum/declaration.py

+
+        if values != getattr(self, '_%s' % name, list(initial)):
+            # We use the initial type to make an instance with the set values
+            setattr(self, '_%s' % name, initial.__class__(values))


I get what you're doing here, but I think it would be better to be explicit - set the "storage class" as an attribute of the property, and then coerce the initial value into that storage class.

Ok, sounds good

colosseum/declaration.py

freakboy3742 · 2020-01-18T03:48:52Z

tests/test_wrappers.py

+        self.assertNotEqual(id(ilist1), id(ilist1.copy()))
+        self.assertNotEqual(id(ilist2), id(ilist1.copy()))
+        self.assertEqual(hash(ilist2), hash(ilist1.copy()))
+        self.assertEqual(ilist1, ilist1.copy())


This is (at least) 8 independent tests, not one tests. You've already got the ImmutableList namespace; might as well use it to differentiate the different properties you're checking.

Same comment for many of the following tests.

Will add more shorthand tests as soon as I have more clarity on the approach

freakboy3742 · 2020-01-18T03:50:34Z

tests/test_wrappers.py

+    def test_font_shorthand(self):
+        # Check initial
+        font = FontShorthand()
+        self.assertEqual(str(font), 'normal normal normal medium/normal initial')


While this is technically correct, this... isn't a very natural way to describe a "default font". If you're not setting any values, then I'd expect this to fall back to the barest minimum on output - "initial". I'd only expect to see "normal normal normal medium/normal" if those values had been explicitly set.

Fair enough :-)

@freakboy3742 I kinda disagree because setting the font shorthand to say just 'initial', will reset all other font properties to their default values so this is like effectively setting those values to their initial values "implicitly"

I am not sure how we could really handle this use case.

I could try to display the minimum values in this str but once this "Font" object is applied to the style of a given node it is effectively resetting all the other properties.

Still need to think this a bit further

goanpeca · 2020-01-18T04:29:58Z

More solid improvements; still a bit more to go.

In particular, the FontShorthand handling (and, more generally, the approach we take for "shorthand" properties) needs some work

It strikes me that we've got two different purposes for FontShorthand. There's a storage class that is "a font" (i.e., this is "italic bold 14pt Helvetica") that can be easily mapped to an actual platform font instance; and there's the validation and storage mechanism for the individual configuration points that make up that definition. Asking for "the font" should give you a single object; however, that doesn't necessarily mean that's how it should be stored. This becomes most apparent in the tests you've added for FontShorthand.repr - a font declaration of "normal normal normal..." ... isn't normal. :-)

I think FontShorthand and the Font object that should provide whatever font specific properties (em values x height etc...) we might need might be a single object. They could be one and the same I guess.

I'm wondering if the best approach here might be to make the underlying base properties (e.g., font_family) the storage point for each individual property; with the "shorthand" properties able to either compose the underlying properties into a string representation, or generate an "object" representation as needed.

Not sure, your call :-)

Regarding adopting pytest - my general preference is to have more, finer granularity tests. When you have monolithic tests, seeing a test failure doesn't tell you how widespread the problem actually is. The worst outcome for me is when a test fails, you make a fix, and the same test fails, but three lines later. For me, that means you've got 2 different tests; it's vastly preferable to have 2 failures in that situation. Having 1000 tests isn't a problem in my book. If it's causing a problem with test runtime, I'd be interested to know if the pytest-xdist extension helps.

Will look into pytest-xdist

Also - could I ask that you don't click mark as resolved if it's a question/issue that I've raised? On a long-running ticket like this one, it's difficult for me to tell which pieces of feedback you've addressed and which ones you haven't.

Will do!

goanpeca

Thanks for the review, indeed the shorthand hadling might need some extra thinking but I think we are progressing in that direction :-)

goanpeca · 2020-01-23T14:53:31Z

colosseum/declaration.py

+
+    def setter(self, value):
+        try:
+            shorthand = storage_class(**parser(value))


@freakboy3742 what should be the behavior when setting a font shorthand?

If the user only provides some parts, let's say"

style.font = "12px/3 serif"

Since this shorthand resets the missing properties to their initial values, then should we explicitly write the properties or delete them (which would be akin to setting them to their initial values when the user requests them)

So:

Set properties with missing values

style.font = "12px/3 serif" print(style) font: .. font_size: ... font_family: ... font_style: ... font_variant: ... font_weight: ...

vs.

Delete properties with missing values

style.font = "12px/3 serif" print(style) font: .. font_size: ... font_family: ...

I'd say the latter. "normal normal normal normal 12px/3 serif" doesn't communicate anything that "12px/3 serif" doesn't communicate; and I'd say the same is also true for the inverse. If I explicitly ask for "what is the font variant", then the answer is "normal"; but if I just ask for the font, the explicitness of including "normal" for font variant doesn't really aid any understanding.

freakboy3742 requested changes Jan 2, 2020

View reviewed changes

goanpeca requested a review from freakboy3742 January 2, 2020 06:08

goanpeca force-pushed the enh/add-font-properties branch 2 times, most recently from 18436e9 to 1f48945 Compare January 2, 2020 06:23

goanpeca added 7 commits January 2, 2020 20:33

Add font shorthand parser

6feae5a

Add font properties to declaration

c7b7dc5

Add tests for font parser and properties

e1f1401

Fix codestyle

ba2cc2f

Add font_family list property, fix code review comments

32b9f47

Fix code style

f497c6f

Update list property and tests

fc14114

goanpeca force-pushed the enh/add-font-properties branch from 1f48945 to fc14114 Compare January 3, 2020 06:33

Fix code style

3fad53c

goanpeca added 2 commits January 3, 2020 15:05

Add initial support to search for fonts on mac with rubicon

c4cc7bb

Update font for linux

4f8c4b9

goanpeca force-pushed the enh/add-font-properties branch from c56254f to 4f8c4b9 Compare January 3, 2020 21:51

goanpeca commented Jan 3, 2020

View reviewed changes

colosseum/constants.py Outdated Show resolved Hide resolved

Update tests

925bf17

goanpeca force-pushed the enh/add-font-properties branch from c1c5556 to 861786b Compare January 4, 2020 18:01

Copy ahem font

bc376e5

goanpeca force-pushed the enh/add-font-properties branch from 861786b to bc376e5 Compare January 4, 2020 18:01

Add win font query

71eb789

freakboy3742 requested changes Jan 5, 2020

View reviewed changes

goanpeca added 2 commits January 8, 2020 15:38

Refactor code, cache fonts and address code review comments

ba7de67

Add fontdb and refactor code. Fix code style

d496261

goanpeca commented Jan 8, 2020

View reviewed changes

colosseum/fonts.py Outdated Show resolved Hide resolved

goanpeca added 2 commits January 8, 2020 21:07

Update CI and add font copy initialization

d755bc5

Test CI

ed23e67

Update font registry on windows

dfe6e61

goanpeca force-pushed the enh/add-font-properties branch from 44d3c2e to dfe6e61 Compare January 10, 2020 00:06

Add more tests

8d063b9

Fix code style

8e40d6d

goanpeca requested a review from freakboy3742 January 11, 2020 00:16

goanpeca added 5 commits January 10, 2020 19:23

Fix tests

2fb5815

Fix tests

be72024

More tests

91b816d

Remove print statements

1e496c6

Update CI config and add exception message for incorrect font shortha…

382e758

…nd order

goanpeca mentioned this pull request Jan 11, 2020

PR: Add cursor property #56

Merged

freakboy3742 requested changes Jan 11, 2020

View reviewed changes

goanpeca added 3 commits January 15, 2020 21:47

Add font shorthand wrapers and update tests

8b3b6f1

Fix code style

e4cd59c

Fix error on GTK test

dc1710f

goanpeca requested a review from freakboy3742 January 16, 2020 03:08

freakboy3742 requested changes Jan 18, 2020

View reviewed changes

goanpeca commented Jan 18, 2020

View reviewed changes

goanpeca added 2 commits January 19, 2020 22:49

Break tests, add pytest-xdist, clean up code

c2bd8cd

Fix tests

944e53f

goanpeca commented Jan 23, 2020

View reviewed changes

goanpeca added 5 commits January 26, 2020 23:16

Merge branch 'master' into enh/add-font-properties

94cb398

Merge branch 'master' into enh/add-font-properties

40faa93

Fix code style

b754c81

Merge with master

94329c9

Fix code style

d824c86

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Add font properties handling #52

PR: Add font properties handling #52

goanpeca commented Jan 2, 2020 •

edited

freakboy3742 left a comment

goanpeca commented Jan 2, 2020 •

edited

goanpeca commented Jan 3, 2020

freakboy3742 left a comment

goanpeca commented Jan 11, 2020

freakboy3742 left a comment

goanpeca commented Jan 16, 2020

goanpeca commented Jan 18, 2020

freakboy3742 left a comment

freakboy3742 Jan 18, 2020

goanpeca Jan 18, 2020 •

edited

freakboy3742 Jan 18, 2020

goanpeca Jan 18, 2020

goanpeca Jan 18, 2020 •

edited

freakboy3742 Jan 18, 2020

goanpeca Jan 18, 2020 •

edited

freakboy3742 Jan 18, 2020

goanpeca Jan 18, 2020

goanpeca Jan 18, 2020 •

edited

freakboy3742 Jan 18, 2020

freakboy3742 Jan 18, 2020

goanpeca Jan 18, 2020

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 20, 2020

freakboy3742 Jan 18, 2020

goanpeca Jan 18, 2020

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 20, 2020

goanpeca commented Jan 18, 2020

goanpeca left a comment

goanpeca Jan 23, 2020 •

edited

freakboy3742 Jan 26, 2020

goanpeca Jan 26, 2020

PR: Add font properties handling #52

Are you sure you want to change the base?

PR: Add font properties handling #52

Conversation

goanpeca commented Jan 2, 2020 • edited

freakboy3742 left a comment

Choose a reason for hiding this comment

goanpeca commented Jan 2, 2020 • edited

goanpeca commented Jan 3, 2020

freakboy3742 left a comment

Choose a reason for hiding this comment

goanpeca commented Jan 11, 2020

freakboy3742 left a comment

Choose a reason for hiding this comment

goanpeca commented Jan 16, 2020

goanpeca commented Jan 18, 2020

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 18, 2020 • edited

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

goanpeca Jan 18, 2020 • edited

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

goanpeca Jan 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jan 18, 2020

goanpeca left a comment

Choose a reason for hiding this comment

goanpeca Jan 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jan 2, 2020 •

edited

goanpeca commented Jan 2, 2020 •

edited

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 18, 2020 •

edited

goanpeca Jan 23, 2020 •

edited