NewPipe
NewPipe copied to clipboard
Migrate comment fragments to Jetpack Compose
What is it?
- [ ] Bugfix (user facing)
- [x] Feature (user facing)
- [x] Codebase improvement (dev facing)
- [ ] Meta improvement to the project (dev facing)
Description of the changes in your PR
- Update the Compose dependencies to their latest versions.
- Add the Compose library for Coil to handle image loading.
- Add the LazyColumnScrollbar library to display scrollbars in Compose.
- Migrate the comments to use Jetpack Compose and replace the comment replies fragment with a bottom sheet dialog.
- Handle loading the comments and comment replies using the Paging library and a view model.
Before/After Screenshots/Screen Record
- Before:
Comments
Comment replies
- After:
Comments
Comment replies
- Recording
https://github.com/TeamNewPipe/NewPipe/assets/31027858/72923272-bda6-4eb0-b1b4-63debae9d02d
Relies on the following changes
- #11238
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.
Due diligence
- [x] I read the contribution guidelines.
Quality Gate failed
Failed conditions
C Reliability Rating on New Code (required ≥ A)
See analysis details on SonarCloud
Catch issues before they fail your Quality Gate with our IDE extension
SonarLint
Thank you for the effort. Looks overall good to me. Very good UI and UX changes that make the app feel modern 👍
The light theme looks like this:
Quick look, not a final review because I am currently lacking spare time
Fixed the issue, it was due to a recent change.
Hi, thank you so much for your work, it's terrific! Since your rewriting this part, do you think it would be a good chance to tackle #7191? Support in extractor has been added if I'm not wrong. Also, perhaps an m3 coloured scroll handle would blend in better. I would comment on a few more stuff, but they're things that were there before the migration (replies count on the bottom sheet header, copy on long press), so I guess you'll add them back eventually.
@tsiflimagas we are happy about every reviewer, so if you have any comments on the PR leave them here😉
Hi, thank you so much for your work, it's terrific! Since your rewriting this part, do you think it would be a good chance to tackle #7191? Support in extractor has been added if I'm not wrong.
As far as I can tell, it hasn't been implemented yet, seeing as that issue is still open.
Also, perhaps an m3 coloured scroll handle would blend in better.
I'm using a third-party library for the scrollbars, since first-party support is yet to be implemented.
we are happy about every reviewer, so if you have any comments on the PR leave them here😉
Thank you!
As far as I can tell, it hasn't been implemented yet, seeing as that issue is still open.
The issue is open on the app side, since it hasn't been implemented in the app. If I'm not wrong this PR has added the necessary support in the extractor.
I'm using a third-party library for the scrollbars, since first-party support is yet to be implemented.
By looking at the library's repo, it seemed to me that it offers the ability to edit scrollbars colour thus set it to an adaptive one, but maybe I got it wrong.
Also, it seems that none of the migrated fragments follows the black night theme. Perhaps it is planned to revamp app theming, or would it make sense to make these fragments follow app's theme as they it before?
we are happy about every reviewer, so if you have any comments on the PR leave them here😉
Thank you!
As far as I can tell, it hasn't been implemented yet, seeing as that issue is still open.
The issue is open on the app side, since it hasn't been implemented in the app. If I'm not wrong this PR has added the necessary support in the extractor.
Thanks, I missed that one.
I'm using a third-party library for the scrollbars, since first-party support is yet to be implemented.
By looking at the library's repo, it seemed to me that it offers the ability to edit scrollbars colour thus set it to an adaptive one, but maybe I got it wrong.
Yeah, it's possible to change the scrollbar colour. How does red (the primary colour) sound?
Also, it seems that none of the migrated fragments follows the black night theme. Perhaps it is planned to revamp app theming, or would it make sense to make these fragments follow app's theme as they it before?
Yeah, the black theme will need to be implemented.
How does red (the primary colour) sound?
It's good I guess. The app hasn't been migrated to material You anyway, so it looks fitting. But you may choose what you find better, as the author of the PR.
Quality Gate passed
Issues
7 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code