-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Conversation
I added the icons, and changed the string to Copy URL, do I need to change anything else? |
Wait, now that I think of it, maybe this solution is better: #6435 (comment) |
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. |
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? |
@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. |
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. Edit: I noticed that some buttons are showing the wrong tooltip, like the share button and mute button showing "Bookmarked Playlists". |
There was a problem hiding this 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?
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. |
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.
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". |
Ok, I will add the tooltip, for the share button, then we can make a new PR to fix the others. |
@opusforlife2 Is the tooltip the same as the android:contentDescription? 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? |
@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. |
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? |
Well, I'm out of my depth here. ¯\_(ツ)_/¯ Only a developer can help you. |
So to add a tooltip to the share button, what do I need to do? |
There was a problem hiding this 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 :-)
What is it?
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