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

Add long press action on hashtags and web links in descriptions #7725

Conversation

AudricV
Copy link
Member

@AudricV AudricV commented Jan 30, 2022

What is it?

  • Feature (user facing)

Description of the changes in your PR

This PR adds a long press action on web links and hashtags which copy them to clipboard.

To do so:

  • Some changes in our TextView class have been required and metadata items are now using a NewPipeTextView instead of a standard TextView.

  • Three new classes have been added: a custom LinkMovementMethod class (adapted from a StackOverflow answer), a custom ClickableSpan class have been added in order to set a long press event and a class to avoid code duplication in CommentTextOnTouchListener, TouchUtils.

Before/After Screenshots/Screen Record

  • Before:
Long.press.links.and.hashtags.before.mp4
  • After:
Long.press.links.and.hashtags.after.mp4

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

@AudricV AudricV added the feature request Issue is related to a feature in the app label Jan 30, 2022
@triallax triallax added the GUI Issue is related to the graphical user interface label Jan 30, 2022
@triallax triallax self-requested a review January 30, 2022 17:15
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.

Note:

  • The videos are unplayable (can't view them in Firefox or Chromium-based browsers)
  • There are some Sonar warnings

@AudricV
Copy link
Member Author

AudricV commented Jan 30, 2022

  • The videos are unplayable (can't view them in Firefox or Chromium-based browsers)

I had to compress them, unfortunately, but it seems Video Transcoder converted them in an unplayable format for browsers (you can play them in VLC I think)

@SameenAhnaf
Copy link
Collaborator

SameenAhnaf commented Jan 30, 2022

@litetex You may download the video with Video Download Helper. I eventually had to download, the video is still loading on my browser.

@TiA4f8R I included some suggestions in #7097. I think, it'll be more suitable for copying links.

@litetex

This comment has been minimized.

@AudricV
Copy link
Member Author

AudricV commented Jan 30, 2022

So what's the best way to proceed when long pressing links and hashtags?

  • copy them directly to clipboard
  • show an alert dialog to allow opening/searching them and copying them to clipboard

I am not all a UX designer so feedback is appreciated.

@opusforlife2
Copy link
Collaborator

@TiA4f8R The usual behaviour for Android apps is to take the specific action when directly tapping, and to copy to clipboard (or something else) when long-pressing.

@AudricV
Copy link
Member Author

AudricV commented Jan 30, 2022

Ok, I will do this.

Edit: done

@AudricV AudricV force-pushed the add-long-press-actions-on-hashtags-and-links-in-descriptions branch from 0c16ea7 to 770d9c8 Compare January 30, 2022 19:32
@AudricV AudricV changed the title Add long press actions on hashtags and web links in descriptions Add long press action on hashtags and web links in descriptions Jan 30, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jan 30, 2022

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 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ktprograms
Copy link
Contributor

@TiA4f8R Can you please add this functionality to timestamps in the description? (Example video with timestamps: https://www.youtube.com/watch?v=mOSirVeP5lo)

Thanks!

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.

Looks almost good to me ;-)

@AudricV
Copy link
Member Author

AudricV commented Jul 19, 2022

Note: even if it seems I forgot this pull request, I didn't forget it and I will continue to work on this, maybe once my work is done on other features/bug fixes I have on NewPipe projects.

@triallax triallax removed their request for review August 21, 2022 21:36
@AudricV AudricV force-pushed the add-long-press-actions-on-hashtags-and-links-in-descriptions branch from 770d9c8 to cb703bd Compare September 7, 2022 20:33
@AudricV AudricV marked this pull request as ready for review September 7, 2022 20:34
@AudricV
Copy link
Member Author

AudricV commented Sep 7, 2022

@TiA4f8R Can you please add this functionality to timestamps in the description? (Example video with timestamps: https://www.youtube.com/watch?v=mOSirVeP5lo)

Thanks!

@ktprograms Done (in the app side) on services on which timestamps supported (otherwise, a fallback to copy timestamp raw text is made).

@AudricV
Copy link
Member Author

AudricV commented Sep 7, 2022

Note that the Sonar bug is a false positive, as nullity is checked by Utils.isBlank method of the extractor as the first instruction of the method and that the TODO that I added, is of course, not feasible in this PR.

@AudricV AudricV requested a review from Stypox September 28, 2022 15:38
@Stypox Stypox force-pushed the add-long-press-actions-on-hashtags-and-links-in-descriptions branch from cb703bd to aaae49a Compare November 29, 2022 11:19
@sonarcloud
Copy link

sonarcloud bot commented Nov 29, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

At this point you should probably create a library for clipboard & sharing handling 😂

I tested on API 33 emulated, API 22 emulated and my API 29 phone and everything works well. Comments with links now behave correctly :-D

The few comments I left are really small things: once you solve them, you can merge this PR yourself. Code looks good. I rebased and solved conflicts, otherwise it wouldn't build, because of the missing extractor commit build on Jitpack.

AudricV and others added 2 commits January 15, 2023 11:40
…long-press

This commit adds the ability to copy to clipboard hashtags, URLs and timestamps
when long-pressing them.

Some changes in our TextView class related to text setting have been required
and metadata items are now using a NewPipeTextView instead of a standard
TextView.

Six new classes have been added:

- a custom LinkMovementMethod class;
- a custom ClickableSpan class, LongPressClickableSpan, in order to set a long
  press event;
- a class to avoid code duplication in CommentTextOnTouchListener, TouchUtils;
- three implementations of LongPressClickableSpan used when linkifying text:
  - HashtagLongPressClickableSpan for hashtags;
  - TimestampLongPressClickableSpan for timestamps;
  - UrlLongPressClickableSpan for URLs.
@Stypox Stypox force-pushed the add-long-press-actions-on-hashtags-and-links-in-descriptions branch from aaae49a to 22c201b Compare January 15, 2023 10:51
@TeamNewPipe TeamNewPipe deleted a comment from sonarcloud bot Jan 15, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jan 15, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

LGTM, thanks! The bug reported by Sonar is wrong: it says content might be null but actually we check for isBlankness above.

@Stypox Stypox merged commit 2db2918 into TeamNewPipe:dev Jan 15, 2023
@Stypox Stypox mentioned this pull request Jan 22, 2023
3 tasks
@goyalyashpal
Copy link
Contributor

goyalyashpal commented Jan 22, 2023

kinda strange that yeah, it doesn't play on desktop browser ( Vivaldi | 5.5.2805.48 (Stable channel) (64-bit) ; Chrome/106.0.0.0), but is working fine on android browser (Vivaldi 5.6.2868.29)

@goyalyashpal
Copy link
Contributor

but i still don't understand the premise of this PR.

like, selecting tags and links was available before this pr as well:

  • "Enable selecting text in description"
  • press and hold on links to select and copy them
  • the only problem was that clicking them caused them to open

This pr shows that press and holding them automatically copies them - one tap saved.

But what happens in this pr when u single tap on links/tags? Did this problem was an effort to solve this single tap problem?

Following is a screenrecord from Newpipe v0.23.3 i.e. back from 2022.08'

v0.23.3-newpipe.mp4

@AudricV AudricV deleted the add-long-press-actions-on-hashtags-and-links-in-descriptions branch January 22, 2023 22:52
@Stypox
Copy link
Member

Stypox commented Jan 23, 2023

The videos are unplayable

I don't understand what you are referring to.

Did this problem was an effort to solve this single tap problem?

What do you mean by "single tap problem"? For me single taps work normally. Also, this PR was also needed because soon we will have comments with copiable links and hashtags.

@goyalyashpal
Copy link
Contributor

I don't understand what you are referring to.

sorry, have updated with link. : #7725 (review)

single tap problem"? For me single taps work normall

this is shown in the screen recording - i meant that on single tapping, the links were opening. i was asking that was that considered the problem?

Also, this PR was also needed because soon we will have comments with copiable links and hashtags.

yeah, that is understandable. the thing that confused me was the following quoted part: "reopening as we are not able to copy full links" which we definitely were.

Reopening as we are not able to copy full links on YouTube. I will open a PR which will add this ability soon.
- @ AudricV at #3253 (comment)

@Stypox
Copy link
Member

Stypox commented Jan 28, 2023

which we definitely were

Oh ok, I see what you meant. Yeah, in the description you were already able to copy YouTube links, since those are always shown in full. I am not sure about Peertube though, where Markdown is employed, and [link](https://example.org) gets transformed into the literal "link". Unless Android is able to copy rich text, I doubt you would be able to copy links from there. So the YouTube description was just a special case among other services' descriptions and also comments.

@goyalyashpal
Copy link
Contributor

about Peertube though, where Markdown is employed,
Unless Android is able to copy rich text,

ohw ohkayh, yeah, makes sense then 👍

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 GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links in description can't be copied
8 participants