Skip to content

Refactor EmptyStateComposable to remove modifier from EmptyStateSpec #12217

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

Merged
merged 5 commits into from
Jun 6, 2025

Conversation

SttApollo
Copy link

@SttApollo SttApollo commented Apr 28, 2025

What is it?

Description of the changes in your PR

  • Removed modifier from EmptyStateSpec to follow Compose best practices (layout should be decided by parent composable)
  • Fixed EmptyStateComposable to correctly pass the modifier to the internal composable
  • Updated Previews

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/small PRs with less than 50 changed lines label Apr 28, 2025
Copy link

@ShareASmile ShareASmile added the rewrite Issues and PRs related to rewrite label Apr 29, 2025
@github-project-automation github-project-automation bot moved this to In Progress in Rewrite Apr 29, 2025
@SttApollo
Copy link
Author

Hi @Stypox not sure if I need to tag anyone to review? :)

@ShareASmile
Copy link
Collaborator

Thank you for your contribution to NewPipe Rewrite & it is emmensly appreciated.

Regarding your query, team members are busy (mostly with their studies, exams, etc.) So it may take time.

@Isira-Seneviratne would you take a look if you have some free time!

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! Now the composable function follows the right standards. Sorry for taking some time to review 😬

Since now the .fillMaxWidth() calls are not in the spec anymore, but rather need to come from the outside, I think you need to update the constraint in some places (comments, related items and download fragment, see the images). After you have done so, please test all of the places that I tested in the screenshots here, to confirm everything is correctly aligned. Thanks!

You can search for random stuff on mediaccc to get 0 results, you can open this video to get 0 comments, and you can use this patch to temporarily make the related items fragment empty:

Patch
diff --git a/app/src/main/java/org/schabi/newpipe/ui/components/video/RelatedItems.kt b/app/src/main/java/org/schabi/newpipe/ui/components/video/RelatedItems.kt
index 7267c66b3..05f16d927 100644
--- a/app/src/main/java/org/schabi/newpipe/ui/components/video/RelatedItems.kt
+++ b/app/src/main/java/org/schabi/newpipe/ui/components/video/RelatedItems.kt
@@ -42,7 +42,7 @@ fun RelatedItems(info: StreamInfo) {
     }
 
     ItemList(
-        items = info.relatedItems,
+        items = listOf(), // info.relatedItems,
         mode = ItemViewMode.LIST,
         listHeader = {
             item {
@@ -72,7 +72,7 @@ fun RelatedItems(info: StreamInfo) {
                     }
                 }
             }
-            if (info.relatedItems.isEmpty()) {
+            if (info.relatedItems.isEmpty() || true) {
                 item {
                     EmptyStateComposable(EmptyStateSpec.NoVideos)
                 }
diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml
index 7c564f9e9..aa9839fe7 100644
--- a/gradle/libs.versions.toml
+++ b/gradle/libs.versions.toml
@@ -54,7 +54,7 @@ swiperefreshlayout = "1.1.0"
 # This works thanks to JitPack: https://jitpack.io/
 teamnewpipe-filepicker = "5.0.0"
 teamnewpipe-nanojson = "1d9e1aea9049fc9f85e68b43ba39fe7be1c1f751"
-teamnewpipe-newpipe-extractor = "67f3301d9"
+teamnewpipe-newpipe-extractor = "v0.24.6"
 webkit = "1.9.0"
 work = "2.8.1"

image
image
image

@Profpatsch Profpatsch added waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. and removed waiting for review labels May 11, 2025
@SttApollo
Copy link
Author

@Stypox thank you for taking a look! I was traveling; I will address your comments this week!

@github-actions github-actions bot removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label May 31, 2025
@github-actions github-actions bot added size/medium PRs with less than 250 changed lines and removed size/small PRs with less than 50 changed lines labels Jun 5, 2025
@SttApollo
Copy link
Author

SttApollo commented Jun 5, 2025

@Stypox done! I tested in all the screens you shared + the patch
Screenshot 2025-06-04 at 1 21 13 PM
Screenshot 2025-06-04 at 9 49 24 PM
Screenshot 2025-06-04 at 9 49 46 PM
Screenshot 2025-06-04 at 9 52 01 PM
Screenshot 2025-06-04 at 9 53 05 PM
Screenshot 2025-06-04 at 10 35 52 PM
Screenshot 2025-06-04 at 12 45 25 PM
Screenshot 2025-06-04 at 12 45 35 PM
Screenshot 2025-06-04 at 12 45 59 PM
Screenshot 2025-06-04 at 12 46 11 PM

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thanks for the effort, looks good!

Copy link

sonarqubecloud bot commented Jun 6, 2025

@Stypox Stypox merged commit f16becc into TeamNewPipe:refactor Jun 6, 2025
7 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Rewrite Jun 6, 2025
@Stypox
Copy link
Member

Stypox commented Jun 6, 2025

Jfyi a build with this included will be built tonight by https://github.com/TeamNewPipe/NewPipe-refactor-nightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rewrite Issues and PRs related to rewrite size/medium PRs with less than 250 changed lines
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants