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

added: hide thumbs for unwatched episodes option to show plot is off #7462

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@un1versal
Copy link
Contributor

un1versal commented Jul 9, 2015

And am now slipping into a paint proof suit.

Thank you to mkortstiege for the code assist and change from the original in #6919

Edit: Removed the text so as not to annoy the spiff

@zag2me

This comment has been minimized.

Copy link
Contributor

zag2me commented Jul 9, 2015

Nice feature, thanks.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Jul 9, 2015

The new thumb image works well, but there's one thing I can't work out - why is there no fanart for the unwatched episodes? It was the same with #6919 - you'd only see fanart for the watched episodes, and the skin background (ie. bubbles, with Confluence) for the unwatched episodes.

This isn't how it works without this patch - when hiding the plot, although you see the episode thumb you also see the fanart.

Is there any way to see the fanart, and hide only the thumb?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jul 9, 2015

Perhaps the set thumb is overwriting all other artwork and the skin uses the fallback

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Jul 10, 2015

From a brief look this is happening because of the early return.

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 10, 2015

From a brief look this is happening because of the early return.

If you have a suggestion how to address it let me know, Ill be in IRC in a bit, if you have a couple of minutes.

@un1versal un1versal force-pushed the un1versal:hidethumbs branch from b9a1492 to b584ade Jul 10, 2015

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 10, 2015

@mkortstiege that works a treat, ive replaced the code with yours, added you as author and here it is the proof: Thank you for the assist much cleaner as well.

screenshot004

screenshot005

screenshot006

@MilhouseVH this works a treat and no more issues with fanart.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jul 10, 2015

can you adjust the commit message as that part between brackets is non relevant

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 10, 2015

Done, want a backport?

@un1versal un1versal force-pushed the un1versal:hidethumbs branch from b584ade to 7964a76 Jul 10, 2015

@un1versal un1versal changed the title [backport-resurrect] added: also hide thumbs for unwatched episodes option to show plot is off added: hide thumbs for unwatched episodes option to show plot is off Jul 10, 2015

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jul 10, 2015

no backport

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Jul 10, 2015

I guess this still needs adjusting the setting label etc, as pointed out by @Montellese in the previous PR.

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 10, 2015

I guess this still needs adjusting the setting label etc, as pointed out by @Montellese in the previous PR.

The help string needs adjusting also, I haven't thought about a label text rewording yet, suggestions?

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 10, 2015

Ive updated and added the strings changes in un1versal@5b8c3c4 open to suggestions on wording of the label I think the rest is OK.

Label is: Show plot for unwatched items (Hides plot and thumb when disabled)
Plot message is: * Plot and thumbnail hidden to prevent spoilers *
Description is: [Enabled] Show plot information for unwatched media in the video library.[Disabled] Hides plot and thumbnail to prevent spoilers.

@un1versal un1versal force-pushed the un1versal:hidethumbs branch from 5b8c3c4 to 18e61a0 Jul 10, 2015

@@ -10976,11 +10976,11 @@ msgstr ""

#: system/settings/settings.xml
msgctxt "#20369"
msgid "Show plot for unwatched items"
msgid "Show plot for unwatched items (Hides plot and thumb when disabled)"

This comment has been minimized.

@phil65

phil65 Jul 10, 2015

Member

This is weird. Why not just "Show plot and thumbnail for unwatched items"?

This comment has been minimized.

@un1versal

un1versal Jul 10, 2015

Author Contributor

Actually this label also applies for movies plots and episodes plots,,,,

This comment has been minimized.

@phil65

phil65 Jul 10, 2015

Member

Both hiding thumb and hiding plot applies to movies, too? If only hiding plot applies to movies then I think this setting is really inconsistent.
Since I dont really care about this feature I will let other people decide though.

This comment has been minimized.

@da-anda

da-anda Jul 10, 2015

Member

keep it short as it's likely to scroll if it's that long, so phil's version is IMO better

This comment has been minimized.

@MilhouseVH

MilhouseVH Jul 10, 2015

Contributor

Actually this label also applies for movies

"Show plot and episode thumbnail for unwatched items"?

In the description clarify that plot applies to both movies and episodes.

msgstr ""

msgctxt "#20370"
msgid "* Hidden to prevent spoilers *"
msgid "* Plot and thumbnail hidden to prevent spoilers *"

This comment has been minimized.

@phil65

phil65 Jul 10, 2015

Member

this doesnt need change IMO. It´s the plot replacement text, right?

This comment has been minimized.

