Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate about activity to Jetpack Compose #11282

Merged
merged 28 commits into from
Nov 27, 2024

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Jul 14, 2024

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • 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.
  • Use a vector drawable instead of the adaptive icon for the app, as Compose doesn't support loading adaptive drawables.

Before/After Screenshots/Screen Record

Before
After

Fixes the following issue(s)

  • Fixes #

Relies on the following changes

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

@github-actions github-actions bot added the size/giant PRs with more than 750 changed lines label Jul 14, 2024
@opusforlife2
Copy link
Collaborator

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

@wb9688
Copy link
Contributor

wb9688 commented Jul 14, 2024

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 wb9688 marked this pull request as draft July 14, 2024 21:08
@wb9688
Copy link
Contributor

wb9688 commented Jul 14, 2024

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.

@TobiGr
Copy link
Contributor

TobiGr commented Jul 14, 2024

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

@Isira-Seneviratne
Copy link
Member Author

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

The comments PR has visible changes as well.

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Jul 14, 2024

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 Isira-Seneviratne marked this pull request as ready for review July 15, 2024 03:16
@Jean-BaptisteC
Copy link

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?

@Isira-Seneviratne
Copy link
Member Author

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.

Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@Isira-Seneviratne
Copy link
Member Author

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.

# Conflicts:
#	app/build.gradle
#	app/src/main/java/org/schabi/newpipe/ktx/Bundle.kt
#	build.gradle
@TobiGr TobiGr added the GUI Issue is related to the graphical user interface label Sep 2, 2024
@ShareASmile ShareASmile added the rewrite Issues and PRs related to rewrite label Oct 5, 2024
@AudricV
Copy link
Member

AudricV commented Nov 21, 2024

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.

@Stypox
Copy link
Member

Stypox commented Nov 21, 2024

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

@Stypox
Copy link
Member

Stypox commented Nov 24, 2024

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.

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Nov 24, 2024

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

@Stypox
Copy link
Member

Stypox commented Nov 24, 2024

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
Copy link
Member

Stypox commented Nov 25, 2024

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 Stypox merged commit c0c08a4 into TeamNewPipe:refactor Nov 27, 2024
6 checks passed
@Isira-Seneviratne Isira-Seneviratne deleted the About-Compose branch November 28, 2024 00:33
@tsiflimagas
Copy link
Contributor

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

@tsiflimagas tsiflimagas mentioned this pull request Dec 1, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issue is related to the graphical user interface rewrite Issues and PRs related to rewrite size/giant PRs with more than 750 changed lines
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants