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

Use Paging. #8718

Closed
Closed

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Jul 31, 2022

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

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.

Due diligence

@TacoTheDank
Copy link
Member

This PR contains the commits from the Lifecycle PR. Could you fix this please?

Copy link
Member

@TacoTheDank TacoTheDank left a comment

Choose a reason for hiding this comment

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

I like the idea of this a lot.

Some comments:

app/build.gradle Outdated Show resolved Hide resolved

override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): SuggestionItemHolder {
TODO("Not yet implemented")
}
Copy link
Member

@TacoTheDank TacoTheDank Jul 31, 2022

Choose a reason for hiding this comment

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

TODO detected, is there something to be done here?

Edit: I guess that's why it's a draft lol.

@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_Paging branch 2 times, most recently from 1dbff2e to b98b14a Compare July 31, 2022 23:57
@Stypox
Copy link
Member

Stypox commented Aug 4, 2022

Interesting! Since I don't know much about good practices for Android design, if you could provide some insight into how you did things I would be really glad :-)

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Aug 6, 2022

It seems like making use of the Paging library will require extensive changes to the extractor as well, since it assumes that the additional page items will be of the same type as the initial ones, while the extractor makes use of separate types for additional items. Retrofit could be useful there.

A useful resource: https://developer.android.com/codelabs/android-paging

@TacoTheDank @Stypox What do you think? Will the library be useful enough for these changes?

@Stypox
Copy link
Member

Stypox commented Aug 6, 2022

I think each and every list present in NewPipe should be the same, and should hence be able to display all kinds of info items. E.g. info items that come to my mind are: video, channel, playlist, local playlist, video from history, subscription, ... So Paging should be made so that it can handle all item types we use at the same time. Does this help you?

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Aug 8, 2022

I think each and every list present in NewPipe should be the same, and should hence be able to display all kinds of info items. E.g. info items that come to my mind are: video, channel, playlist, local playlist, video from history, subscription, ... So Paging should be made so that it can handle all item types we use at the same time. Does this help you?

Yeah, definitely. I think it would be best to start with using a view model for the search fragment (I made a separate PR for that here: #8746) before switching to Paging there (PagingDataAdapter works similar to ListAdapter).

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants