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.MultilineTextInput #1938

Merged
merged 43 commits into from
May 25, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented May 10, 2023

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

@@ -14,9 +17,8 @@

class TogaTextView(NSTextView):
@objc_method
def touchBar(self):
# Disable the touchbar.
return None
Copy link
Member Author

Choose a reason for hiding this comment

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

This was needed in the past to avoid a crash if the app ran on a MacBook with a Touch Bar; however, touchbars are no longer present on modern Macs, and in my testing on an older macBook with a Touch Bar, the bug no longer manifests.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

The Android implementation is now complete, though there are still some bugs with scrolling and background colors which I'll look at tomorrow.

Meanwhile, here's a partial review:

Comment on lines 46 to 48
@property
def placeholder(self):
"""The placeholder text.
"""The placeholder text for the widget.
Copy link
Member

Choose a reason for hiding this comment

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

This, and the other properties, should have type annotations, as in Slider. At some point we should go back and do all the other widgets as well – but not in this PR, because it's big enough already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - I noticed this myself when working on TextInput; I'll wait for you to finish your pass so I don't step on your toes (or, if you feel enthused, feel free to add annotations yourself)

Copy link
Member Author

Choose a reason for hiding this comment

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

Scratch that - I'll do a pass now. Can't step on your toes while you're asleep :-)

docs/reference/api/widgets/multilinetextinput.rst Outdated Show resolved Hide resolved
core/src/toga/widgets/multilinetextinput.py Outdated Show resolved Hide resolved
docs/reference/api/widgets/multilinetextinput.rst Outdated Show resolved Hide resolved
core/src/toga/widgets/multilinetextinput.py Outdated Show resolved Hide resolved
Comment on lines +68 to +70
async def wait_for_scroll_completion(self):
position = self.vertical_scroll_position
current = None
Copy link
Member

Choose a reason for hiding this comment

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

There's no platform-specific code in this method, so it could be moved to the testbed and avoid having empty implementations for all the other backends.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed it would be desirable to avoid needing to define an empty method; however, it would require introducing a sleep into platforms that don't need it.

cocoa/tests_backend/widgets/multilinetextinput.py Outdated Show resolved Hide resolved
Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

[Non-deleteable comment caused by GitHub glitch]

Comment on lines +35 to +37
@property
def vertical_alignment(self):
return toga_vertical_alignment(self.native.getGravity())
Copy link
Member

Choose a reason for hiding this comment

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

This will have to be implemented by the other backends.

Copy link
Member Author

@freakboy3742 freakboy3742 May 19, 2023

Choose a reason for hiding this comment

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

I've done this for iOS, Cocoa, GTK and Winforms; although for iOS and Cocoa, it's a bit of a non-test because there's literally no property to inspect to verify vertical alignment - it's just what the widget does (or, in the case of iOS Label, what the widget has been overridden to do).

@freakboy3742
Copy link
Member Author

In the process of debugging a test for NumberInput, I've found a bug with all the text input APIs - we've overloaded the clear() API. TextInput and MultilineTextInput both define a clear() method that removes all content; but the base Widget also defines clear() that removes all children. As TextInput/MultilineTextInput don't have children, there isn't any functional issue; but it's obviously a confusing naming overlap.

The clear() name was only added recently to Toga (#1893); but the name was inherited from Travertino as a core node operation.

I can see a couple of ways forward:

  1. Ignore the problem. It isn't functionally ambiguous, as input widgets will always be leaf nodes.
  2. Remove clear() as an API on text inputs. This would be backwards incompatible, but the functionality is achievable with self.value = "".
  3. Rename clear() on the base node (and possibly also Travertino) - clear_children() perhaps? If we do this, we might need to consider renaming add, insert and remove as well for consistency. If we do this rename, we may also need to re-introduce add/insert/remove/clear APIs for Box as proxies, as this is a commonly used API for constructing layouts.
  4. Remove clear() as a method on the base node, and only make it available as an API on Box (which actually has children). Again, for consistency, there might be an argument to be made for removing add, insert and remove as well, as these can only be used on Box in practice. This won't hide it from the base node though, unless we also modify Travertino to make these APIs private.

I'm not sure which one I prefer at this point, but I probably fall somewhere between 2 and 3. Thoughts @mhsmith?

@mhsmith
Copy link
Member

mhsmith commented May 21, 2023

I think we should go with 2. Remove clear() as an API on text inputs. This method was hardly saving any typing anyway, compared to value = "".

Copy link
Member Author

Choose a reason for hiding this comment

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

Should these be here, or in android.graphics?

Copy link
Member

Choose a reason for hiding this comment

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

At some point we should remove the libs folder and use Chaquopy's from ... import ... syntax, so we don't need to edit 2 files every time we import a new class. To make it easy to do that with a simple search and replace, I've made drawable a submodule of graphics.

@freakboy3742
Copy link
Member Author

I think we should go with 2. Remove clear() as an API on text inputs. This method was hardly saving any typing anyway, compared to value = "".

Done - I've removed it (and added a deprecation note).

@mhsmith
Copy link
Member

mhsmith commented May 23, 2023

Since Winforms displays the widget's value and placeholder through the same mechanism, only one of them at a time can be retrieved through the native API. So I've merged the value and placeholder properties on the probe into a single value property, which always returns whatever text is visible to the user.

I also added a few additional assertions, one of which verifies that a typed character is actually visible after the on_change event fires. The assertion is currently on probe.value, but it should probably check widget.value as well. Anyway, this is currently failing on iOS, maybe because the placeholder isn't being hidden correctly.

@freakboy3742
Copy link
Member Author

Success!! I've cleaned up the iOS error - the cause was a weird edge case where the widget already had focus, and was programmatically cleared. The same error was technically present on GTK (and I think Windows) as well, but wasn't visible because of the way the keyboard handler was triggering.

I've also done a small cleanup on the Winforms implementation, removing the need for a proxied copy of the text of the widget; and added a documentation note about the color limitations on Winforms transparent backgrounds.

With that, I'm happy with what is here; if you're happy to, we're good to merge!

@mhsmith mhsmith merged commit 6f16eef into beeware:main May 25, 2023
@freakboy3742 freakboy3742 deleted the audit-multilinetext branch June 26, 2023 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants