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

[video] Fix watched overlay image of parent movie item #24509

Merged
merged 2 commits into from Jan 16, 2024

Conversation

CrystalP
Copy link
Contributor

@CrystalP CrystalP commented Jan 14, 2024

Description

The watched/unwatched overlay icon doesn't need to be overriden to "always watched" for the parent item of versions when navigation of versions as a folder is enabled. It is already set with a reasonable value (watched status of the default version).

FYI @jurialmunkey could you confirm it fixes the issue.

I used the opportunity to refactor SetOverlayImage() calls a bit with a second commit, the API was confusing with an inverted bool condition, there was a weird +1 in the overlay index... No change in the logic or the results.

Motivation and context

Fix #24493

How has this been tested?

movie without versions watched/unwatched, movie with versions and default version watched/unwatched.

What is the effect on users?

Some skins depend on ListItem.Overlay

Screenshots (if appropriate):

The effect is not visible in standard Estuary but a change suggested in #24493 adds a label displaying the overlay image name in purple at the bottom of the screen.

before, with no versions watched:
image

after, with no versions watched, or versions watched except for the default version:
image

after, with the default version watched:
image

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

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

The fix seems to be correct.
And special thanks for this interface cleanup. It was so confusing before.

@jurialmunkey
Copy link

Thanks! Yes this is great. Working well / as expected now.

@CrystalP CrystalP merged commit 78a2ef1 into xbmc:master Jan 16, 2024
2 checks passed
@CrystalP CrystalP deleted the fix-watched-versionsfolder branch January 16, 2024 07:14
@Ch1llb0
Copy link

Ch1llb0 commented Jan 17, 2024

Just wondering as I'm adjusting my skin to the recent changes: Is the "reasonable value" for the watched status here really to take over the value of the default version? Shouldn't it behave like with sets: a median value of all versions?

@ksooo
Copy link
Member

ksooo commented Jan 17, 2024

Shouldn't it behave like with sets: a median value of all versions?

Good question. Current design is different from everything else we have in Kodi. The node represents both a folder and a movie (the parent or default version). Currently, context menu actions, art, state indicators are for the movie, not the folder. We should not start mixing data!

@Ch1llb0
Copy link

Ch1llb0 commented Jan 17, 2024

Ok, makes sense with that explanation 👍🏻

@CrystalP
Copy link
Contributor Author

This could be refined but I don't think in v21. There are some performance implications.

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

Successfully merging this pull request may close these issues.

[Video Versions] ListItem.Overlay is always set to OverlayWatched.png for when versions set to show as folder.
4 participants