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

Watched indicator fixes #2998

Closed
wants to merge 10 commits into from

Conversation

DoggoOfSpeed
Copy link
Contributor

@DoggoOfSpeed DoggoOfSpeed commented Aug 25, 2023

Changes
Move indicator logic away from setItem() to setUnwatchedCount() so now there is no need for magic numbers like -1 and everything is generally simpler;
Implement the HIDE_UNWATCHED setting;
Also fixes the EPISODES_ONLY setting;
Fixes checkmark not appearing (broken in the .16 betas);
Make the indicator hidden by default. Currently I only enabled it on TV shows, but perhaps I forgot about some other places where they're useful;

@DoggoOfSpeed DoggoOfSpeed changed the title Watched indicator refactor Watched indicator fixes Sep 19, 2023
@nielsvanvelzen
Copy link
Member

image

Doesn't seem to work right 😄. This is for an item in the "recently added" home section, it shows me that I've watched everything and still have 15 to watch at the same time. I think this is because the card views are re-used while scrolling, so the 15 and checkmark came from another item.

@DoggoOfSpeed
Copy link
Contributor Author

Uh oh 😭 It's another case of "Works on my machine".

Have you pulled all the commits? I just rebased and that should fix your issue as this PR depends on one of my previous PRs which has been merged already.

@nielsvanvelzen
Copy link
Member

Yes I pulled it after your last push.

@DoggoOfSpeed
Copy link
Contributor Author

Hmm... That's interesting. I guess it's some gitfuckery then as that probably happens due to the code in CardPresenter incorrectly dealing with the watched symbol being on by default when after #2979 it should be off (which is what this PR was made in mind with)

@nielsvanvelzen
Copy link
Member

I think it is actually because you removed the .setVisibility(INVISIBLE); statements

@DoggoOfSpeed
Copy link
Contributor Author

Yes, that's correct. As I said, I made the indicators invisible by default, so turning them invisible a second time is unnecessary.

@nielsvanvelzen
Copy link
Member

Once again, the default in the XML will not be magically set when the view is reused in the adapter. You need to make sure that all changes made in the Java code are reverted when the items change.

@DoggoOfSpeed
Copy link
Contributor Author

Welp, I can only hope that fixes that as I'm unable to replicate the issue :(.

Still don't understand why that's happening to you as the default in the XML is set to GONE, but if it works that's good.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Sep 20, 2023
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 2, 2023
@nielsvanvelzen
Copy link
Member

Closing due to inactivity and unsolved review comments.

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