@un1versal

un1versal Jul 10, 2015

Author Contributor

Yes, but why not? this way people not wondering about the thumbnail looking like that.

This comment has been minimized.

@phil65

phil65 Jul 10, 2015

Member

Because there could be cases where the viewtype doesnt contain a thumb at all. the label would be misleading then.

This comment has been minimized.

@phil65

phil65 Jul 10, 2015

Member

.... also the user has to change the setting before so they shouldnt wonder at all.

#: system/settings/settings.xml
msgctxt "#36141"
msgid "Show plot information for unwatched media in the video library."
msgid "[Enabled] Show plot information for unwatched media in the video library.[CR][Disabled] Hides plot and thumbnail to prevent spoilers."

This comment has been minimized.

@phil65

phil65 Jul 10, 2015

Member

Is "media" correct? Doesnt this just deal with episodes?

This comment has been minimized.

@un1versal

un1versal Jul 10, 2015

Author Contributor

Actually no this is for movies and episodes.

So movies only plot is hidden/shown Episodes plot/thumb is hidden or shown.

Back to the drawing board.

This comment has been minimized.

@da-anda

da-anda Jul 10, 2015

Member

doesn't it also apply for movies? I'm actually not sure.

@notspiff

This comment has been minimized.

Copy link
Contributor

notspiff commented Jul 10, 2015

Please stop referencing me unless i need to be involved. I dont care about credits, and i do find all the noise annoying.

@un1versal un1versal force-pushed the un1versal:hidethumbs branch 2 times, most recently from 1ea0e88 to 88b59d9 Jul 10, 2015

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 10, 2015

@mkortstiege testing this further as requested.

Setting watched/unwatched the thumbnail for episodes does indeed change to reflect the state.
When you finish watching something the hidden thumbnail goes away and shows the original episode thumbnail. will go away only you start watching something else.... for some reason not refreshing straight away like other tests.

Pending minor strings tweaks, Ide say this is good complete feature though some may not care (I dont use it since plot for movies is also hidden and tbh dont think that is a spoiler really.

Side case: Web interface like Chorus2 shows no thumb at all (not the DefaultHidden thumb), when you mark watched you will get a thumbnail as expected, when marking unwatched after a couple refreshes it goes back to no thumb at all.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Jul 10, 2015

Tested latest updates and it's working really well now - not necessarily something I'll use myself, but still a nice feature.

@NedScott

This comment has been minimized.

Copy link
Contributor

NedScott commented Jul 10, 2015

Nice work, uNiversal! Thank you for cleaning this up.

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 10, 2015

Nice work, uNiversal! Thank you for cleaning this up.

yw, Code is courtesy of mkortstiege, but seeing your comment on the other one reminded me this was someone elses feature request on forums. Not something I would use myself but seems a shame to let it goto waste.

@@ -10976,7 +10976,7 @@ msgstr ""

#: system/settings/settings.xml
msgctxt "#20369"
msgid "Show plot for unwatched items"
msgid "Show plot and episode thumbnail for unwatched items"

This comment has been minimized.

@MartijnKaijser

MartijnKaijser Jul 11, 2015

Member

This change makes it only seem like it's for episodes and not for movies like the help string suggests. Also it's way too long.

This comment has been minimized.

@MilhouseVH

MilhouseVH Jul 11, 2015

Contributor

Then what about leaving "Show plot for unwatched items" as it is, and introduce a new option, "Show thumbnail for unwatched episodes" - this would allow increased flexibility, hiding of the plot but not necessarily the thumbnail, as I'm pretty sure some clever dick will want to be able to do just that as soon as this is merged...

This comment has been minimized.

@MartijnKaijser

MartijnKaijser Jul 11, 2015

Member

one option is more than enough

This comment has been minimized.

@HitcherUK

HitcherUK Jul 11, 2015

Contributor

This should be separate options because of the need to see the plot for movies but not see spoiler revealing thumbs for episodes. Just my 2 cents.

This comment has been minimized.

@un1versal

un1versal Jul 11, 2015

Author Contributor

Everyone probably knows this but Ill put it on black and white none-the-less
One option should be enough, I agree (Enabled does A / Disabled does B) its just the exact wording that's neither too long and is fairly clear which is cumbersome to achieve. Which may actually prevent it from happening in one option.

I will wait and see what is suggested that's not too long (not possible within reason) and does this job. Either two options or the length requirement has to be slightly flexible while not going overboard.

I cant add another option, My skills there are limited so Im open to all suggestions and help on that front.

Ill Keep thinking of Label wording but it will definitely have to be longer than it is now, I challenge anyone to achieve the goal with less words.

@un1versal un1versal force-pushed the un1versal:hidethumbs branch from 88b59d9 to 6405c10 Jul 11, 2015

@un1versal un1versal force-pushed the un1versal:hidethumbs branch from c240d90 to 2ffd8d3 Jul 14, 2015

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 14, 2015

@MartijnKaijser and others please review the label name, this is the absolute best and only thing I could come up with that is short and covers the topic as a label name.

@mkortstiege @Milhouse
I found however a huge huge problem with this PR probably a blocker and am trying to test it further under other circumstances.

With this option active I ran python ./texturecache.py C after the scaling algorithm merge for testing how that would go, so after it finished I was browsing around and found some episodes thumbs marked as watched with the DefaultEpisodeHidden.png very much taking over many entries.
Now already starting to panic slightly Enabled show plot and much to my surprised the thumbnail stayed as DefaultEpisodeHidden.png*

Not all episodes have it but certainly more than enough to make me wonder @MilhouseVH you could perhaps test? (I suspect they were added to library with this option to hide spoilers enabled)

uNiversaI uNiversaI
[strings] adjust labels/description to reflect the feature.
Show / Hide plot for movies and plot and thumbnail for episodes.

@un1versal un1versal force-pushed the un1versal:hidethumbs branch from 2ffd8d3 to 1d85c34 Jul 14, 2015

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Jul 14, 2015

With this option active I ran python ./texturecache.py C after the scaling algorithm merge for testing how that would go

Yes, I can confirm that when querying the media library with VideoLibrary.GetEpisodes and requesting art for an unwatched episode, when the user has requested not to show thumbs for unwatched items the art returned is "thumb":"image://DefaultEpisodeHidden.png/" rather than the real art...

Apart from caching, this leads on to what should happen in a smartphone app - should the real art be shown when this option is active, or should the app show DefaultEpisodeHidden.png?

At the moment the url path for DefaultEpisodeHidden.png isn't valid and so can't be downloaded from the webserver, meaning smartphone apps will just produce errors (as does texturecache.py).

If there's a way to discriminate between a GUI attempt to display the artwork, and a JSON request, then maybe DefaultEpisodeHidden.png should only be returned to the former, with the real artwork always returned to the latter irrespective of any plot setting - it should be up to the caller (ie. smartphone app) to decide whether to show unwatched thumbs to the user.

As for the label wording, that seems fine to me.

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 14, 2015

I think the if the option is active then any remote apps should show the hidden thumb no they should honour the kodi settings).

Now... I can confirm that the only affected episodes that get to cache the DefaultEpisodeHidden.png when they get added to library with this option enabled, in any case caching DefaultEpisodeHidden.png is 100% the wrong thing todo since only way to get the real thuumb back is to remove/rescan or refresh the affected episodes one by one.

No idea how to prevent DefaultEpisodeHidden.png from being cached and to ensure the real thmb is cached when this option is active.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Jul 14, 2015

The problem with texturecache.py (and other remote apps) is that the hidden image url isn't valid and can't be downloaded for display in the remote app.

When DefaultEpisodeHidden.png is cached by the GUI (which is currently the only way it can be cached), I think what will happen is that once an episode is watched the url for the episode thumb will start being returned as that of the real artwork and when this artwork is next displayed the real artwork will be cached as per normal (the real artwork url won't be in the texture cache until the episode is watched).

The cache should only ever have one entry for DefaultEpisodeHidden.png and this single cached item will be used by all currently unwatched episodes, until such time as they are watched when their real artwork (using a different url) is added to the cache.

So the solution is one of:

  1. Always return real artwork urls to to JSON requests - let the remote app decide whether to display the artwork to the user

or

  1. Return a valid, downloadable url for DefaultEpisodeHidden.png so that remote apps can display the hidden image

Personally, I prefer #1.

@Paxxi

This comment has been minimized.

Copy link
Member

Paxxi commented Jul 15, 2015

@MilhouseVH I agree that method one is preferable, would it make sense to actually send the setting in the response so the app knows right there what the user prefers?

@da-anda

This comment has been minimized.

Copy link
Member

da-anda commented Jul 15, 2015

I'm also for first suggestion

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Jul 15, 2015

I still maintain this should be a multiple selection option to cover all user preferences.

eg Avoid spoilers - No / Plot only / Episode Thumbs only / Both

As it is now I'll have to leave my 'Hide unwatched episode thumbs' option in fTV because I want to be able to read movie plots while hiding episode thumbs as they're the worse for spoilers.

@NedScott

This comment has been minimized.

Copy link
Contributor

NedScott commented Jul 15, 2015

I'm fine with Hitcher's idea as well, but maybe it would be more logical to just separate movies from this logic completely. I don't think I've ever been spoiled by a movie summary (that I can remember). Maybe we could have it like this:

Hide unwatched: TV episodes, everything, off

It's not like we hide TV show summaries, only TV episode summaries.

That being said, I won't oppose the PR as-is. I'd rather we have a starting point and then work from there :)

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Jul 15, 2015

If it were separated would it not be a good idea to place it with the other episode specific option 'Select first unwatched TV show season / episode'?

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 15, 2015

I found another issue with this feature. @MilhouseVH (too many issues now)

  1. If you hide spoliers the DefaultEpisodeHidden.png is active on all unwatched items including the video widgets on home menu as below
    widgets
    Now you enable show spoilers, and everywhere they return to the actual thumb except in this area which requires kodi to be restarted to return to normal. (my initial tests this didn't happen) So something is up with the caching/refreshing.

  2. The other issues (reported earlier) of DefaultEpisodeHidden.png being cached as the thumb for newly added episodes to library, while this feature is enabled and you cant return to the actual thumb without refreshing/removing/rescraping all the affected episodes.

  3. Or issue also when you finish watching an episode and the thumb doesnt switch to real thumb immediatly.

@MilhouseVH @mkortstiege I cant really fix those issues or add options like @HitcherUK suggests.
It would be better if someone who understand the code to try and make this at least bug free by either taking over or pushing the fixes to my branch.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Jul 15, 2015

Now you enable show spoilers, and everywhere they return to the actual thumb except in this area which requires kodi to be restarted to return to normal. (my initial tests this didn't happen) So something is up with the caching/refreshing.

Are you sure this isn't simply a case of the Home Screen Widget not refreshing - it usually only updates after a database update, or if you enter a node, ie. Recently Added, then return to the Home screen it should update. I had tested the scenario you describe and hadn't seen this problem - for items scanned without this option enabled, when enabling and disabling the "Spoiler" option the widget would display the correct thumb (eventually).

  1. The other issues (reported earlier) of DefaultEpisodeHidden.png being cached as the thumb for newly added episodes to library, while this feature is enabled and you cant return to the actual thumb without refreshing/removing/rescraping all the affected episodes.

Ah, I wonder if, while scanning new items when the "Spoiler" option is enabled, the "hidden" artwork url is being written into the media library, meaning DefaultEpisodeHidden.png remains the "real" artwork url even once the episode is watched. That's really bad. I bet if you were to query your MyVideos database using SQL you'd see that you have DefaultEpisodeHidden.png artwork assigned to unwatched episodes (querying the media library using JSON will return unreliable results for unwatched episodes, as previously discussed).

It seems the point at which the "hidden" image is chosen (and assigned) is simply wrong - it happens too early and therefore has too much unwanted impact. Could it be set/selected later, just before the image is thrown on to the screen by the GUI? Whenever anything queries Kodi for the url, it should always return the real url, but in the case of the GUI the hidden image should be substituted just before the thumb artwork is actually displayed.

Perhaps CFileItem could have a ishidden property, which the GUI can act on later in the rendering process?

@un1versal

This comment has been minimized.

Copy link
Contributor Author

un1versal commented Jul 15, 2015

Are you sure this isn't simply a case of the Home Screen Widget not refreshing - it usually only updates after a database update, or if you enter a node, ie. Recently Added, then return to the Home screen it should update. I had tested the scenario you describe and hadn't seen this problem - for items scanned without this option enabled, when enabling and disabling the "Spoiler" option the widget would display the correct thumb (eventually).

I tested that before and didnt see this issue, now I started to see it, I did navigate around to recently added and TV shows and nothing refreshed, only restarting kodi seems to be doing it now, so perhaps turning off this option should trigger an refresh of the widgets and same for when you finish watching an episode?
Same thing when you finish watching an episode (in listing view), a refresh isn't triggered and you need to start watching something and or navigate away to see the artwork return to the actual spoiler thumb..

@MilhouseVH perhaps be best to continue discussions on this at http://forum.kodi.tv/showthread.php?tid=130587 or IRC
Lets hope a kind soul will help this baby being delivered in meanwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.