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

Recognize timestamps and hashtags in descriptions and do some sharing fixes and improvements #5523

Merged
merged 20 commits into from
Jun 15, 2021
Merged

Conversation

AudricV
Copy link
Member

@AudricV AudricV commented Feb 1, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)

Description of the changes in your PR

This PR:

  • removes a toast which is shown when no app is able to handle the maket scheme on user's device (a toast is only shown if no browser is installed).
  • opens the popup player for timestamps in the description of contents.
  • adds title of the shared content in Android Share Sheet (only for Android 10+ users)
  • does some other improvements.
  • adds support for hashtags in descriptions, which open for each a search for the hastag in the current service of the content

TO DO: add a proper support of disposables (I can't do it myself) done

Fixes the following issue(s)

Fixes #5507, fixes #5455 (comment), fixes #5695

APK testing

On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.

Due diligence

(Sorry in advance for my English)

@AudricV AudricV mentioned this pull request Feb 1, 2021
14 tasks
@AudricV
Copy link
Member Author

AudricV commented Feb 1, 2021

@skyGtm Does the APK in the CI works as you want in #5507?

@skyGtm
Copy link

skyGtm commented Feb 1, 2021

Yes, it worked as expected.

But I found 2 new issues:

  1. Clicking timestamp shows Toast "Playing in popup mode", even if the player is already in popup mode.
    (Its ok for now, but in future surely someone will open an issue)

  2. Crashes when clicking http/https links at the description.(No such behaviour was observed in previous version of the app)
    This bug is somewhat serious, it crashed again after reopening. (Video demo below)
    "Guru Meditation" wasn't even able to capture the crash log.

Luckily, my phone is rooted and Scoop App installed which captured the crash log.
(I don't think root and Xposed hook methods are any problem
as the previous versions of NewPipe used to work without any issues
)

(Video demo and Crash log are posted below)

abc.mp4

crash 1.txt
crash 2.txt

@AudricV
Copy link
Member Author

AudricV commented Feb 1, 2021

Looks very strange, it perfectly works on my device, an Honor 9X running EMUI 10. In order to test, I build a debug APK: Debug share-improvements APK build with JDK 15.zip.
Does it works for you (may be something is wrong with your ROM)?
Edit: I just tried the APK from CI and it also works on my device.

@skyGtm
Copy link

skyGtm commented Feb 1, 2021 via email

@AudricV
Copy link
Member Author

AudricV commented Feb 1, 2021

@skyGtm No because it opens a web url so if there is a browser on your device, it will open the URL, unless it will show a message which is No app on your device can open this. F-Droid can also handle the Google Play Store URL (https://play.google.com/store/apps/details?id= + app_package_id).

@skyGtm
Copy link

skyGtm commented Feb 2, 2021

My phone has Android OS 5.1 API 22.
I also tried running in android emulator with similar configuration as my phone in my laptop.
But still same issue persists.
On clicking hyperlinks, app crashes.
On reopening it, app freezes with black screen.

Trying to run in emulator might help you to find the cause

@AudricV
Copy link
Member Author

AudricV commented Feb 2, 2021

@skyGtm Does this APK fix the crash: Share-improvements debug APK.zip?

@skyGtm
Copy link

skyGtm commented Feb 2, 2021

Yes. now it works perfectly.
Thankyou.

@triallax triallax added the GUI Issue is related to the graphical user interface label Feb 3, 2021
@AudricV AudricV mentioned this pull request Feb 3, 2021
4 tasks
@prateekmedia
Copy link

prateekmedia commented Feb 5, 2021

@TiA4f8R Can you add a option to select what player to open timestamp from Content settings, #5507 (comment)

Also in some videos description timestamp are not clickable on newpipe orignal one + this one too, but are recognized by player with timestamps menu.

@Stypox
Copy link
Member

Stypox commented Feb 5, 2021

@prateekmedia this is not the place to ask

@prateekmedia

This comment has been minimized.

@AudricV

This comment has been minimized.

@AudricV AudricV changed the title Fix toast shown when falling back to Google Play Store web url and open recognized timestamps in the popup player Fix toast shown when falling back to Google Play Store web url and open descriptions timestamps in the popup player Mar 3, 2021
@AudricV AudricV requested a review from TobiGr March 3, 2021 18:01
@AudricV
Copy link
Member Author

AudricV commented Mar 3, 2021

@TobiGr What do you think of the last feature that I tried to implement? For now I found only one bug: two toasts Playing in popup mode are shown when playing a content with a timestamp.

@AudricV AudricV added bug Issue is related to a bug feature request Issue is related to a feature in the app labels Mar 12, 2021
@AudricV
Copy link
Member Author

AudricV commented Jun 5, 2021

Thank you @Stypox for your work on my PR! You are the best!

@AudricV AudricV marked this pull request as ready for review June 5, 2021 15:12
AudricV and others added 20 commits June 11, 2021 12:11
…p player

This commit adds support of opening recognized timestamps in the popup
player instead of starting an intent which opens the YouTube website with
the video timestamp.
…ptions

This commit adds support for opening plain text timestamps by parsing the description text using a regular expression, add a click listener for each timestamp which opens the popup player at the indicated time in the timestamp.
In order to do this, playOnPopup method of the URLHandler class. Also, handleUrl method of this class has been renamed to canHandleUrl.
…tion of Open with Kodi button in the player

Add a boolean param, showToast, in ShareUtils.openIntentInApp and only show toast "No app on your device can open this" if this boolean is true.
Fix the action of play with Kodi button by applying the fix provided in #5599 (adding the flag Intent.FLAG_ACTIVITY_NEW_TASK to the intent in NavigationHelper.playWithKore method).
Do also some cleanup in viewWithFileProvider and shareFile methods of MissionAdapter class.
This commit tries to change the title of the system chooser shown, which is from Android System ("Open links with"), when no defaut browser is present, for the update notification.
Also do some fixes when sharing a file in downloads and some improvements in JavaDocs of ShareUtils class.
Rename URLHandler and KoreUtil classes to InternalUrlsHandler and KoreUtils.
Move InternalUrlsHandler, KoreUtils, TextLinkfier, ShareUtils classes to external_communication subpackage.
Remove unused param showPreviewText in shareText method of ShareUtils class.
Add initial work to be able to display an image preview of the content shared (not for downloads).
Use a better regular expression to parse timestamps in plain text descriptions.
Rename URLHandler and KoreUtil classes to InternalUrlsHandler and KoreUtils.
Move InternalUrlsHandler, KoreUtils, TextLinkfier, ShareUtils classes to external_communication subpackage.
Remove unused param showPreviewText in shareText method of ShareUtils class.
Add initial work to be able to display an image preview of the content shared (not for downloads).
Use a better regular expression to parse timestamps in plain text descriptions.
…ions

This commit adds supports for opening hashtags in plain text descriptions, using the same logic as timestamps.
Every hashtag opens a search on the current service with the text in the hashtag.
Also use a better regular expression for parsing timestamps.
…wser or sharing a content to other apps

Use an ACTION_CHOOSER intent has a negative impact for user experience, because user cannot set as default an activity for an intent
Split handleURL method, now private, into two methods:
handleUrlCommentsTimestamp and handleUrlDescriptionTimestamp. Code is
now more proper.
Fix the error due to the rebase on the dev branch of this branch
Add a shareText method in the ShareUtils class which has 3 parameters and calls
the original shareText method with an empty string for the
imagePreviewUrl param.
also use differently Markwon methods to convert plain text to markdown
@AudricV AudricV requested a review from Stypox June 11, 2021 12:16
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.

Everything looks good in my opinion :-)
Thank you for this big chunk of work!
I tested everything I could (share, open in browser, kodi, timestamps, hashtags) and could not find any issues on API 19 emulated (with no play store), API 29 TV emulated (with no browser; with play store), API 29 phone (with Kore; with Foxy Droid and Aurora Store (Foxy Droid was correctly chosen)), API 30 emulated (with no play store).

@Stypox Stypox merged commit 92910eb into TeamNewPipe:dev Jun 15, 2021
@AudricV AudricV deleted the share-improvements branch June 15, 2021 19:59
This was referenced Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
6 participants