NewPipe
NewPipe copied to clipboard
Introducing Jetpack Compose in NewPipe
What is it?
- [ ] Bugfix (user facing)
- [x] Feature (user facing)
- [ ] Codebase improvement (dev facing)
- [ ] Meta improvement to the project (dev facing)
Description of the changes in your PR
This pull request integrates Jetpack Compose into NewPipe by:
- Adding the necessary dependencies and setup.
- This is part of the NewPipe rewrite and fulfills the requirement for the planned settings page redesign.
- Introducing a Toolbar composable with theming that aligns with NewPipe's design.
Note:
- Theme colors are generated using the Material Theme builder (https://m3.material.io/styles/color/overview).
Screenshot
Fixes the following issue(s)
- Fixes #10874
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.
@AudricV, @theScrabi Let me know what you think about these changes. The colors are generated using Material theme builder and updated slightly by me to match toolbar and background colors (I did not have any design doc for NewPipe, so I have only tried to approximate by comparing with existing colors from styles.xml). Feel free to update any changes to match NewPipe design.
xD I eagerly downloaded this and then realised there's no visible difference for me to notice.
I am unable to see any changes in colors on the phone. Our colors were generated using a Material Theme Builder, too. There is a chance that Google did minor changes on the color generation over the years. It might be good to get an overview by seeing all the colors next to each other, similar to this.
I've reviewed the code and downloaded the artifact for this build to test on my phone. I'll go through my typical testing as well as use it as I normally use the app over time to ensure that I don't see any issues. I'll report back soon on either issues I find or whether it is working as expected.
@acrodemocide this PR should not change anything at all, because it only adds the library but doesn't make any change to existing code. So, as I said above, there's nothing to test. (sorry for closing, pressed wrong button)
As a first introduction of Compose in the codebase this looks good, and will be merged as the first PR to start the rewrite. Thank you!
Thanks for the review.
The search bar should be inside the toolbar, the navigation icon should be provided from the outside (since it e.g. might be the drawer instead of back), the spacing in the toolbar is a bit off. But we can fix these things later.
I had tried to get it inside the Toolbar, but it caused different layout issues inside the Toolbar. After looking it up online, this approach seems like standard for search bar. We can always revisit it in the future.
Did you use this color generator? Because the link you provided points to a documentation page.
Yes, this was the one.
Anyway, even if the colors are not perfect, it's really easy to generate different ones later, so I wouldn't worry too much. Yeah the colors look good (tested on #10876):
Sounds good, thanks
I think you're supposed to rebase, not merge, so that the commit history is clean.
I think you're supposed to rebase, not merge, so that the commit history is clean.
Exactly. I was about to say this as well.
Ah, I didn't realize that the project prefers rebasing over merging. I understand the contribution guide states "you must rebase to update," but I wasn't aware that rebasing with git rebase
would be a strict requirement over git merge
.
I'm curious, is there a specific reason for this preference?
Here's why I generally prefer merging to rebasing when pushing to remote branches:
1. Preserves History: Merging keeps the original commit history intact, showing the branching point and how changes were integrated. Rebasing, on the other hand, rewrites history, making it harder to track the original development flow.
2. Avoids Force Push Issues: With merging, you don't need to force push, which can disrupt collaboration and invalidate review comments on platforms like Github. Force pushing rewrites history on the remote branch, potentially causing confusion and making it difficult to track feedback.
I'm happy to use rebasing if it's the project's strict preference.
Can we merge this PR? Are there any blockers? If the only issue is the branch name, can we discuss resolving it now?
Merge does not preserve history well, and makes it difficult to reason about the various commits, since the code that fixes the conflicts is not in the original commits but in the merge commit.
Quality Gate passed
Issues
1 New issue
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
OH MAH GAWD IT HAS BEGUN. 🙌
OH MAH GAWD IT HAS BEGUN. 🙌
Yupp. Although it is only merged to a temporary branch as of now. It will be available in releases only when it is moved into dev branch