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

Use custom TextViews and EditTexts in all XML resources #7061

Merged
merged 8 commits into from
Oct 27, 2021

Conversation

AudricV
Copy link
Member

@AudricV AudricV commented Sep 5, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • migrate all TextViews to AppCompatTextViews
  • migrate all EditTexts to AppCompatEditTexts
  • add two new classes in the view subpackage: NewPipeEditText (which extends from AppCompatEditText) and NewPipeTextView (extends from AppCompatTextView).
  • use these new classes in all XML ressources

This allows NewPipe to show the Android share sheet on EMUI devices (instead of the EMUI share sheet) when sharing selected text in TextViews and EditTexts, by using ShareUtils#shareText(Context, String, String).

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@AudricV AudricV added the feature request Issue is related to a feature in the app label Sep 5, 2021
@TobiGr
Copy link
Contributor

TobiGr commented Sep 5, 2021

what's the advantage of showing the native Android share sheet?

@AudricV
Copy link
Member Author

AudricV commented Sep 5, 2021

See the two first screenshots of #5187 to find the difference: there is no native Copy to clipboard option and no quick actions in EMUI's share sheet.

@opusforlife2 opusforlife2 changed the title Use custom TextViews and EditTexts in all XML ressources Use custom TextViews and EditTexts in all XML resources Sep 5, 2021
@triallax
Copy link
Contributor

triallax commented Sep 8, 2021

I can confirm that this PR makes sharing text use the Android share sheet on EMUI.

@TobiGr
Copy link
Contributor

TobiGr commented Sep 8, 2021

Please add documentation to the two new classes explaining why they are needed / used

@AudricV AudricV force-pushed the custom-textview-edittext branch from b1907bc to 12934dc Compare September 16, 2021 19:46
@AudricV
Copy link
Member Author

AudricV commented Sep 16, 2021

Please add documentation to the two new classes explaining why they are needed / used

@TobiGr Done

Stypox
Stypox previously requested changes Sep 18, 2021
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! I just have a couple of questions

@AudricV AudricV force-pushed the custom-textview-edittext branch 2 times, most recently from 463d175 to 48b3051 Compare September 24, 2021 18:18
@AudricV AudricV requested a review from Stypox September 24, 2021 18:23
@AudricV AudricV force-pushed the custom-textview-edittext branch from 48b3051 to a7c1f8d Compare September 24, 2021 20:34
@AudricV AudricV force-pushed the custom-textview-edittext branch from a7c1f8d to 5495cab Compare October 1, 2021 21:41
@AudricV AudricV requested review from Stypox and litetex and removed request for Stypox October 1, 2021 21:42
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

Added some suggestions for making the code way simpler and easier to understand.
Otherwise the code looks good. Tested it on my Huawei P20 and it works as expected.

Also note that there are merge conflicts.

@AudricV AudricV force-pushed the custom-textview-edittext branch from 5495cab to 5eee81d Compare October 2, 2021 21:37
@AudricV AudricV requested a review from litetex October 2, 2021 21:37
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

@TiA4f8R
I'm sorry, but somehow GitHub seems to have lost my 5th proposed key-change here.

@AudricV AudricV removed the request for review from Stypox October 13, 2021 17:34
…the selected text

This TextView class extends the AppCompatTextView class from androidx.

These changes (only in XML ressources) allow us to share the selected text by using ShareUtils.shareText, which opens the Android system chooser instead of the Huawei system chooser on EMUI devices.
…the selected text

This EditText class extends the AppCompatEditText class from androidx.

These changes (only in XML ressources) allow us to share the selected text by using ShareUtils.shareText, which opens the Android system chooser instead of the Huawei system chooser on EMUI devices.
Use the same logic as Android TextViews
A new class has been added in the util package: NewPipeTextViewHelper.
It shares the selected text of a TextView with ShareUtils#shareText (with the created shareSelectedTextWithShareUtils static method).
Only this static method can be used by other classes, other methods are private.
@AudricV AudricV force-pushed the custom-textview-edittext branch from 5eee81d to ddaafb6 Compare October 16, 2021 13:33
@AudricV AudricV requested a review from litetex October 16, 2021 13:34
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM now

@litetex
Copy link
Member

litetex commented Oct 20, 2021

Reminder @Stypox that there is still an open change request that was fixed.

@triallax
Copy link
Contributor

@Stypox sorry, I wanted to dismiss your review, but I requested it by mistake.

@TobiGr TobiGr removed the request for review from Stypox October 23, 2021 08:27
@litetex litetex merged commit e9e2afa into TeamNewPipe:dev Oct 27, 2021
@AudricV AudricV deleted the custom-textview-edittext branch October 28, 2021 10:47
@tsiflimagas
Copy link
Contributor

Merging this PR seems to have reverted changes made by #6824 (I don't know if it reverted more stuff).

@AudricV
Copy link
Member Author

AudricV commented Nov 2, 2021

Thank you for letting me know! Looks like I did something bad when I rebased this PR...

I opened #7348 to fix this.

This was referenced Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants