Add line_height_factor to canvas #3217

jannikhoesch · 2025-02-28T15:24:13Z

The Canvas write_text and measure_text methods accepted multiple lines, but the spacing could not be controlled. By adding a line_height_factor this can now be modified, for example by using a slider, which adjusts the distance between two lines (Visual demonstration in canvas example).

Added to the following backends:

android
cocoa
gtk
iOS
winforms

Fixes #2144

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

Co-authored by: @Heroldpls
Co-authored by: @amaekh
Co-authored by: @Trighap52
Co-authored by: @karindev

Added line height factor with default value of 1.2 to macOS backend. Fixes #7

Added a line_height parameter to write_text, _text_path and measure_text, in order for the user to be able to customize the line height of their text. Default value is currently 1.2 if no other value is specified. This addition was done for the Windows version of the application. Fixes #3

* reformat canvas.py in order to pass pre-commit checks * add a change file for the issue we're fixing (passes the Towncrier Check) closes issue #15

Fixes #17

* Add line_height_factor to write_text and measure_text * Default value set to 1 Closes #2

Fixes #21

* Update measure_text and write_text tests * Add new test case for measure_text with multiline string * Update dummy backend to be compatible with api changes Closes #5 --------- Co-authored-by: jannikhoesch <[email protected]>

Added the line_height_factor to the methods write_text, measure_text in iOS canvas.py

Fixes #22

Fixes #34

Fixes #28

Fixes #37

Fixes #41

Fixes #1

freakboy3742

I've only done a quick pass over this PR so far - but on the whole, it looks really solid. Nice work, everyone!

Two (relatively minor) things stood out in my initial review:

line_height_factor is a bit verbose as a name for a public-facing API. I'd lean toward using line_height, mirroring the CSS property managing the same property. Right now, it will only handle floats; but in theory, with some extra work, it could be expanded to handle all the values that are legal in CSS (I'm not saying this is necessary for this PR to be accepted - just that would be an area that could be explored in future).
This looks to be thoroughly tested in the core API, but the backend APIs aren't tested at all. The PR requires a change the to testbed app to prove that the backends all work as expected. This will be either a modification to the multiline_text test, or a new test following the same general pattern, but exercising multiline text with line height.

(1) is a relatively simple rename; (2) needs a bit more work. The canvas testbed tests are a little annoying to write, because they involve comparison against reference images - and the reference images can be subtly different on every machine depending on specific OS versions, fonts installed, display properties and more. The reference images that exist in the repo are what is produced in CI conditions.

So - the suggested approach here is to write a testbed test that looks like it's doing the right thing, but with a "known incorrect" reference image. You then push the PR to CI, where it will also fail - but as part of the failed run, the "correct" reference images for each platform should be saved and attached to the CI run. You can then download those images, check that they actually look right, add them to the PR as the "real" images, and re-submit the PR - at which point, it should pass.

I hope that explanation makes sense - if it doesn't let me know and I can fill in any gaps.

jannikhoesch · 2025-03-02T10:40:19Z

Thanks for your feedback! We are going to have a look at it and try to improve the points you mentioned.

mhsmith · 2025-03-03T19:02:38Z

We usually follow CSS for the definitions of properties as well as their names. And CSS defines line-height as a ratio of the font size, not the default line height. The default is usually somewhere around 1.2, but it depends on the font.

So I think this argument should accept two options:

None (default) – the current behavior
A float – a multiple of the font size.

See #2144 for more details.

Closes #45

renamed line_height_factor to line_height, fixes #51

jannikhoesch · 2025-03-06T15:45:34Z

We are almost done fixing all the suggestions. One thing that does not work is that the android testbed does not save an image when failing in the CI. Is there another way to get this reference image?

* add None as valid parameter for line_height * modify measure_text test to include test for None value

freakboy3742 · 2025-03-07T00:53:21Z

We are almost done fixing all the suggestions. One thing that does not work is that the android testbed does not save an image when failing in the CI. Is there another way to get this reference image?

I presume you mean a local run of the CI test suite? I'm not seeing any CI runs on this PR. If so, yes - it is possible... just a little complicated. An Android emulator is essentially an entire VM; so if you want to get files off the device, you need to exfiltrate them from the VM image.

The easiest way I know of to do this is with the tools provided by Android Studio - if the VM is running, Android Studio's simulator tools will let you inspect the filesystem of the device, and copy those files out to your local machine. You'll be looking for the "data/canvas" folder in the app's files. This SO answer gives a high level summary of the process. That page also has a version of the same instructions using adb from the command line.

Longer term, it might be nice to wrap those commands up into something Briefcase can do - being able to populate and interrogate user-space files could be a useful capability.

Fixes #36

feat: change line_height to scale by font size instead of default line_height to match CSS

Prepare PR

* feat: Add android canvas image * fix: update testbed references --------- Co-authored-by: jannikhoesch <[email protected]>

jannikhoesch · 2025-03-07T11:29:52Z

It should now be fixed:

Line height accepts none values
Uses ratio of font size instead of default line height
Renamed line_height_factor to line_height
Added testbed tests

Please review again

freakboy3742

Awesome work - Thanks for the updates!

The code structure and testing is dead on - no feedback to give there. The only issue that is outstanding is the specifics of the font height calculation. You've used the "ascender height" fairly consistently as the base from which to construct the line height; while that value should be very close to the font size, my understanding is that the font size isn't required to match the ascender size. This blog post does a decent explanation of font geometry.

As far as I'm aware, we should be literally using the font-size value (in points); that might require other changes to convert locally into PX sizes. In the case of macOS and iOS at least, ascender is already given in points, so switching to font size should be fairly straightforward.

(Of course, if you're able to defend using ascender on some grounds that I've either misunderstood or misinterpreted what is going on, let me know! I know a decent amount about fonts, but I won't claim to be a domain expert. It's possible I've got something wrong here!)

freakboy3742 · 2025-03-09T03:11:54Z

cocoa/src/toga_cocoa/widgets/canvas.py

+            # descender is a negative number.
+            return ceil(font.native.ascender - font.native.descender)
+        else:
+            return font.native.ascender * line_height


Are you sure this should be ascender, and not fontSize? The two are likely very similar, but I'm not sure they're required to be identical.

freakboy3742 · 2025-03-09T03:14:47Z

gtk/src/toga_gtk/widgets/canvas.py

+            # get_height was added in Pango 1.44, but Debian Buster comes with 1.42.
+            scaled_line_height = ascent + descent
+        else:
+            scaled_line_height = ascent * line_height


Again - I'm not sure "ascent" is necessarily the same as the font size.

freakboy3742 · 2025-03-09T03:15:06Z

iOS/src/toga_iOS/widgets/canvas.py

+            # descender is a negative number.
+            return ceil(font.native.ascender - font.native.descender)
+        else:
+            return font.native.ascender * line_height


Same ascender vs fontSize issue

freakboy3742 · 2025-03-09T03:16:26Z

winforms/src/toga_winforms/widgets/canvas.py

+        if line_height is None:
+            return font.metric("LineSpacing")
+        else:
+            return font.metric("EmHeight") * line_height


Same issue here - but I'm more certain Em height isn't the same as font size.

freakboy3742 · 2025-03-09T03:31:15Z

changes/2144.feature.rst

@@ -0,0 +1 @@
+A line_height argument can now be used to change the line height of multiline text.


Suggested change

A line_height argument can now be used to change the line height of multiline text.

The line height of multiline text on a Canvas can be configured.

jannikhoesch and others added 17 commits February 24, 2025 17:00

feat: Add line height factor

9c3a3a8

feat: Update macOS backend (#11)

d9466fb

Added line height factor with default value of 1.2 to macOS backend. Fixes #7

refactor: Improve formatting of write_text and measure_text (#16)

0acea1f

* reformat canvas.py in order to pass pre-commit checks * add a change file for the issue we're fixing (passes the Towncrier Check) closes issue #15

feat: Change default value of line_height_factor (#18)

1c44b4f

Fixes #17

Add line_height_factor to write_text and measure_text #2

f73e493

* Add line_height_factor to write_text and measure_text * Default value set to 1 Closes #2

feat: line_height_factor iOS (#29)

1bb06c4

Fixes #21

Update core api tests to account for line_height_factor

44bf721

* Update measure_text and write_text tests * Add new test case for measure_text with multiline string * Update dummy backend to be compatible with api changes Closes #5 --------- Co-authored-by: jannikhoesch <[email protected]>

feat: line_height_factor for linux (#31)

e4cc9b8

Added the line_height_factor to the methods write_text, measure_text in iOS canvas.py

feat: line_height_factor for iOS (#30)

4ca6962

Fixes #22

fix: remove default value and correct parameter order (#35)

ce0160c

Fixes #34

fix: fix windows backend bug (#33)

35e597f

Fixes #28

fix: Fix bug in iOS backend (#38)

c916546

Fixes #37

fix: fix bug in linux backend (#42)

fdf4b9a

Fixes #41

feat: add line height slider to canvas (#32)

3048a93

Fixes #1

doc: delete changefiles

f248714

doc: edit changefile

ee512b6

freakboy3742 requested changes Mar 2, 2025

View reviewed changes

karindev and others added 2 commits March 4, 2025 13:12

fix: missing multiply by line height factor

501f3c1

Closes #45

refactor: change line_height_factor to line_height (#53)

c61fb7e

renamed line_height_factor to line_height, fixes #51

karindev added 2 commits March 6, 2025 16:52

feat: add None as valid parameter for line_height

766cbe6

* add None as valid parameter for line_height * modify measure_text test to include test for None value

Merge branch 'beeware:main' into main

0b14e10

jannikhoesch and others added 4 commits March 7, 2025 10:20

feat: add line height test to testbed (#44)

e39a6c3

Fixes #36

feat: change line_height to scale by font size

5f1bc44

feat: change line_height to scale by font size instead of default line_height to match CSS

Merge pull request #63 from Heroldpls/main

2603c76

Prepare PR

fix: update testbed reference images

3b9190d

* feat: Add android canvas image * fix: update testbed references --------- Co-authored-by: jannikhoesch <[email protected]>

jannikhoesch and others added 2 commits March 7, 2025 12:04

fix: update testbed reference images

9098a9b

doc: Remove changefiles

8823ce5

freakboy3742 requested changes Mar 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add line_height_factor to canvas #3217

Add line_height_factor to canvas #3217

jannikhoesch commented Feb 28, 2025 •

edited

Loading

freakboy3742 left a comment

jannikhoesch commented Mar 2, 2025

mhsmith commented Mar 3, 2025

jannikhoesch commented Mar 6, 2025

freakboy3742 commented Mar 7, 2025

jannikhoesch commented Mar 7, 2025

freakboy3742 left a comment

freakboy3742 Mar 9, 2025

freakboy3742 Mar 9, 2025

freakboy3742 Mar 9, 2025

freakboy3742 Mar 9, 2025

freakboy3742 Mar 9, 2025

		@@ -0,0 +1 @@
		A line_height argument can now be used to change the line height of multiline text.

	A line_height argument can now be used to change the line height of multiline text.
	The line height of multiline text on a Canvas can be configured.

Add line_height_factor to canvas #3217

Are you sure you want to change the base?

Add line_height_factor to canvas #3217

Conversation

jannikhoesch commented Feb 28, 2025 • edited Loading

PR Checklist:

freakboy3742 left a comment

Choose a reason for hiding this comment

jannikhoesch commented Mar 2, 2025

mhsmith commented Mar 3, 2025

jannikhoesch commented Mar 6, 2025

freakboy3742 commented Mar 7, 2025

jannikhoesch commented Mar 7, 2025

freakboy3742 left a comment

Choose a reason for hiding this comment

freakboy3742 Mar 9, 2025

Choose a reason for hiding this comment

freakboy3742 Mar 9, 2025

Choose a reason for hiding this comment

freakboy3742 Mar 9, 2025

Choose a reason for hiding this comment

freakboy3742 Mar 9, 2025

Choose a reason for hiding this comment

freakboy3742 Mar 9, 2025

Choose a reason for hiding this comment

jannikhoesch commented Feb 28, 2025 •

edited

Loading