Skip to content

Conversation

@hotchemi
Copy link
Contributor

@hotchemi hotchemi commented Feb 17, 2021

Task/Issue URL: #1074

Description:

  • This PR modifies KeyboardAwareEditText to be aware of pasting text from the clipboard and clear its style beforehand.
    • If API level >= M, we can just use R.id.pasteAsPlainText
    • If API level < M, we need to manually clear and set the data to primary clip again(see ClipboardManager.convertClipToPlainText)

Discussion

  • Now KeyboardAwareEditText has kind of two responsibilities and I was thinking I should rename the class as OmniToolbarEditText or something, but since this is my 1st PR as an external contributor I'd like to keep the diff smaller as much as I can. Let me know your thoughts around here🙏

Steps to test this PR:

  1. API level >= M
  1. API level < M

Screenshots

API level >= M
Before After
API level < M
Before After

Internal references:

Software Engineering Expectations
Technical Design Template

By overriding onTextContextMenuItem in EditText.
@cmonfortep
Copy link
Contributor

Thanks, we will take a look as soon as possible.

@malmstein malmstein self-assigned this Feb 18, 2021
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

Thanks for the submission @hotchemi. This is indeed very helpful and fixes an issue, good job!

On the actual code:

  1. How do you feel about not using extension functions? We don't really need this feature anywhere else and I'd prefer if the convertClipToPlainText function would only be known to the KeyboardEditText
  2. On the naming of the EditText itself, we are working on a few things internally that will change its behavior, so it's better not to change anything else just yet.

@hotchemi
Copy link
Contributor Author

hotchemi commented Feb 24, 2021

@malmstein Thank you for the quick review!

  • RE: 1 I'm fine with your suggestion and let me make my aim clear just in case🙏 My intention here for using Kotlin's extension function is to provide a more readable way to define logic and actually those extensions are not available outside of KeyboardAwareEditText(because those are private, which I've occasionally seen in Kotlin repo)🙇

  • RE: 2 Copy that!

@malmstein
Copy link
Contributor

@hotchemi thanks for clarifying, I completely missed the private on the extension functions declaration! It actually reads quite nice and I'm happy with it.

Thanks for the contribution!

Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

Happy with the changes because the extension functions are private to the View
:shipit:

@malmstein malmstein merged commit ef561c4 into duckduckgo:develop Feb 24, 2021
@hotchemi
Copy link
Contributor Author

Thank you so much for your review😄🙏

@hotchemi hotchemi deleted the fix/paste_clip_as_plain_text_in_omnibar branch February 24, 2021 15:18
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.

3 participants