NewPipe icon indicating copy to clipboard operation
NewPipe copied to clipboard

Migrate comment fragments to Jetpack Compose

Open Isira-Seneviratne opened this issue 9 months ago • 8 comments

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:

Screenshot_20240621-161715 Comments

Screenshot_20240621-183000 Comment replies

  • After:

Screenshot_20240621-161728 Comments

Screenshot_20240623-110556 Screenshot_20240623-110604 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

Isira-Seneviratne avatar May 12 '24 02:05 Isira-Seneviratne

Quality Gate Failed 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

sonarqubecloud[bot] avatar May 12 '24 12:05 sonarqubecloud[bot]

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.

Isira-Seneviratne avatar Aug 28 '24 12:08 Isira-Seneviratne

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 avatar Aug 28 '24 21:08 tsiflimagas

@tsiflimagas we are happy about every reviewer, so if you have any comments on the PR leave them here😉

TobiGr avatar Aug 28 '24 22:08 TobiGr

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.

Isira-Seneviratne avatar Aug 29 '24 01:08 Isira-Seneviratne

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?

tsiflimagas avatar Aug 29 '24 22:08 tsiflimagas

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.

Isira-Seneviratne avatar Aug 29 '24 23:08 Isira-Seneviratne

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.

tsiflimagas avatar Aug 29 '24 23:08 tsiflimagas