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

Correct iOS delegate method prototype #1963

Merged
merged 2 commits into from
Jun 6, 2023

Conversation

freakboy3742
Copy link
Member

After the merge of #1946, we've been seeing intermittent failures on two iOS tests:

  • tests/widgets/test_multilinetextinput.py::test_focus
  • tests/widgets/test_multilinetextinput.py::test_placeholder_focus

The errors are both caused when the widget being tested loses focus as a result of another widget being explicitly given focus. In CI conditions, the widget doesn't lose focus as a result of call to give the other widget focus.

I haven't been able to reproduce this problem locally; however, via #1960 I have been able to rule out it being an issue of the event loop not catching up - a longer wait after the focus call doesn't make the test pass; this indicates that the widget is refusing to give up focus.

The willingness of a UITextView widget to give up focus is controlled by its delegate; specifically, the textViewShouldEndEditing: selector. This method returns True if the view is allowed to lose focus.

The current implementation decleares this method, but doesn't declare a return type on the delegate method. ObjC only uses the argument prototype for matching, so the delegate was being found; however, the return type wasn't declared, so Rubicon would have been interpreting the True value as a c_void_p before passing to the ObjC runtime. In most cases, this will result in the right answer, but clearly there is an edge case where it could fall through and be interpreted as False.

The problem appears to be fixed by explicitly providing a return value of bool on the delegate method; but we're not actually doing anything in our custom implementation of the method, and implementation is explicitly described as optional, with a default behavior equivalent to returning YES, so we can simplify our code by removing the method entirely.

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 mhsmith merged commit 86ee779 into beeware:main Jun 6, 2023
@freakboy3742 freakboy3742 deleted the focus-handler-fix branch June 6, 2023 06:49
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