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

Show Studio icons (e.g. on folders, episodes or movies) #16298

Merged
merged 2 commits into from Oct 7, 2019

Conversation

dagwieers
Copy link
Contributor

@dagwieers dagwieers commented Jun 24, 2019

Description

This PR shows Studio icons in much more situations than originally (only TV-shows).

So rather than limiting the skin to show Studio icons on a tvshow only,
we would like to see the studio icons on episodes and folders too. And
even on movies.

If the ListItem has the studio infolabel set, we use it.

Motivation and Context

This fixes add-ons/plugin.video.vrt.nu#250, but also fixes it for everything else that has Studio information.

How Has This Been Tested?

In use for a few months, works as expected in various different cases (local content, internet streaming).
Works fine for:

  • Different content views (TV shows, movies and videos)
  • Different viewtypes (List, WideList, Wall, InfoWall, Banner, Poster, Fanart, Shift)
  • Different availability of infoLabels (i.e. with/without Premiered, audio/video/codec, duration)

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)
  • 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

So rather than limiting the skin to show Studio icons on a tvshow only,
we would like to see the studio icons on episodes and folders too. And
even on movies.

If the ListItem has the studio infolabel set, we use it.
@dagwieers
Copy link
Contributor Author

dagwieers commented Jun 25, 2019

This fix was previously contributed as phil65/skin.estuary#258 to the unmaintained Estuary repository.

@DaVukovic
Copy link
Member

Tested that and it generally works. But I would suggest that this might need a bit more love ;)

While it looks somewhat ok on some views it looks not so good at the "List"-view:

image

The yellow section is generally meant to show the list of movies and placing a studio icon below that list looks a bit misplaced, if you ask me. What about placing that at the green square and probable let the location of those icons depend on the view in use. Maybe the artwork needs to be made a bit smaller for the "List"-view as well.

Similar thing counts for episodes:

image

Where I would say, that both (also the date when the episode was aired) should be moved to the info-section of that view ("Wide List" in that case).

Jm2c

@ronie what are your thoughts on this?

@dagwieers
Copy link
Contributor Author

dagwieers commented Jun 25, 2019

@DaVukovic I understand your concern, but the fact that the InfoLabels fit into the right pane is just a coincidence, but if the entry in your list-view would show e.g. a release-date, it would also fall outside of that pane. That is also the reason why there is a space-gap in your view, but there isn't in mine (in our add-on).

screenshot008

IMO it's just a long small statusbar at the bottom that displays all kind of information about the selected item. Same for the Wide list.

@dagwieers
Copy link
Contributor Author

Here is an example with less information (since it wasn't played before) and it looks like this.

screenshot010

That is why I called it a coincidence, rather than a quirk.

@dagwieers
Copy link
Contributor Author

So I don't mind to improve how things are being laid out, however I would not hold off on this minor change, because the issue you are reporting already existed before and is unrelated to the fact we are now showing it on other types of media/views.

@dagwieers
Copy link
Contributor Author

I have a fix that would get rid of the empty spacing when there is no Premiered information, but I don't know if it belongs in this PR (as it may be more controversial than this PR).

So I would rather have this merged and then we can discuss how to improve the placement of these pieces of information.

@DaVukovic DaVukovic requested a review from ronie June 25, 2019 22:02
@DaVukovic
Copy link
Member

It's not up to me alone to decide that. I requested a review. Let's wait for that

@DaVukovic DaVukovic added Component: Skin Type: Improvement non-breaking change which improves existing functionality v19 Matrix labels Jun 25, 2019
@ronie ronie removed their request for review June 25, 2019 22:10
@ronie
Copy link
Member

ronie commented Jun 25, 2019

sorry, i don't have any time.

@dagwieers
Copy link
Contributor Author

So what can we do to get a decision here?

@jjd-uk
Copy link
Member

jjd-uk commented Jul 2, 2019

Perhaps @HitcherUK has an opinion on this.

@DaVukovic
Copy link
Member

DaVukovic commented Jul 3, 2019

If it would be my decision then I have to admit that I don't like it too much in the way it is shown above.

Also if I look at images like that from a mod: https://i.imgur.com/DQvD4IN.jpg

Then I have the feeling that those are completely misplaced. I would really suggest to let those icons depend on the view rather make them visible for any view because they simply don't match. I guess that those are not shown currently was done for a reason and even if I like the idea (at least somewhat) I would suggest to improve it more and probably in a more beautiful way.

Please don't take this as an offense. It's really not.

Edit:

But we could still leave that open for discussion. I guess that one won't get a backport at all and K19 is still far away.

@dagwieers
Copy link
Contributor Author

dagwieers commented Jul 3, 2019

I guess that those are not shown currently was done for a reason

Not true, as I have shown in my screenshots, additional infoLabels are all shown on the lower-right screen from right-to-left. Except for the spacing-issue (for which I have a fix as well) everything is aligned correctly.

The issue you show is not actually an issue, and exists currently also for tvshows. So if this was done for a reason, Studio icons and Premiered infoLabel should have been ommited in every content view (incl. tvshows).

You based your expectation on the screenshots/experience you have, but my experience (with more information inside our add-on, not just local content) is a lot more advanced and shows that it works as designed without any visual deficits (except that spacing problem if you have no "premiered" infoLabel).

@Hitcher
Copy link
Contributor

Hitcher commented Jul 3, 2019

I'm ok with this as long as you can fix the empty space issue as they all need to by right-aligned with no gaps imo.

@ksooo
Copy link
Member

ksooo commented Jul 3, 2019

I would like to wait for @ronie commenting on this as he is the maintainer of Estuary. Even if he currently got no time, it's quite some time left until Matrix release. ;-) We are not in a hurry.

This ensures all infoLabels are properly aligned to the right.
@dagwieers
Copy link
Contributor Author

@HitcherUK I have added that small fix.
@ksooo Will there not be a 18.3 release where this could be fixed?

@ksooo
Copy link
Member

ksooo commented Jul 4, 2019

Will there not be a 18.3 release where this could be fixed?

18.x is for bug fixes only, not for functional enhancements.

@bigretromike
Copy link
Contributor

@ksooo Isn't this like bugfix because it should be there but someone omitted addons which use same view/xml. It's nothing new, it was there but someone forgot to include addons to that view.
Definitely something to have in 18.4

@dagwieers
Copy link
Contributor Author

So what is needed to get this fairly simple (but to our users, very useful) fix added to Kodi?

@ksooo
Copy link
Member

ksooo commented Sep 7, 2019

Isn't this like bugfix

@bigretromike No, because there is nothing broken.

So what is needed to get this fairly simple (but to our users, very useful) fix added to Kodi?

@dagwieers A dev with enough insights to judge on the correctness of your fix has to approve and merge this PR. If @ronie or another skinner does not find the time, you have to be patient.

@dagwieers
Copy link
Contributor Author

@ksooo Depends on how you look at it, studio icons are broken in all almost all views. It surely is not a regression because it never worked, but whether it is broken depends on your point of view of what the expected behaviour should have been.

@ksooo
Copy link
Member

ksooo commented Sep 7, 2019

Sure sure.

@dagwieers
Copy link
Contributor Author

Another reminder to get this reviewed.

@ronie
Copy link
Member

ronie commented Oct 7, 2019

lgtm, thx!

@ronie ronie merged commit b68ca73 into xbmc:master Oct 7, 2019
@Rechi
Copy link
Member

Rechi commented Oct 8, 2019

@ronie please always set the milestone before or right after merging a PR.

@ronie
Copy link
Member

ronie commented Oct 8, 2019

shoot.. forgot about it again :-)

can someone tell me who uses these tags and what for?
might help me remember in the future...

@Rechi
Copy link
Member

Rechi commented Oct 8, 2019

You still haven't set it.

One can see a list of all PRs for a specific version.

@ronie
Copy link
Member

ronie commented Oct 8, 2019

i didn't dare touching it since i wasn't aware of it's usecase.
setting it 'right after' gave me the impression it was too late now to change it.

thanx for the explanation!

@ronie ronie added this to the Leia 18.5 milestone Oct 8, 2019
@DaveTBlake
Copy link
Member

I for one use the the milestone tags @ronie for looking at what PRs are intended to be in the next release, so setting pre-merge can be useful as a reminder of those things we want to complete before releasing. After merge it helps to be able to see quickly what PR went into what version.

As the first master milestone it is not so obviously helpful, but once we have more phases it will be so good pratice

@Rechi
Copy link
Member

Rechi commented Oct 8, 2019

This is a PR for master and v19, so Leia 18.5 milestone is wrong.

@ronie ronie modified the milestones: Leia 18.5, Matrix 19.0-alpha 1 Oct 8, 2019
@ronie
Copy link
Member

ronie commented Oct 8, 2019

meh... need more coffee

@dagwieers
Copy link
Contributor Author

Would a PR for Leia be accepted?

@Hitcher
Copy link
Contributor

Hitcher commented Oct 8, 2019

Would a PR for Leia be accepted?

It's not a bug fix so no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Skin Type: Improvement non-breaking change which improves existing functionality v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No studio icons on folders and episodes (Estuary skin)
9 participants