Implemented Divider widget on iOS

[proneon267]

Implemented the divider widget on iOS. I have also enabled divider widget test for iOS and updated the support matrix. Here is the example divider app:

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

[freakboy3742]

Thanks for the contribution. Unfortunately, we're currently experiencing an issue with iOS CI, which is the cause of the test failure you're seeing (See #2476 for details).

However, if you run the test suite locally, you get a coverage failure:

Name                                                                                                                                                                                                                            Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Users/rkm/Library/Developer/CoreSimulator/Devices/478BD004-DC12-4D72-AB6D-D7D1FC6EF0B0/data/Containers/Bundle/Application/9D11144E-CDBD-4213-BEBE-0B6F343D4CCD/Toga Testbed.app/app_packages/toga_iOS/widgets/divider.py           26      1      4      1  93.3%   25
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                                                                                                                                                            2394     68    236      1  97.0%

45 files skipped due to complete coverage.
Test coverage is incomplete

This is because the current divider tests don't currently exercise changing the background color. You can address this by adding:

    test_background_color,
    test_background_color_reset,
    test_background_color_transparent,

to the test list at the start of testbed's test_divider.py; however, this currently fails the test_background_color_reset.

[proneon267]

Thanks! I'll complete them and get back to you soon.

[proneon267]

Done!

[freakboy3742]

The iOS implementation now looks good; however, it's breaking the Android backend, because the new tests aren't passing. This isn't strictly a problem if you're making, but the test needs to pass before this can be merged.

The fix will be an implementation of the background_color probe that is able to correctly interpret the return color in the reset case; see the probe for Button for an example of what can be done here.

[proneon267]

The test was failing because of a bug in Android's set_background_simple() where it wasn't actually setting the color to transparent. I have fixed it.

[freakboy3742]

I'm not sure if this was intentional, but you've opened a can of worms here. Changing the implementation of Android has very different implications to changing the testbed for a single widget - and, as I've noted inline, the change you've proposed raises major red flags because it doesn't break tests, and does open a coverage gap that you've papered over.

Fundamentally, I don't have a major problem with the conceptual change that this is implementing - that is, interpreting background_color=TRANSPARENT as true transparency, rather than "reset to default". However, it is an API change, and one that needs to be accompanied with tests that verify this behavior - as well as some eyeball verification, because Android's relationship with "background color" is complex (see the implementation of the base Android probe for the reason why).

I can see two paths forward here:

1. You can get to the bottom of what background=TRANSPARENT means, and fix the issue in this PR.
2. You can punt on the background color issue, log a ticket describing the ambiguity, implement a workaround fix at the level of the Divider probe (as I originally described), and reference this ticket and the divider probe workaround in the ticket.

In the interests of keeping one idea per PR, I'd strongly recommend (2).

[freakboy3742]

This line was previously covered. If you've made a change that alters that, we have a problem. We either need a new test to exercises this branch, or a very good explanation (documented in the code) why this branch is required, but can't be hit by testing.

[proneon267]

On the testbed summary, I got coverage gap for 245->247. Hence, I thought that the coverage gap was about missing else branch, and so I added # pragma: no branch.

[freakboy3742]

This is changing the behaviour of the set_background_simple, not changing the probe to expect a different value because of the implementation of divider. The fact that you're able to make a fairly radical change to the implementation of a method that is used in many other widgets without also changing tests is a major red flag.

The only way I can see this passing is if the default background for all the widgets that use set_background_simple is TRANSPARENT. At the very least, test_box and test_table don't include test_background_transparent tests, so that might be the cause of the problem; but it's also possible you've exposed a gap in our tests.

Regardless: assuming that there's were no other changes needed, at the very least, this logic can be dramatically simplified:

Suggested change

	if value in (None, TRANSPARENT):
	self.native.setBackground(self._default_background)
	if value is TRANSPARENT:
	self.native.setBackgroundColor(Color.TRANSPARENT)
	if value is None:
	self.native.setBackground(self._default_background)
	elif value is TRANSPARENT:
	self.native.setBackgroundColor(Color.TRANSPARENT)

[proneon267]

When I was testing for transparency of background_color, I found that, this was not setting the background to transparent:

toga/android/src/toga_android/widgets/base.py

Lines 141 to 142 in 1b96ca1

	if value in (None, TRANSPARENT):
	self.native.setBackground(self._default_background)

And so, the probe was returing error, even when I had put an indentical implementation of Button probe's:

toga/android/tests_backend/widgets/button.py

Lines 27 to 30 in 1b96ca1

	@property
	def background_color(self):
	color = super().background_color
	return None if color == TRANSPARENT else color

on the Divider probe. Hence, I changed the implementation.

[freakboy3742]

Yes, but that doesn't address my comment.

I agree that, conceptually, this change makes sense. However, you've made a non-trivial change to the implementation and interpretation of background_color that has no impact on testing output. That indicates that either:

1. The previous implementation was needlessly complex, or
2. The tests are currently inadequate.

Unless you can provide a satisfactory explanation that (1) is true, or you write tests that validate the new behavior (i.e., tests that would fail on the current implementation, but pass on your implementation), this change can't be accepted.

I pointed you at the implementation of background_color on the button probe as an example of the sort of implementation that you could use, not as a "fix" for the divider test specifically. It wouldn't surprise me at all if a different implementation is needed.

[proneon267]

This is changing the behaviour of the set_background_simple, not changing the probe to expect a different value because of the implementation of divider. The fact that you're able to make a fairly radical change to the implementation of a method that is used in many other widgets without also changing tests is a major red flag.
The only way I can see this passing is if the default background for all the widgets that use set_background_simple is TRANSPARENT.

I think on Android, divider widget is the only one that is explicitly setting a background color on initialization:

toga/android/src/toga_android/widgets/divider.py

Lines 13 to 14 in 1b96ca1

	# Background color needs to be set or else divider will not be visible.
	self.native.setBackgroundColor(Color.LTGRAY)

Hence, the default background is also not transparent:

toga/android/src/toga_android/widgets/base.py

Lines 138 to 142 in 1b96ca1

	if not hasattr(self, "_default_background"):
	self._default_background = self.native.getBackground()
	if value in (None, TRANSPARENT):
	self.native.setBackground(self._default_background)

So, should TRANSPARENT mean the default color that the widget sets or actual transparency?

You can get to the bottom of what background=TRANSPARENT means...

Does that mean enabling test_background_transparent on all widgets and then checking for errors?

[freakboy3742]

So, should TRANSPARENT mean the default color that the widget sets or actual transparency?

It depends what you're proposing. Right now, the implementation is clearly saying the background_color=TRANSPARENT resets to the default.

I completely agree that this isn't a great interpretation, and I agree this should be addressed. Modifying this behavior is a reasonable change to propose. However, it is a change in behavior. You cannot change behavior without either:

1. a corresponding change in tests; or
2. An explanation for why the previous behavior was untested; or
3. a very good explanation for why the behaviour can't be tested at all.

You can get to the bottom of what background=TRANSPARENT means...

Does that mean enabling test_background_transparent on all widgets and then checking for errors?

Yes.

In addition, you need to verify that test_background_transparent is actually testing transparency - because at present, it clearly isn't, at least in the Android case.

[proneon267]

Since, we've established in #2667, that the background color of a Divider widget shouldn't be allowed to change, so I have removed any such changes from this PR. Can this be merged now?

[freakboy3742]

Looks good; the only piece missing at this point is the screenshot for the documentation page; as it stands, the docs will say that divider is supported on summary page, but will say not supported on the divider docs themselves.

The screenshot can be generated using the screenshots example app.

[proneon267]

Done :)

[freakboy3742]

Awesome - thanks for the PR!