NewPipe icon indicating copy to clipboard operation
NewPipe copied to clipboard

Introducing Jetpack Compose in NewPipe

Open snaik20 opened this issue 11 months ago • 11 comments

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

NewPipe_Compose_Toolbar_Preview

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

snaik20 avatar Mar 09 '24 21:03 snaik20

@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.

snaik20 avatar Mar 09 '24 22:03 snaik20

xD I eagerly downloaded this and then realised there's no visible difference for me to notice.

opusforlife2 avatar Mar 26 '24 15:03 opusforlife2

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.

TobiGr avatar Mar 26 '24 21:03 TobiGr

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 avatar Mar 31 '24 17:03 acrodemocide

@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)

Stypox avatar Mar 31 '24 18:03 Stypox

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

snaik20 avatar Mar 31 '24 19:03 snaik20

I think you're supposed to rebase, not merge, so that the commit history is clean.

opusforlife2 avatar Apr 24 '24 19:04 opusforlife2

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.

wb9688 avatar Apr 24 '24 19:04 wb9688

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?

snaik20 avatar Apr 24 '24 19:04 snaik20

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.

Stypox avatar Apr 25 '24 06:04 Stypox

OH MAH GAWD IT HAS BEGUN. 🙌

opusforlife2 avatar May 15 '24 04:05 opusforlife2

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

snaik20 avatar May 16 '24 22:05 snaik20