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

[guillib][estuary] Make the existence of extras visible in the library #24456

Merged
merged 2 commits into from Jan 8, 2024

Conversation

CrystalP
Copy link
Contributor

@CrystalP CrystalP commented Jan 8, 2024

Description

Added a GUI information label "hasvideoextras" and made a basic Estuary change to easily identify the movies that have extras.

image

  • Repurposed the image used for extras in the info dialog, I could replace with a smaller one if provided.

  • The skin change is just a simple idea to show what could be done, and skinners will likely have better ideas to display the information, as you can have multiple versions AND extras at the same time..

@jjd-uk @Hitcher your ideas would be appreciated.

Motivation and context

It's not possible to tell quickly which movies have extras and which don't.

The video scanner identifies automatically videos in a "extras" subfolder of movies and adds them to the movie, there are ways to play back the extras, and the only piece missing for extras is the ability to tell which movies have extras.

The only way to find out at this point is to go to Info > extras just in case, or to trigger the context menu button of the extras addon. It's not ideal.

This small change is a major usability improvement and is enough as far as I am concerned for v21. No other extra-related changes planned,

How has this been tested?

movie with no extra > no overlay
movie with single version and extras > extras overlay
movie with multiple versions and extras > versions overlay

What is the effect on users?

Major usability improvement of extras, you can easily tell with a quick glance which movies have extras and warrant the extra steps to navigate and play the extras.

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

@Hitcher
Copy link
Contributor

Hitcher commented Jan 8, 2024

  • Repurposed the image used for extras in the info dialog, I could replace with a smaller one if provided.

Here are the other sizes:

https://teamkodi.slack.com/archives/C06MTEFFB/p1703842788643889?thread_ts=1703286370.875919&cid=C06MTEFFB

This raises an interesting situation now though - what to do when a movie has versions and extras.

@jjd-uk
Copy link
Member

jjd-uk commented Jan 8, 2024

This raises an interesting situation now though - what to do when a movie has versions and extras.

For views that have the area to display plot or other info then these icons probably should go somewhere there. For the poster views with no info then I'm not sure we should be cramming every icon into a poster overlay, in which case users will have to rely on the Movie info dialog.

@CrystalP
Copy link
Contributor Author

CrystalP commented Jan 8, 2024

Here are the other sizes:

Thanks, art updated.

The choose dialog with both versions and extras was a decent way to handle multiple versions and extras for the same movie, but I don't want to change course and undo the recent work of @ksooo

The info dialog is not that great for that purpose either, because the versions and extras buttons look the same, regardless of the number of versions or extras, and are located way off screen. Thankfully the Play button provides a shortcut to the version chooser.

I'd be satisfied with the PR as is and no further changes, in combination with the extras addon to give quick access to playback of extras.
As a selfish user with movie extras but only single versions I'd have all I need :-)

@CrystalP
Copy link
Contributor Author

CrystalP commented Jan 8, 2024

Jenkins build this please

Copy link
Member

@jjd-uk jjd-uk left a comment

Choose a reason for hiding this comment

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

Good enough for what we require in v21

@enen92
Copy link
Member

enen92 commented Jan 8, 2024

If possible please bring the news about the new info on the skin dev sub-forum: https://forum.kodi.tv/showthread.php?tid=372280&page=2

@jjd-uk
Copy link
Member

jjd-uk commented Jan 8, 2024

The info dialog is not that great for that purpose either, because the versions and extras buttons look the same, regardless of the number of versions or extras, and are located way off screen.

Has anyone tried if $INFO[ListItem.Property(Total)] returns the number of versions/extras in a similar manner that we use here https://github.com/xbmc/xbmc/blob/master/addons/skin.estuary/xml/Variables.xml#L293 for sets? as if that is available we can include the number of items on the button.

[edit] To answer my own question it doesn't seem that $INFO[ListItem.Property(Total)] returns anything.

addons/skin.estuary/xml/Variables.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/Variables.xml Outdated Show resolved Hide resolved
@ksooo
Copy link
Member

ksooo commented Jan 8, 2024

The choose dialog with both versions and extras was a decent way to handle multiple versions and extras for the same movie, but I don't want to change course and undo the recent work of @ksooo

Thanks. I still think it is a no-go to have something labelled "Choose version" or "Versions" (key is no "extras" in that strings) that provides access to extras. There should be dedicated UI elements with intuitive labels, making its relation to extras really very clear. Everything can be done, somebody just needs to step up to do the work.

BTW: I kept the implentation of the "dual-mode" selection dialog. To reactivate extras is a one-liner. But I will put my veto on any attempt to only do this, without adapting calling UI to make clear that not only versions can be selected here. ;-)

@CrystalP
Copy link
Contributor Author

CrystalP commented Jan 8, 2024

Changed the idents to tabs, waiting for the green to merge.

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.

LGTM

@CrystalP
Copy link
Contributor Author

CrystalP commented Jan 8, 2024

Actually one more thing.

I will lower the priority of extras among the overlays, so that playing and resumable come first. I think it's more important information about the movie. The visibility of extras is most useful after playing a movie I think - or before maybe, but not during.

The same reasoning doesn't work as well for versions and I think discussion would be required. Someone else can change that priority if they care enough.

@CrystalP CrystalP merged commit 87ef87a into xbmc:master Jan 8, 2024
2 checks passed
@CrystalP CrystalP deleted the add-hasvideoextras-label branch January 8, 2024 19:53
@CrystalP
Copy link
Contributor Author

CrystalP commented Jan 8, 2024

If possible please bring the news about the new info on the skin dev sub-forum: https://forum.kodi.tv/showthread.php?tid=372280&page=2

Added a message. I don't have the rights to edit the first post of the thread.

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.

None yet

5 participants