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 copy url function to share button #6436

Merged
merged 2 commits into from
Jun 8, 2021

Conversation

Abanoub8
Copy link

@Abanoub8 Abanoub8 commented Jun 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

This pull request adds a button beside the share button and open in browser button, that copies the URL of the video to clipboard. This is very useful in many situations.

Fixes the following issue(s)

Relies on the following changes

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

app/src/main/res/layout/player.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@Abanoub8
Copy link
Author

Abanoub8 commented Jun 6, 2021

I added the icons, and changed the string to Copy URL, do I need to change anything else?

@Stypox
Copy link
Member

Stypox commented Jun 7, 2021

Wait, now that I think of it, maybe this solution is better: #6435 (comment)
Adding many buttons is not a good idea; just let the share button be long-pressed. Also, add this long-press action to the button in video details, too. Sorry for not telling you this before.

@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

To be honest I was thinking about doing that do, but I thought that something basic like this would have its own button. Anyways, I would have to figure out how to do this now.

@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

I found out that you already had an OnLongClickListener, so it was only a matter of rewiring, and now it is done. I can do one thing more, which is remove the ic_content_copy.xml file if you don't want it?

@opusforlife2
Copy link
Collaborator

@Abanoub8 Please update your PR title and description accordingly, and link it to your issue so it gets closed automatically upon merging this.

Also, please add a tooltip like the other long-press buttons.

In fact, @Stypox shouldn't there also be a tooltip for the download button? Long-pressing it opens Downloads, which is almost like an easter egg every user has to discover on their own or read about somewhere.

@Abanoub8 Abanoub8 changed the title Add url button Add copy url function to share button Jun 7, 2021
@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

I already changed the title, and for some reason I am unable to link the issue, so I can close it manually, or delete it.
As for the tooltip, I will figure out how to do that, and I can also do it for the download button if you want.

Edit: I noticed that some buttons are showing the wrong tooltip, like the share button and mute button showing "Bookmarked Playlists".
Also how should the share button have two tooltips? for the short and long press.

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!

Edit: I noticed that some buttons are showing the wrong tooltip, like the share button and mute button showing "Bookmarked Playlists".
Also how should the share button have two tooltips? for the short and long press.

If it requires these many changes, maybe let's keep this PR just for the player and do that in another PR. What do you think?

app/src/main/java/org/schabi/newpipe/player/Player.java Outdated Show resolved Hide resolved
app/src/main/res/drawable-night/ic_content_copy.xml Outdated Show resolved Hide resolved
@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

I fixed the problems with the code, and the unneeded icons, as for the tooltips, I would say maybe starting a new PR would be better.

@opusforlife2
Copy link
Collaborator

Agreed on the separate PR for the other tooltips. They aren't related to your feature or code, anyway. If you're willing to make a PR for all the tooltips, that's fine. But if not, then I think you should add at least the Share button tooltip in this PR.

Also how should the share button have two tooltips? for the short and long press.

There is only one type of tooltip. There is a toggle for this in the Appearance menu that is on by default. When you tap on the Play in Background or Popup buttons, it shows a tooltip telling you you can long-press the same buttons to enqueue instead.

So what I'm getting at is that tapping on the Share button should show a tooltip saying something like "You can long-press to copy the URL directly".

@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

Ok, I will add the tooltip, for the share button, then we can make a new PR to fix the others.

@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

@opusforlife2 Is the tooltip the same as the android:contentDescription?
and can I set the tooltip using android:tooltipText="for bar" in the xml?

I would be happy to work on the tooltip for other stuff, but I need to know how do you set it.

Edit: I don't mean if I can set it technically set the tooltiop this way, because I can. I mean do you organize the all the tooltips in a file or something?

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Jun 7, 2021

@Abanoub8 No idea about the code. You could follow the trail from the "Show 'Hold to append' tip" toggle in the Appearance menu? Maybe that helps?

Edit: That's the only toggle in the app which has anything to do with tooltips.

Edit 2: Wait, there are tooltips for the main page tabs as well. But they have no toggle to turn them off.

@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

I can add tooltips in the xml like so: android:tooltipText="foo bar", but then you would need to add it in all layouts and views, and need to edit them one by one in the future if you need so.

Or maybe we can make a new strings file specially for the tooltips, and just reference them in the xml.

And I don't understand what the android:contentDescription="@string/foo bar" is for?

@opusforlife2
Copy link
Collaborator

Well, I'm out of my depth here. ¯\_(ツ)_/¯ Only a developer can help you.

@Abanoub8
Copy link
Author

Abanoub8 commented Jun 7, 2021

So to add a tooltip to the share button, what do I need to do?

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
Stypox
Stypox previously approved these changes Jun 8, 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.

I did that last thing myself. Thank you for the contribution :-)

@Stypox Stypox merged commit 0b64382 into TeamNewPipe:dev Jun 8, 2021
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants