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

Update NewPipeExtractor and properly linkify comments #9631

Merged
merged 2 commits into from
Jan 28, 2023

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Jan 5, 2023

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

  • Update the extractor to the latest commit, with breaking changes from Use Description object for comments text. NewPipeExtractor#987: now comments have a Description and not a String anymore
  • Changes in TextLinkifier:
    • Move the method that determines how to linkify a description based on its type from DescriptionFragment to TextLinkifier
    • Rename methods in TextLinkifier to not contain the redundant "link" word already contained in "Linkifier"
    • Allow passing a callback to TextLinkifier methods to be notified when the text has been set to the TextView, needed to set ellipsis and movement method in comments
    • Since TextLinkifier only needs stream url and service id to generate popup-timestamps and hashtags, and since comments only have that information and not a full Info, now the TextLinkifier methods take those two parameters instead of an `Info.
  • Rework the comments touch handling because it wouldn't work anymore since the links are now linkified with a different approach than before (I don't know why it stopped working)
    • Now calling getText() does not return a Spannable, but rather a Spanned (even though in TextLinkifier a SpannableBuilder is used). This means that the text selection (i.e. the highlight that appears when tapping on links) can't be manually set by us as done before in CommentTextOnTouchListener, unfortunately.
    • Note that long clicks still don't work, I'll leave it to a future PR. The same logic as LongPressLinkMovementMethod needs to be implemented in CommentTextOnTouchListener.
  • Rework comment ellipsis handling, since it added ... by cutting away too many characters in case of links, and also because it made the text view size smaller on some cases for some reason (?)
    • Now links are not clickable when the comment is ellipsized (this should have been the case before, too, but it used to be buggy)
    • Only the last 2 characters are chopped off, and are only chopped off if there would not already be space for the ellipsis

Fixes the following issue(s)

It definitely fixes some problems with comments, but I couldn't find any related issue.

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

@Stypox Stypox force-pushed the update-npe branch 2 times, most recently from 393499f to 29fca1a Compare January 5, 2023 18:08
@Stypox Stypox force-pushed the update-npe branch 3 times, most recently from 8325bc0 to 608c2fa Compare January 15, 2023 13:58
@sonarcloud
Copy link

sonarcloud bot commented Jan 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@SameenAhnaf SameenAhnaf added the codequality Improvements to the codebase to improve the code quality label Jan 20, 2023
TobiGr
TobiGr previously approved these changes Jan 25, 2023
Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Code looks good. I tested briefly and did not find any bugs related to the changed linking.

There are a few bugs related to other extractor PRs which are introduced to NP in this PR, but are fixed separately. The extractor version can be updated independently.

@Stypox Stypox changed the base branch from dev to release-0.25.0 January 28, 2023 21:40
@Stypox Stypox dismissed TobiGr’s stale review January 28, 2023 21:40

The base branch was changed.

@Stypox Stypox merged commit cd12503 into release-0.25.0 Jan 28, 2023
@Stypox Stypox mentioned this pull request Jan 28, 2023
3 tasks
@Stypox
Copy link
Member Author

Stypox commented Jan 28, 2023

I'm merging this now so that we can have it in the RC

@TobiGr TobiGr deleted the update-npe branch January 28, 2023 22:53
@TobiGr
Copy link
Member

TobiGr commented Jan 28, 2023

we should also update the extractor commit. otherwise, there will be reports about broken comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants