Add initial macOS probe implementation

[freakboy3742]

An initial set of test probes for macOS.

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

[mhsmith]

On the other platforms I tried to make the probe hierarchy follow the native widget hierachy. For example, on Windows the base Control class has a ForeColor property, so the SimpleProbe implements the color method. But on Android there is no such property, so SimpleProbe.color calls skip, and the widget-specific probes override the method where applicable.

If I understand the Cocoa documentation correctly, there are no color properties in the base NSView class, so we should take the second approach.

[mhsmith]

But maybe I've misunderstood something, because if NSButton doesn't have a backgroundColor property, then how did it pass test_button.py::test_background_color?

[mhsmith]

I guess that's what this page calls a "cover" method for NSButtonCell. But I still can't see where drawsBackground is coming from.

[freakboy3742]

Organising probe APIs to match the underlying platforms makes sense. It does open up an interesting gap where some platforms will have a "color" API on the base widget, but on others it will be platform specific; but I guess that will manifest as a test failure/error due to missing a required probe API.

You're correct that backgroundColor is a cover API; however, it looks like the base widget usage of drawsBackground is an accidental pass. drawsBackground only exists on a few widgets (NSTextField and NSScrollView being the most notable); it doesn't exist on NSButton (even as a cover method), but you can set an arbitrary property on an Rubicon ObjC object and it will just be annotated on the object.

However - this becomes an interesting case of somewhere that checking properties won't necessarily tell us that something is working correctly. This is the current output of the colors example on macOS, with a background color of green, and a color of blue.

The button does have something that could be called the background color altered, and the backgroundColor property is correct; but it's not changing the background color (at least, not in the way you'd expect).

I guess this comes down to verifying that the behaviour (and the probe) is actually correct at the time of implementation.

[mhsmith]

Yes, it'll be the responsibility of the probes to check whatever native properties are appropriate for each widget.

[mhsmith]

For future reference: see related discussion below.

[mhsmith]

pytest is supposed to print the actual and expected values automatically, but I guess it didn't work because the "assertion rewriting" feature is only on by default for code directly inside of test modules.

I previously tried fixing this by adding the following to tests/__init__.py, but it didn't have any effect, at least on Windows:

import pytest
pytest.register_assert_rewrite("tests")

I think we should get to the bottom of this and fix it properly rather than cluttering all of our assertions like this.

[freakboy3742]

Looks like register_assert_rewrite("tests") will work - but you need to include it in conftest.py.

[mhsmith]

I'm not sure about adding this to the public API, as it's unlikely to be useful outside of the Toga testbed. And as we mentioned the other day, whenever a property changes we should refresh things as necessary, just like the HTML DOM does. So any time an app needs to call refresh manually, that means there's a bug in Toga.

Instead, how about adding this as a method on the probe class? Then it could be implemented at either the window level or the widget level, depending how redrawing works on each platform.

[freakboy3742]

I'm not sure that we would want to force a refresh after every update - at least, not without also adding another mechanism to suspend those updates.

Consider the case where you're updating a whole lot of widgets in a single handler - say - a big HUD style GUI with 100 widgets. At present, you can update each of the 100 widgets, and they'll all get refreshed when the handler exits and the redraw loop happens. If we bake a redraw into every widget change, there's going to be 99 "in-progress" redraws before the final update and redraw. Aside from the potential for drawing the UI in an "incomplete" state, that's going to be a pretty heavy redraw load. GUI paint operations aren't cheap, and they're the most likely to be observed by users as a "performance problem", because they'll manifest as graphical glitches you can clearly see.

The only way this approach would work in practice is if we also added a "pause updates" context manager that instructs Toga to not do per-widget updates while in the graphical context - this sort of thing is reasonably common for graphical APIs, so it wouldn't be completely weird; but it means decorating your code with "pause" wrappers anywhere that you've got moderately complex GUI updates.

The alternative is what I've got here. By default, redraws happen "as soon as practical". While I agree that this is somewhat contrary to a beginners expectation, it's also something that is reasonably easy to explain; and I'd argue that in most cases, it's the behavior you actually want, because it is optimised for visual performance. However, if you need the GUI to update right now, await redraw() is an explicit way of doing the await asyncio.sleep(0.01) workaround. It's not something you need in the common case, but it's there if you do need it, and the default behaviour is semi-optimized.

[freakboy3742]

Following discussion in person; an async redraw() coroutine won't actually be that useful in practice, as the most likely use case is someone wanting to update in the middle of a synchronous handler; and the real fix for that is to not use a synchronous handler. On that basis, I'll demote redraw() to being a probe method.

[freakboy3742]

I've finally got to the bottom (or, at least, the side) of the iOS crash problem; it appears caused by the cleanup process of coverage. We'll need to tackle that as part of adding the iOS test backend, but for now, we can punt on it.

So - I think this is ready for another review pass. Some review notes:

• It will need to be rebased onto/merged with Modify dependency pins and definition. #1731 (and that PR contains some elements of this one)
• The colors demo was fairly badly broken by the get_widget_by_id change; I needed to use that app to test the fixes for color handling on macOS, so there's a bunch of changes there.
• There should be 100% coverage for button and label widgets, and the docstrings for those widgets should be fully up to date.

[freakboy3742]

Looks like the iOS CI failure was one of the transient ones (😓). Passed on a re-run.

[mhsmith]

I'll do a full review of this PR on Monday.

[mhsmith]

It will need to be rebased onto/merged with #1731

I've now done this.

[mhsmith]

As discussed, this should probably be addressed in a separate PR. But here are some ideas for that PR:

What if we simply didn't let the main thread die? After the call to main_loop, put a wait on a condition to block the thread, and then notify that condition in the on_exit handler. Then the main thread will resume after the test suite finishes, and it can deal with stopping the coverage and printing the exit status.

[freakboy3742]

I thought of (and tried) that, but it doesn't work. On both iOS and Android, we rely on the main Python code block exiting to start the native event loop. On iOS, the call to create UIApplicationMain() happens immediately after the Python main block exits; if you don't call that method, you don't get GUI updates. On Android, blocking in the main block effectively blocks onCreate() from returning, which effectively locks the app.

[mhsmith]

Oh right, of course.

[mhsmith]

I imagine we might need a method like this eventually, but maybe we don't need it here. What if ButtonProbe just overrides the height property so that it calls through to the superclass implementation and then runs this assertion? Then the tests don't need to be cluttered with all the assert_display_properties calls, and the other probe classes don't need all the empty implementations.

I did something similar with the Slider here: implementing one probe property in terms of two native properties, while asserting certain constraints on the combinations of values they can take.

[freakboy3742]

I don't particularly like having a standalone assertion class either (especially given it will be empty on most implementations) - but having assertions on an attribute probe bugs me as well. I think it makes a certain amount of sense to maintain a separation between "things being measured" and "things being asserted".

That said, I think you may be right. Completely aside from the issue of having mostly empty implementations, for Button on Cocoa, we need to assert that when the button's height changes, the properties of the button also change. That obviously happens when a test changes the size of the button, but it also happens implicitly if a test changes the font - something that the current font change property tests doesn't check for.

So - as a sort of "general policy" statement, I guess it makes sense to say that if an implementation-specific property (like bezelStyle) is a direct consequence of an interface-level property (like height), we should include an assertion in the probe's attribute access.

If I'm reading the slider example correctly, the same is true there as well. There's an interface level probe property reporting the number of ticks, which returns None if the widget doesn't have ticks; but there's also a specific implementation requirement that if the widget hasn't been configured for tick marks, numberOfTickMarks must be 0. I'm not sure if this is because of some API issue where having numberOfTickMarks != 0 and allowsTickMarkValuesOnly == False causes a GUI issue, or it's just a case of keeping a clean house internally - but either way, it's a native level concern stemming from an observable interface property, and changes to the interface property should impact on the native rendition of that property.

[mhsmith]

I assume this was added for the benefit of the Cocoa button bezel style. But doesn't this mean that the bezel will only respond to a button with an explicit height attribute, as opposed to one that's been automatically sized by the Pack layout algorithm? That seems inconsistent with the assertions in the Button probe.

Could we fix this by making the Cocoa Button override set_bounds instead?

[freakboy3742]

You're correct that this was added to deal with the bezel style.

The approach of adding set_size() was the result of me overthinking this a bit. The concern I was trying to avoid is that macOS buttons (and buttons generally) don't expand vertically to fill space - and as a result, on cocoa, we only need the bezel change when the height is set explicitly. Adding set_size() meant we could respond directly to setting the height of the widget, rather than implicitly responding whenever the bounds are set.

However, in practice, it won't actually change the observed behavior. Absent of a specific height value, the bounds height of a button will be the intrinsic height - the bounds of the button don't expand to fill the available space, even if the layout would allow it..

[mhsmith]

If most widgets will need the same implementation of this method, wouldn't it make more sense to put it in the base class and override it where necessary?

[freakboy3742]

This has come up in the past - It turns out supporting background color actually isn't the common case.

Cocoa currently has 22 widgets (excluding the base class); of those, only 8 support setting the background color (with PasswordInput inheriting TextInput's implementation), and one of those (Button) has a different implementation to the others. Of the remaining widgets, it's not clear if they should support background colors (ActivitySpinner? Divider? SplitContainer?) - and if they should, that the implementation will be the common implementation (Table? Tree?)

Given that, do we implement:

1. A base class that provides a default implementation, overridden on subclasses that have a different implementation, or no support for background colors; or
2. A base class with no implementation, and a duplicated implementation on the classes that need it.

The code currently does (2), on the basis that not supporting set_background_color is the common case; but as you've noted, it does leads to some code duplication. Maybe this is a case for a mixin or a utility method; to date it hasn't been enough of an issue to warrant the effort/complication.

[freakboy3742]

... and now I do a review of the actual diff, and I realise that I was reversing the previous implementation.

[mhsmith]

For future reference: see related discussion above.

[mhsmith]

Why is str necessary: what other type could it be? IAlso applies to Button.)

[freakboy3742]

The type returned by Rubicon will be an NSString (or, strictly, an ObjCStrInstance wrapper of __NSCFString). which will mostly behave like a string for most common usage, but aren't completely compatible. There's a couple of reasons to not autoconvert to str (see the note here). However, to make sure that Objective C types don't leak into user space, the cocoa and iOS backends cast to str(); I figured the probe should do the same.

[mhsmith]

Thanks: I don't see a clear explanation in the documentation of why it doesn't autoconvert, but I found beeware/rubicon-objc#101 and the linked Gitter discussion, which says it's to allow Python code to call NSString methods like sizeWithAttributes.

[mhsmith]

For future reference: see related discussion above.

[freakboy3742]

I think I've addressed all your feedback; we're now waiting on a Rubicon-objc release that includes beeware/rubicon-objc#246, and a related bump to the macOS and iOS dependency pin.