NewPipe icon indicating copy to clipboard operation
NewPipe copied to clipboard

Migrate about activity to Jetpack Compose

Open Isira-Seneviratne opened this issue 1 year ago • 11 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

  • Migrate the about activity to use Jetpack Compose and remove unused layout files.
  • Add a ScaffoldWithToolbar composable for future reuse in refactoring.
  • Remove the ViewPager2 library as its only use was in the about activity.
  • Use AboutLibraries to dynamically display the list of external dependencies of the project.

Before/After Screenshots/Screen Record

  • Before: Screenshot_20240714_073717_NewPipe Screenshot_20240714_073722_NewPipe Screenshot_20240714_074847_NewPipe

  • After: Screenshot_20240715_084245_NewPipe About-Compose Screenshot_20240823_140621_NewPipe Compose-all Screenshot_20240823_140636_NewPipe Compose-all

Fixes the following issue(s)

  • Fixes #

Relies on the following changes

  • #11539

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 Jul 14 '24 02:07 Isira-Seneviratne

OMG finally a JC PR where there is a visible change in the UI.

opusforlife2 avatar Jul 14 '24 21:07 opusforlife2

Dark red text for the tabs isn't quite ideal though, but I suppose the PR is still WIP. Can you please mark WIP PRs as draft next time?

Great work again with regards to doing yet another Jetpack Compose migration.

wb9688 avatar Jul 14 '24 21:07 wb9688

Now I looked a bit at the code and I think the list of libraries is definitely outdated, so that should be fixed as well.

wb9688 avatar Jul 14 '24 21:07 wb9688

Regarding outdated libs: #11202 should be implemented once this is merged.

TobiGr avatar Jul 14 '24 21:07 TobiGr

OMG finally a JC PR where there is a visible change in the UI.

The comments PR has visible changes as well.

Isira-Seneviratne avatar Jul 14 '24 23:07 Isira-Seneviratne

Dark red text for the tabs isn't quite ideal though, but I suppose the PR is still WIP. Can you please mark WIP PRs as draft next time?

Great work again with regards to doing yet another Jetpack Compose migration.

It's mostly completed, it's just that one detail remaining.

Also, we might want to review how the theme should look. This should help: https://material-foundation.github.io/material-theme-builder/

Isira-Seneviratne avatar Jul 14 '24 23:07 Isira-Seneviratne

I have noticed 2 things:

  • A scroll bar is present with weird color (in my case it's blue)
  • Maybe it's better to have a transparent android app bar?

Jean-BaptisteC avatar Aug 10 '24 06:08 Jean-BaptisteC

I have noticed 2 things:

  • A scroll bar is present with weird color (in my case it's blue)

The scrollbar is provided by a third-party library (Compose does not currently support scrollbars).

  • Maybe it's better to have a transparent android app bar?

Sounds good.

Isira-Seneviratne avatar Aug 10 '24 13:08 Isira-Seneviratne

Quality Gate Failed Quality Gate failed

Failed conditions
9.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Aug 11 '24 04:08 sonarqubecloud[bot]

Regarding outdated libs: #11202 should be implemented once this is merged.

That library includes a composable that automatically displays the external dependencies, so I used that in this PR itself.

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

Does AboutLibraries allow to provide non-Android dependencies (maybe by using a specific Android module)? It would be useful if we want to use BgUtils at some point to generate poTokens for YouTube and so list the dependency in the about section of the app.

AudricV avatar Nov 21 '24 17:11 AudricV

@AudricV yes, you can specify a custom libraries block. How did you think about this really really specific thing though? :sweat_smile: image

Stypox avatar Nov 21 '24 22:11 Stypox

Unfortunately the default UI implementation of libraries is not exactly how we want it, and it would take more time to fine-tune its colors and paddings and find workarounds (like the workaround for loading more libraries) than to just reimplement the few components. Also, when the version is too long the name of the library disappears. image

@Isira-Seneviratne please list in more detail which decisions you had to take while writing the code. For example, switching the launcher icon from adaptive mipmap to a new launcher drawable, which I will revert.

Stypox avatar Nov 24 '24 15:11 Stypox

@Isira-Seneviratne please list in more detail which decisions you had to take while writing the code. For example, switching the launcher icon from adaptive mipmap to a new launcher drawable, which I will revert.

I got a crash with the adaptive mipmap, as Compose doesn't support it (not sure if it works in newer versions).

Isira-Seneviratne avatar Nov 24 '24 15:11 Isira-Seneviratne

Yeah it's totally fine that you took a decision, just list it in the PR description next time so we don't have to realize and then guess why that change was made.

Stypox avatar Nov 24 '24 15:11 Stypox

Ok, so, I made a lot of changes (and I should have gone to sleep 5 hours ago, instead of going down various rabbit holes...):

  • I used the adaptive mipmap icon in AboutTab
  • I improved the layouts and reimplemented the AboutLicenses Library composables. Licenses show up in a bottom sheet dialog for easier full-page scrolling.
  • I wrote the information about NewPipeExtractor, nanojson and NoNonsenseFilePicker so that their licenses are shown correctly, and added a translatable description for NewPipeExtractor so people may find out it exists.
  • The GPLv3 license content wasn't included by the AboutLicenses plugin, so GPLv3 components' licenses (including NewPipe's) could not be seen. Other licenses, e.g. Apache 2.0, were included, which was strange. Then I figured out that the AboutLicenses plugin was making network requests during build time to obtain license content (though idk why it didn't fetch GPLv3), which I disabled because it would introduce reliance on a network connection for builds, and would also harm reproducibility. So in the end I had to restore the previous HTML assets containing license contents, and ditch AboutLicenses' automatic fetching of libraries.
  • The HTML parsing currently just uses AnnotatedString.fromHtml, which still has some limitations (e.g. no bullet points), but hopefully this will be solved by just updating Compose in the future
  • When there is no HTML for the license of a library, its SPDX url is opened in browser
  • I implemented a ViewModel for the LicenseTab, to load libraries and licenses from files in the background, and parse HTML in the background.
  • I fixed the top app bar colors under white theme.
  • I made small bumps to a couple of libraries.

Some TODOs for followup PRs:

  • Create some component that combines LazyColumnThemedScrollbar and LazyColumn, because they are often used together
  • I don't like ScaffoldWithToolbar much, if we want to use it throughout the app we need to also be able to pass a navigation icon and more things
  • We need to decide the toolbar colors, i.e. whether we want to follow Material guidelines (which makes the bar the same color as the background), or to make the bar always red/service color

Stypox avatar Nov 25 '24 03:11 Stypox

As #11684 intervened, it was to omitted to remove ViewPager2 library.

tsiflimagas avatar Nov 30 '24 11:11 tsiflimagas