-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix : make peertube link clickable #13015
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
base: dev
Are you sure you want to change the base?
Fix : make peertube link clickable #13015
Conversation
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.
Looks good to me. Thank you.
| final String helpText = getString(R.string.peertube_instance_url_help, | ||
| "<a href=\"" + instanceListUrl + "\">" + instanceListUrl + "</a>"); | ||
| binding.instanceHelpTV.setText(HtmlCompat.fromHtml(helpText, | ||
| HtmlCompat.FROM_HTML_MODE_LEGACY)); |
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.
Just one question, why legacy and not compat?
| "<a href=\"" + instanceListUrl + "\">" + instanceListUrl + "</a>"); | ||
| binding.instanceHelpTV.setText(HtmlCompat.fromHtml(helpText, | ||
| HtmlCompat.FROM_HTML_MODE_LEGACY)); | ||
| binding.instanceHelpTV.setMovementMethod(LinkMovementMethod.getInstance()); |
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.
Note that if we use this way, if you set NewPipe as default app of one of the PeerTube instances registered in the Android manifest and if the app is able to open base instance URLs in the future, they will open in NewPipe, which is not what we want.
A better approach would be to use a custom LinkMovementMethod, similar to what's done in the text linkifier app classes, which would call ShareUtils.openInBrowser.
Also, sorry if you didn't do so, did you use AI to generate this PR description, while that's forbidden? Its tone seems that's the case to me.
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.
Thanks for your review !
I'll implement the better approach in the next few days
did you use AI to generate this PR description,
I did , my apologies if it's forbidden. I just tried to save a couple of minutes on writing the description, cause the change is trivial in my opinion, so I just clicked a copilot button to generate the description automatically.
I'm gonna rewrite it soon as well 🙏
What is it?
Description of the changes in your PR
This pull request updates the PeerTube instance help text in the settings to make the instance list URL clickable and improves its display. The change enhances user experience by allowing users to directly access the instance list from the help text.
UI/UX improvements:
PeertubeInstanceListFragmentnow displays the instance list URL as a clickable HTML link, and enables link navigation usingLinkMovementMethod.android:autoLink="web"attribute fromfragment_instance_list.xmlsince the link handling is now managed programmatically.Code updates:
HtmlandLinkMovementMethodinPeertubeInstanceListFragment.javato support rich text and clickable links.Before/After Screenshots/Screen Record
Fixes the following issue(s)
i didn't find any bug related to this problem
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. You can find more info and a video demonstration on this wiki page.
you can see the result of this PR on the video above. i've tested my own apk already
Due diligence