Add a new font-editor that uses Enable to draw fonts by corranwebster · Pull Request #928 · enthought/enable

corranwebster · 2022-04-08T09:58:30Z

This font editor is largely toolkit independent: it embeds a Window with a Label component to display the font and the simple editor uses the Pyface font dialog to get new font values. It also includes some drive-by fixes to the Label component (eg. mis-drawn borders, not updating when size of widget changes).

There is a demo of the code in enable/demo/enable/editors/font_editor.py:

Some functionality is broken out into experimental base classes for building editors with components in the thought that those may be independently useful in the future.

Things not done:

a custom editor with widgets that control styling, etc. independently
better string font representation/parsing
changes to the font trait itself

This is towards #922

This font editor is laregly toolkit independent - it embeds a Window with a Label component to display the font and the simple editor uses the Pyface font dialog to get new font values. Some functionality is broken out into experimental base classes for building editors with components.

mdickinson · 2022-04-21T12:41:30Z

Comments from manual testing; I haven't yet looked at the code.

Minor feature request: for the demo script, in the case where I'm editing the text view, it would be nice to also see which font has ultimately been selected. (I'm entering random things like "20 point Comic Sans", pressing return, and not being sure whether the result is actually comic sans or not. Well, okay, in this case, I am sure that it's not just by looking at the results, but more generally ...)

In manual testing of the demo script (with wx; I haven't yet tried with Qt), I get a traceback if I click on "Cancel" after launching the font dialog with either the "simple" button or the "custom" button.

Otherwise, this is looking good.

Exception occurred in traits notification handler for event object: TraitChangeEvent(object=<enable.tools.button_tool.ButtonTool object at 0x7fb930be7780>, name='clicked', old=<undefined>, new=True)
Traceback (most recent call last):
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/traits/observation/_trait_event_notifier.py", line 122, in __call__
    self.dispatcher(handler, event)
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/traits/observation/observe.py", line 27, in dispatch_same
    handler(event)
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/enable/trait_defs/ui/kiva_font_editor.py", line 100, in button_clicked
    pyface_font = get_font(self.window.control, pyface_font)
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/pyface/font_dialog.py", line 37, in get_font
    result = dialog.open()
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/pyface/i_dialog.py", line 168, in open
    self.close()
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/pyface/ui/wx/font_dialog.py", line 51, in close
    self.font = Font.from_toolkit(wx_font)
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/pyface/font.py", line 354, in from_toolkit
    return cls(**toolkit_font_to_properties(toolkit_font))
  File "/Users/mdickinson/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/pyface/ui/wx/font.py", line 160, 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/wxWidgets/src/common/fontcmn.cpp(407) in GetFamily(): invalid font

mdickinson · 2022-04-21T12:52:42Z

I get a traceback

Update: I'm only seeing this with wx, not with PyQt5.

corranwebster · 2022-04-21T13:22:36Z

Minor feature request: for the demo script, in the case where I'm editing the text view, it would be nice to also see which font has ultimately been selected. (I'm entering random things like "20 point Comic Sans", pressing return, and not being sure whether the result is actually comic sans or not. Well, okay, in this case, I am sure that it's not just by looking at the results, but more generally ...)

Unfortunately we don't have any way of telling whether the given description is "matching" a font - basically this generates a font object which is passed over to the backend to do whatever it will with it. What Qt does with that in the QPainter backend follows a completely different path compared to what the agg backends do, and there is currently no feedback on what the result is.

And in all cases it "matches" something, it's just that the match might be really bad.

So in this it's a lot like CSS fonts: you can type whatever you want as a font descriptor in CSS, but whether the browser renders anything like what you asked for is hugely dependent on the browser, the OS, the user's installed fonts, what you make available via the website, etc.

Even the simple version of the editor might be misleading: it's allowing you to select fonts via the Qt/OS font dialog, but if you are rendering with agg it may not 100% agree with what Qt thinks you have available.

So I don't know what we could do to improve the UX here without fairly substantial changes.

corranwebster · 2022-04-21T13:23:48Z

Update: I'm only seeing this with wx, not with PyQt5.

Probably lack of testing on my part, but can you tell me what version of WxPython? I think we don't fully support 4.1 right now.

mdickinson

Code LGTM; a few nitpick-level comments. There's also the wx traceback reported earlier; that might be worth an issue if it's not going to be fixed in this PR.

mdickinson · 2022-04-21T12:54:14Z

@@ -1,4 +1,4 @@
-# (C) Copyright 2005-2022 Enthought, Inc., Austin, TX
+# (C) Copyright 2005-2021 Enthought, Inc., Austin, TX


Looks like this change should be reverted.

mdickinson · 2022-04-21T12:59:08Z

+""" Test the interaction between traitsui and enable's ComponentEditor.
+"""
+import unittest
+from unittest import mock


There seem to be a good number of unused imports here. Flake8 shows me this:

enable/tests/trait_defs/test_kiva_font_editor.py:13:1: F401 'unittest.mock' imported but unused enable/tests/trait_defs/test_kiva_font_editor.py:15:1: F401 'pyface.font.Font as PyfaceFont' imported but unused enable/tests/trait_defs/test_kiva_font_editor.py:16:1: F401 'pyface.font_dialog' imported but unused enable/tests/trait_defs/test_kiva_font_editor.py:17:1: F401 'traits.api.Any' imported but unused enable/tests/trait_defs/test_kiva_font_editor.py:24:1: F401 'enable.tests._testing.get_dialog_size' imported but unused enable/tests/trait_defs/test_kiva_font_editor.py:24:1: F401 'enable.tests._testing.skip_if_not_qt' imported but unused enable/tests/trait_defs/test_kiva_font_editor.py:24:1: F401 'enable.tests._testing.skip_if_not_wx' imported but unused

mdickinson · 2022-04-21T13:02:01Z

+
+Editor = toolkit_object("editor:Editor")
+if not issubclass(Editor, BaseEditor):
+    Editor = object


I'm assuming that this is so that the import doesn't break if the toolkit is "null"; is that correct? It might be worth an explanatory comment.

Yes, I believe so; this behaviour was copied from earlier code. It is possible it may not be needed any more.

Yep, still needed here; may be removable in #929 which re-works the trait which uses the editor.

mdickinson · 2022-04-21T13:03:21Z

+
+        return width, height
+
+    def set_size_policy(self, direction, resizable, springy, stretch):


It would be nice to have some documentation about the expected types of the various arguments here.

This is an internal Qt backend method (despite being public): full documentation is here https://github.com/enthought/traitsui/blob/44459d729ca013a6b58cebda2bf94bd789de8b7c/traitsui/qt4/editor.py#L298-L322

I can copy it over if you think it is needed.

mdickinson · 2022-04-21T13:06:47Z

+from pyface.font import Font as PyfaceFont
+from pyface.font_dialog import get_font
+from traits.api import Bool, Callable, Instance, Str, observe
+from traits.trait_base import SequenceTypes


Possibly not for this PR, but it would be good to move away from this use of SequenceTypes. Like anything else not in one of the Traits *.api modules, it's not a supported part of the Traits API and may move or disappear in a future Traits version.

Actually, I'm not sure it's even useful here; see question below.

Agreed. I'd like a better way to express the listy/tuply-sort of things consistently and in one place - we already have some definitintions that aren't common which differ eg. on the status of numpy arrays. There was also some reason why collections.abc.Sequence didn't work for this purpose.

mdickinson · 2022-04-21T13:18:42Z

+    """
+    face_name = font.face_name
+    if isinstance(face_name, SequenceTypes):
+        face_name = face_name[0]


Is this branch exercised? The Font constructor seems to enforce that face_name is always an instance of str, and I don't seem to hit this branch at all when running the test suite. (If the branch is needed, it might be good to have a test that exercises it.)

This was just copied over from the previous implementation.

I think what this allows is that the function can be used on Font-like objects that can specify "if we don't have font X, font Y is acceptable" via a list of font names (eg. as the Pyface Font does, although this code predates that class). So I don't want to just change it now, but I'll add an issue.

mdickinson · 2022-04-21T13:29:11Z

can you tell me what version of WxPython?

It's whatever was installed by python ci/edmtool.py install --toolkit=wx. Looks like that's wxPython-4.0.7.post2.

jwiggins · 2022-04-22T07:23:11Z

+    def string_value(self, value, format_func=None):
+        if self.factory.sample_text:
+            return self.factory.sample_text
+
+        return super().string_value(value, str_font)


Did you intend to ignore the value of format_func here? And shouldn't it be self.factory.format_func instead of str_font?

Yes, this is intentional, and it's the way that it is because of the terribly convoluted way that TraitsUI editors produce string values. But in any case, for this editor, the only way it is called is from _get_str_value which doesn't supply format_func.

jwiggins · 2022-04-22T07:31:39Z

+        self.font = self.value
+        super().update_editor()
+
+    def string_value(self, value, format_func=None):


Is this some magic TraitsUI function which is used to supply the value of the str_value trait? It's a bit confusing for a naïve reader.

Yes, precisely. The TraitsUI stuff is a mess, but the core need for this is this is what you call if you need a string representation of a possible valid value for the editor. It's used to get the string representation of the current value by _get_str_value, but other editors use it for things like formatting the bounds of a range editor or the possible values of an enum editor.

The typical code-path bounces back and forth between editor and factory and is somewhat confusing. I'll add a docstring with a bit of explanation.

corranwebster · 2022-04-26T12:59:07Z

In manual testing of the demo script (with wx; I haven't yet tried with Qt), I get a traceback if I click on "Cancel" after launching the font dialog with either the "simple" button or the "custom" button.

That is looking like a Pyface issue.

corranwebster · 2022-04-26T14:40:01Z

In manual testing of the demo script (with wx; I haven't yet tried with Qt), I get a traceback if I click on "Cancel" after launching the font dialog with either the "simple" button or the "custom" button.

That is looking like a Pyface issue.

Opened enthought/pyface#1130

corranwebster · 2022-04-26T16:38:03Z

I have opened #932 for the case of multiple font faces.

corranwebster commented Apr 8, 2022

View reviewed changes

Comment thread enable/label.py

corranwebster mentioned this pull request Apr 8, 2022

New, more modern KivaFont trait #929

Merged

corranwebster requested a review from mdickinson April 20, 2022 16:06

mdickinson approved these changes Apr 21, 2022

View reviewed changes

jwiggins reviewed Apr 22, 2022

View reviewed changes

Comment thread enable/examples/demo/enable/editors/font_editor.py Outdated

Comment thread enable/label.py

jwiggins reviewed Apr 22, 2022

View reviewed changes

Improve docstrings and other fixes suggested by code review.

b0b2447

Fix flake8 issues with imports.

15cdcec

corranwebster merged commit c23a50a into main Apr 26, 2022

corranwebster deleted the feat/new-font-editor branch April 26, 2022 16:39

corranwebster mentioned this pull request Apr 27, 2022

Remove font traits and editors from Kiva #934

Open

		@@ -1,4 +1,4 @@
		# (C) Copyright 2005-2022 Enthought, Inc., Austin, TX
		# (C) Copyright 2005-2021 Enthought, Inc., Austin, TX


		return width, height

		def set_size_policy(self, direction, resizable, springy, stretch):

Conversation

corranwebster commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mdickinson commented Apr 21, 2022

Uh oh!

mdickinson commented Apr 21, 2022

Uh oh!

corranwebster commented Apr 21, 2022

Uh oh!

corranwebster commented Apr 21, 2022

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdickinson commented Apr 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranwebster commented Apr 26, 2022

Uh oh!

corranwebster commented Apr 26, 2022

Uh oh!

corranwebster commented Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

corranwebster commented Apr 8, 2022 •

edited

Loading