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

Warn of potential duplicates when adding to library #691

Merged
merged 17 commits into from
Oct 26, 2023

Conversation

Luna712
Copy link
Contributor

@Luna712 Luna712 commented Oct 14, 2023

@Luna712 Luna712 changed the title Warn possible duplicates for favorites and subscriptions Warn possible duplicates for library Oct 14, 2023
@Luna712 Luna712 changed the title Warn possible duplicates for library Warn of possible duplicates when adding to library Oct 15, 2023
@Luna712
Copy link
Contributor Author

Luna712 commented Oct 15, 2023

This might be a very messy way to do this and might be much better ways, but I'm not sure. If this is a total mess, I apologize.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 16, 2023

I updated this to be much more clean (in my opinion) by reducing duplicate code through use of better callbacks, and cleaning up messages in dialog to be more clear also.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 16, 2023

I've tested almost every possibility I can think of on both TV and Phone for Subscriptions, Favorites, and each and every bookmark option (both in viewer and homepage preview), but there is still a chance I missed something. This works pretty consistently and accurately from my observations.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 17, 2023

This now does the IMDb ID checking support as well.

@Luna712 Luna712 changed the title Warn of possible duplicates when adding to library Warn of potential duplicates when adding to library Oct 19, 2023
Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

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

Very nice 👍, however it is not very futureproof nor support any other service than imdb

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 22, 2023

Very nice 👍, however it is not very futureproof nor support any other service than imdb

Thanks for your review!
Though, this does support even if not using Imdb, it then checks if title is an exact match, could support tmdb but I though that might complicate it as imdb and tmdb would contain fairlt similar stuff so didn't see why we would need to check both.

@fire-light42
Copy link
Collaborator

Very nice 👍, however it is not very futureproof nor support any other service than imdb

Thanks for your review! Though, this does support even if not using Imdb, it then checks if title is an exact match, could support tmdb but I though that might complicate it as imdb and tmdb would contain fairlt similar stuff so didn't see why we would need to check both.

the point is not to do overlapping support like tmdb + imdb, but to have support for ANY service and in this case MAL would cover what imdb does not

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 23, 2023

Very nice 👍, however it is not very futureproof nor support any other service than imdb

Thanks for your review! Though, this does support even if not using Imdb, it then checks if title is an exact match, could support tmdb but I though that might complicate it as imdb and tmdb would contain fairlt similar stuff so didn't see why we would need to check both.

the point is not to do overlapping support like tmdb + imdb, but to have support for ANY service and in this case MAL would cover what imdb does not

I understand, the issue I ran into supporting all syncData is Imdb is in simkl as a text of both tmdb and imdb, parsing through this while supporting non-simkl values makes the code much more complicated and I couldn't really figure it out. I'll try some more though.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 23, 2023

Very nice 👍, however it is not very futureproof nor support any other service than imdb

@fire-light42

Thanks again for the review, I think I got everything now!

Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

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

🙏 This is much better, but I would still like to see mal, anilist and tmdb id check before merge

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 23, 2023

🙏 This is much better, but I would still like to see mal, anilist and tmdb id check before merge

Thanks again, done!

@Luna712 Luna712 mentioned this pull request Oct 24, 2023
3 tasks
Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

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

Minor code quality things then merge. I am somewhat extra strict here with performance as we don't want a 300ms or so delay when clicking just because we have 1000 items in our library

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 24, 2023

Minor code quality things then merge. I am somewhat extra strict here with performance as we don't want a 300ms or so delay when clicking just because we have 1000 items in our library

Thanks, and I appreciate that. It should be done again now.

@fire-light42
Copy link
Collaborator

When testing this I found 2 UX issues

  1. We just want to match on duplicates no matter the status. This might have been my confusing comment about it 3 days ago that made you change it, but just write smth like
    fun getAllBookmarkedData(): List<BookmarkedData> = getKeys("$currentAccount/$RESULT_WATCH_STATE_DATA")?.mapNotNull(::getKey) ?: emptyList()
  2. We only want to do the check when the watch status when it goes from None -> something else, we have already confirmed that we want a series bookmarked if we have on watching and move it to dropped for example.

Other than that it looks fine 👍 good job.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 25, 2023

When testing this I found 2 UX issues

  1. We just want to match on duplicates no matter the status. This might have been my confusing comment about it 3 days ago that made you change it, but just write smth like
    fun getAllBookmarkedData(): List<BookmarkedData> = getKeys("$currentAccount/$RESULT_WATCH_STATE_DATA")?.mapNotNull(::getKey) ?: emptyList()
  2. We only want to do the check when the watch status when it goes from None -> something else, we have already confirmed that we want a series bookmarked if we have on watching and move it to dropped for example.

Other than that it looks fine 👍 good job.

Got it thanks, I'll finalize that i a couple hours. Sorry about that.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 25, 2023

When testing this I found 2 UX issues

  1. We just want to match on duplicates no matter the status. This might have been my confusing comment about it 3 days ago that made you change it, but just write smth like
    fun getAllBookmarkedData(): List<BookmarkedData> = getKeys("$currentAccount/$RESULT_WATCH_STATE_DATA")?.mapNotNull(::getKey) ?: emptyList()
  2. We only want to do the check when the watch status when it goes from None -> something else, we have already confirmed that we want a series bookmarked if we have on watching and move it to dropped for example.

Other than that it looks fine 👍 good job.

@fire-light42

I think I got it done, had some issues with how NONE was done, which saved to data regardless (seems like even before it did) so counted as a duplicate but I think I fixed that, and made it remove properly, before it seems like it was removed and immediately readded, which I have no idea if that was intentional or not, but it does not do that anymore. If you want it to, I can figure something else out.

@fire-light42 fire-light42 merged commit 968bd59 into recloudstream:master Oct 26, 2023
2 checks passed
@Luna712
Copy link
Contributor Author

Luna712 commented Oct 26, 2023

Thanks again for all your reviewing and suggestions @fire-light42

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