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

Disabling suggestions on Android if readonly mode is set. #2139

Merged
merged 4 commits into from
Oct 4, 2023

Conversation

Aarontheissueguy
Copy link
Contributor

@Aarontheissueguy Aarontheissueguy commented Sep 28, 2023

Set "TYPE_TEXT_FLAG_NO_SUGGESTIONS" as "InputType" if "readonly" is True and testing it in the backend probe

This ensures that no spelling suggestions are made if a text is set to readonly.

Fixes #2136

"TYPE_TEXT_FLAG_NO_SUGGESTIONS" if readonly is True and testing it in the backend probe
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good, and works well in my testing.

There's a couple of things standing in the way of me merging this:

  1. When you submit a PR that closes a ticket, you need to include the text Fixes #xxx in the PR description; this is the trigger to Github to close the issue when the PR is merged. I've made this modification on your behalf.
  2. Toga's CI requires that every PR include a changenote - this is why CI is currently failing. In this case, we need a file named changes/2136.bugfix.rst , with a single line of content that reads something like "Multiline text inputs no longer show spelling suggestions when in readonly mode."
  3. When you submit a PR, it's really helpful if you submit the PR from a branch of your fork, rather than the main branch. The missing changenote is a fairly minor thing, so ordinarily I would add it myself as part of the review process - but because you've submitted from your main branch, I can't do that: maintainers can edit PRs submitted from a branch, but not PRs submitted from your main. Submitting from a branch also makes your life easier in the long run, because it allows you to have multiple PRs in flight at the same time (after all, you can only have 1 main) - but once your PR is merged, it also means the reconciliation process is easier, because your fork's mainline can stay a "clean" copy of the upstream repository, rather than something you need to merge or rebase.

There's no need to re-work this PR to be from a different branch; if you add the changenote, I'll be happy to merge it. But it's worth keeping in mind for next time.

@Aarontheissueguy
Copy link
Contributor Author

Thanks for the tips, I will keep them in mind the next time I contribute. I added the changenote as requested.

@freakboy3742
Copy link
Member

Getting closer - now the PR is hitting a pre-commit issue. We apply black formatting to format our code, and we check that every commit has had black applied. If you follow the contributors guide you can install pre-commit locally, and have these fixes applied automatically when you commit code. In this case, you'll need to run pre-commit run --all over your code to apply the fixes to previous commits.

@freakboy3742
Copy link
Member

Ok - now we're at a "real" problem, not just a cosmetic one: The test you've added is failing in CI.

@Aarontheissueguy
Copy link
Contributor Author

hmm... Il look into it. Not entirely sure what causes this though.

Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
@Aarontheissueguy
Copy link
Contributor Author

Thats weird, we didn't tamper with anything iOS related, did we?

@freakboy3742
Copy link
Member

The good news (for you) is that the iOS failure has nothing to do with you. I landed an unrelated change to Briefcase this morning, and that is causing problems with Toga's CI.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The CI problem has now been fixed - so this is good to land!

Thanks for contribution!

@freakboy3742 freakboy3742 merged commit 7356cb2 into beeware:main Oct 4, 2023
40 checks passed
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.

Android Read-only Multi-line text input displays suggestions (Spell Checking)
2 participants