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 empty_state_view xml/view to Jetpack Compose #11463

Open
wants to merge 3 commits into
base: refactor
Choose a base branch
from

Conversation

toliuweijing
Copy link

@toliuweijing toliuweijing commented Aug 19, 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

As part of the rewrite effort, this PR migrates the empty_state_view xml and view to Jetpack Compose. It contains the following

  1. introduce EmptyStateComposable and specs used different fragments.
  2. introduce EmptyStateUtil to provide java -> kotlin interpolation methods.
  3. use a simple MutableState<EmptyStateSpec> to trigger recompositation upon ui state changes.

Before/After Screenshots/Screen Record

feature before after
bookmark-light s22 Screenshot_20240819_161005
bookmark-dark Screenshot_20240819_160210 Screenshot_20240819_160812

More screenshot is coming before PR commition.

Fixes the following issue(s)

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/large PRs with less than 750 changed lines label Aug 19, 2024
@toliuweijing toliuweijing marked this pull request as ready for review August 19, 2024 07:30
@toliuweijing
Copy link
Author

toliuweijing commented Aug 19, 2024

cc @opusforlife2 PTAL. Let me know if other code reviewers should be in the review loop as well.

Copy link

sonarcloud bot commented Aug 19, 2024

Copy link
Member

@Isira-Seneviratne Isira-Seneviratne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change set looks okay to me overall (I haven't tested it yet). That being said, I've suggested some changes.


import org.schabi.newpipe.BaseFragment;
import org.schabi.newpipe.R;
import org.schabi.newpipe.ui.emptystate.EmptyStateUtil;

public class EmptyFragment extends BaseFragment {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. This PR is to have a single purpose. Since applying this API requires firstly migrating EmptyFragment to kotlin, I can start another PR purely for the migration without mixing it into one.

@@ -27,7 +31,7 @@
public class CommentsFragment extends BaseListInfoFragment<CommentsInfoItem, CommentsInfo> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already migrated this fragment to Compose in a separate PR, so I don't think this particular change will be needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. This can wait for your PR.

@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
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/large PRs with less than 750 changed lines
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants