Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[widget Audit] toga.Canvas #2029

Merged
merged 67 commits into from Oct 5, 2023
Merged

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jul 11, 2023

Audit of Canvas.

Builds on #1903 as an audit of Font.

This involves a number of backwards incompatible changes:

  • Canvas is no longer its own context. This previously allowed the use of simple drawing operations on the canvas itself, which is fine; but the context also allows for manipulation of primitive drawing operations using remove and clear, which collide with Widget's child manipulation operations of the same name. Interestingly, the method to add was called add_draw_obj, and was explicitly undocumented. To avoid the collision, Canvas now has a context property that returns the root context of the canvas. Simple drawing operations have been retained for backwards compatibility, redirecting to the default context; context generating operations have been retained.
  • To make it clear(er) when an operation is a context producing operation, rather than a simple drawing operation, context-producing operations have been renamed to TitleCase. This also avoids a collision caused by introducing the Canvas.context property; the operation on canvas to produce a new sub-context is now Canvas.Context().
  • The preserve option on a Fill() context has been removed. It appears to be an internal optimization for GTK that isn't required, and isn't easy to explain.
  • The new_path simple drawing operation has been renamed begin_path for consistency with HTML5.
  • The "clicks" argument to event handlers has been removed, in favor of a "on_activate" handler.
  • On GTK, font and stroke handling isn't quite right. It works (to first approximation). This seems to be deeply seated in how fonts are rendered by Cairo; and there's a testing issue with consistent availability of fonts.

Fixes:

Audit checklist

  • Core tests ported to pytest
  • Core tests at 100% coverage
  • Core docstrings complete, and in Sphinx format
  • Documentation complete and consistently formatted
  • cocoa backend at 100% coverage
  • winforms backend at 100% coverage
  • gtk backend at 100% coverage
  • iOS backend at 100% coverage
  • android backend at 100% coverage
  • Widget support table updated
  • Widget Issue backlog reviewed

@mhsmith
Copy link
Member

mhsmith commented Jul 13, 2023

Yes, those changes all look reasonable.

@freakboy3742 freakboy3742 mentioned this pull request Jul 15, 2023
4 tasks
@freakboy3742
Copy link
Member Author

freakboy3742 commented Jul 16, 2023

So I don't forget where I'm up to: the test failures at this point are all due to weird inconsistencies between CI behavior and desktop behavior.

Linux:

  • multiline text: Desktop multiline_text-linux CI multiline_text-linux

iOS:

  • multiline text: Desktop multiline_text-iOS CI multiline_text-iOS

macOS:

  • multiline text: Desktop multiline_text-macOS CI: multiline_text-macOS

  • transparency: Desktop: transparency CI: transparency-macOS

The differences on multiline text all seem to be related to line leading. It may also be due to an image dimensions vs image resolution discrepancy. The iOS test images report dimensions of 100x100, but a resolution of 144x144. It's not clear to me how this impacts pixel inspection calculations on Pillow.

The transparency differences are due to the alpha calculation on the transparent rectangle.

@freakboy3742
Copy link
Member Author

@mhsmith Ok - after further investigation, I think we're stuck between a rock and a hard place.

In the multiline text case, AFAICT the discrepancies are caused by font leading, which is caused by subtle differences in the font itself between OS versions. I'm not sure I see any way to work around this; and in the Linux case, it's going to be hugely dependent on what the local system, GTK theme, and more.

In the transparency case, I can reproduce the problem moving between my M1 MacBook and my x86_64 Mac mini. The issue appears to be that recent MacBooks have a different display profile, so they operate in sRGB colourspace; older Macs don't do this by default - although the colourspace is extremely configurable.

So - the options I see are:

  1. Have multiple reference images. This will let us pass CI, but it will be difficult to guarantee a CI configuration that will pass for every setup.
  2. Use a weaker measure of similarity for the test. For example, in the multiline text case, we could count the number of white pixels in the image, and ensure that number is in a given bound; and maybe refer to individual pixel checking to ensure that some locations are the right color (picking a location like the middle of an L upright as the target location). This will make it much easier for a problem to slip in, as the conditions for "passing" the test are much looser.

Any preferences or alternatives that I may have missed?

@freakboy3742
Copy link
Member Author

Actually - thinking about this some more, there's another option (for macOS and iOS anyway) - we can make the variants dependent on the macOS/iOS version.

The same approach might work for GTK; but we'll need more variants. However, it's a moot point at present because GTK font rendering has deeper problems.

@mhsmith
Copy link
Member

mhsmith commented Sep 29, 2023

I had a similar issue on GTK; the solution is to lean on the fact that a "Bézier" is actually a "Bézier cubic", and a quadratic curve is just a dimensionally reduced Bézier cubic.

Done.

It's also not 100% clear to me if the larger font size is in the bounds of "expected inconsistency", or if the DPI scaling is off in this case.

In this case it's not caused by DPI scaling, but by differences in the definition of a "point". Windows matches the CSS definition of 1.33 logical pixels, while Apple uses exactly 1 logical pixel. Fixed by increasing font sizes on Apple, as mentioned above.

Android does have a "point" unit in its API, but it's not generally used, even for font sizes, and doesn't have a clearly documented definition, so I'll use 1.33 logical pixels there as well.

Copy link
Member

Choose a reason for hiding this comment

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

I can only explain the overlapping stroke on the "a" as being a bug in the font itself. But when we implement font loading support on Cocoa, this file will be replaced anyway.

@mhsmith
Copy link
Member

mhsmith commented Sep 30, 2023

I've changed the default WinForms font to respect the system theme, a change which has already been made in newer versions of .NET (dotnet/winforms#656), though as I understand it, not in the version of .NET which comes with Windows itself.

The difference is not dramatic, but readability is definitely improved: see screenshots at dotnet/winforms#656 (comment). Notice that the title bar was already using the system theme, and the menu bar was too.

The concerns in that comment don't apply to us because we're not using "hand placed controls with pixel perfect layout".

Copy link
Member Author

Choose a reason for hiding this comment

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

Art immitates life... the test case now reflects how we feel about the Canvas widget :-)

@freakboy3742 freakboy3742 merged commit 30fabb4 into beeware:main Oct 5, 2023
41 checks passed
@freakboy3742 freakboy3742 deleted the audit-canvas branch October 5, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants