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

offline caching in home page #56

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

krishnachaitanya0107
Copy link
Contributor

No description provided.

Copy link
Owner

@justdvnsh justdvnsh left a comment

Choose a reason for hiding this comment

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

Hi @krishnachaitanya0107 . I have reviewed the files. Please make the required changes

import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flow
import javax.inject.Inject

const val POPULAR_ANIME="PopularAnime"
Copy link
Owner

Choose a reason for hiding this comment

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

The tags are already made in an enum class. If you want to save them to the database, use enum classes ordinal.

enum class HOME_TYPES(value: Int) {
    POPULAR_ANIME(1),
    RECENT_RELEASE(2)
    ....// and so on. 
}

Also, to use the values we can just use HOME_TYPES.POPULAR_ANIME.ordinal or something like this.

@@ -23,6 +28,9 @@ class HomeDefaultRepo @Inject constructor(
//TODO: Add local data first. (caching purpose)
return flow {
Copy link
Owner

Choose a reason for hiding this comment

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

Here it should be fetching the animes from the local database only first and then if there is no data in cache, then we would be making the network request.

@@ -44,9 +58,14 @@ class HomeDefaultRepo @Inject constructor(
* @returns: Flow<ResultWrapper<*>>
* */
suspend fun parsePopularMovies(): Flow<ResultWrapper<*>> {
//TODO: Add local data first. (caching purpose)
return flow {
val response = remoteRepo.getPopularMovies()
Copy link
Owner

Choose a reason for hiding this comment

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

same

@@ -56,9 +75,14 @@ class HomeDefaultRepo @Inject constructor(
* @returns: Flow<ResultWrapper<*>>
* */
suspend fun parseNewSeasons(): Flow<ResultWrapper<*>> {
//TODO: Add local data first. (caching purpose)
return flow {
val response = remoteRepo.getNewSeasons()
Copy link
Owner

Choose a reason for hiding this comment

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

same

@krishnachaitanya0107
Copy link
Contributor Author

@justdvnsh i implemented the changes , please have a look and let me know if anything can be changed .

one thing i wish to mention - I had to move utils/Hometypes from app module to utils module because i was facing a weird 'circular dependency between the following tasks' issue .

@justdvnsh
Copy link
Owner

@krishnachaitanya0107 Hi, can you resolve the merge conflicts and also, can you please provide a working video of your feature. Thanks

@krishnachaitanya0107
Copy link
Contributor Author

krishnachaitanya0107 commented Oct 10, 2021

Hey , resolved the merge conflicts . About the working video , there isn't any difference that i can show in the screen recording because this feature doesn't have any ui related changes . I actually used logs to see whether data is coming from room database while testing .

@krishnachaitanya0107
Copy link
Contributor Author

@justdvnsh , please do let me know if you want any more changes in the code . I will be happy to implement them .

Copy link
Owner

@justdvnsh justdvnsh left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank your for your contributions.

@justdvnsh justdvnsh merged commit b01d80a into justdvnsh:develop Oct 12, 2021
@krishnachaitanya0107
Copy link
Contributor Author

Thank you @justdvnsh for going through my code , It's amazing to know that my contributions are helping out .

@justdvnsh justdvnsh added hacktoberfest-accepted Attached when pr is ready to merge and can be counted for hacktoberfest. ready-to-merge attached when a pr is ready to be merged labels Oct 12, 2021
@justdvnsh
Copy link
Owner

@krishnachaitanya0107 Offline cache has been added, but it has disrupted the flow of favorites. See the favorites tab. It will show you all the cached items. We don't want that. I am making a new issue, you can take that up if you want. The solution is pretty simple. You just need to create a separate database for the cache rather than the usual database.

@krishnachaitanya0107
Copy link
Contributor Author

krishnachaitanya0107 commented Oct 12, 2021

Oops , Really sorry @justdvnsh i did not mean to mess up that flow 😅 . Ok i will take up the new issue . It would be great if you could guide me a bit on this .

@krishnachaitanya0107 krishnachaitanya0107 deleted the offline_caching branch October 18, 2021 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Attached when pr is ready to merge and can be counted for hacktoberfest. ready-to-merge attached when a pr is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants