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

Fixes hashtags that use non-English characters. #7556

Merged
merged 2 commits into from
Dec 23, 2021
Merged

Fixes hashtags that use non-English characters. #7556

merged 2 commits into from
Dec 23, 2021

Conversation

Jaspann
Copy link
Contributor

@Jaspann Jaspann commented Dec 20, 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

Fixes hashtags that use non-English characters. Hashtags would now include any character above Unicode value U+007F . This solves problems of hashtags with accented characters as well as hashtags in other languages.

Before/After Screenshots/Screen Record

  • Before:
    143439151-6fceee5a-3b7b-4e90-ac75-5955ace623b1 Screenshot_20211219-205219

  • After:
    fixedHashtags fixedHashtagsCh

Fixes the following issue(s)

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

@XiangRongLin XiangRongLin added the bug Issue is related to a bug label Dec 20, 2021
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.

Do we have tests for this regex? If not, we should create a few.

@XiangRongLin
Copy link
Collaborator

Do we have tests for this regex? If not, we should create a few.

@TobiGr I thought about it at first too because we don't currently have them and this seems easily testable. But the code combines recognizing the hashtags (or links) and applying the onClickListeners to the UI making it very hard to add tests.

I would not put the burden on a first timer


The failed pipeline is not related to this change

@TobiGr
Copy link
Member

TobiGr commented Dec 20, 2021

@XiangRongLin I thought more of basic unit tests to ensure the regex is working as expected. Not more. But if you think this should be done in a separate PR, that's not a problem at all.
@Jaspann It's up to you to decide whether you want to create unit tests or not :P

@Jaspann
Copy link
Contributor Author

Jaspann commented Dec 20, 2021

@TobiGr I don't know really anything about unit testing so I would prefer not to.

@Jaspann
Copy link
Contributor Author

Jaspann commented Dec 20, 2021

@XiangRongLin I'm sorry, I found some problems with this solution and want to commit a solution that is much more accurate to how hashtags work. Should I commit here or close this PR and open a new one? Once again I am sorry, as this is my first time committing to a public repo and I don't really know how it works.

@AudricV
Copy link
Member

AudricV commented Dec 20, 2021

Should I commit here or close this PR and open a new one?

No, commit here and force-push if needed. If needed, we will squash your commits if we merge this PR.

I found some problems with this solution and want to commit a solution that is much more accurate to how hashtags work

According to your comment, I am converting this PR to draft.

@AudricV AudricV marked this pull request as draft December 20, 2021 18:26
@Jaspann Jaspann marked this pull request as ready for review December 20, 2021 19:27
Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

@XiangRongLin XiangRongLin merged commit 05370db into TeamNewPipe:dev Dec 23, 2021
This was referenced Feb 11, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hashtags with accented characters not properly recognised
4 participants