Make media_player hide image it can't load.

[andrey-git]

Make media_player hide image it can't load.

Note that it will still try on every state change.

[mention-bot]

@andrey-git, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @bjarniivarsson and @armills to be potential reviewers.

[balloob]

There is a second argument to this function that could help with avoiding trying to catch the picture twice: oldPlayerObj. In that case you can avoid doing any image calculation/attr setting if the picture url is the same.

[andrey-git]

In the backend PR there was a concert that the failure might be temporary. If we cache the URL the image won't come till the next song.

[emlove]

I still support allowing the same URL to retry. Otherwise, the implication would be that any temporary interruption blocks retrying, and could lead to confusion, especially when someone is trying to debug a new media platform.

[balloob]

🐬

Ok to merge when tests pass.

[andrey-git]

I don't have write access in this repo.

[emlove]

We should wrap this in an `if (this.$.cover.style.backgroundImage != 'url(' + picture + ')'), or something along those lines, so that once the image loads successfully, it doesn't keep reloading it.

[balloob]

I like that 🔝

[balloob]

Actually, wouldn't this be cached too?

[emlove]

Hmm, yeah, you're right that the image would be cached. We would still be going through the code of creating the IMG element, but that might be inexpensive enough to justify keeping the code simpler.

[emlove]

What about having a hidden iron-image in the template? The image src could just be a computed binding to the entity_picture. We can then listen to the loaded and error properties and update the background image when these events fire.

[andrey-git]

@balloob The image will be cached, so I don't think if is needed here.
@armills What is the advantage of iron-image? It will attached to the dom so might (not sure) affect performance more.
I don't think the code will be more compact.

[emlove]

The advantage would be that we aren't creating an element and registering events on every state change. I also think that as a high-level idea that making use of the polymer features when available will be more stable than writing our own javascript implementations. This instance should be pretty safe, but using polymer as a practice lets take advantage of their compatibility layer for various browsers.

It will probably be a little larger in LOC, but IMO moving the load/error handlers outside of the playerObjChanged handler will make it easier to quickly grok.

[andrey-git]

Registering events is just setting properties, where with polymer it has to propagate through bindings.
I could extract the functions to separate functions here too. I don't think it is important for readability here.

[balloob]

I'm fine with not using iron-image. If we will need this in another place, we should extract this into a util function that returns a promise.

Oh and next time, you can use dummy = new Image();

[emlove]

I like where this is going. This should also make image transitions smoother, since they'll be loaded in the background before changing the card background image, making the switch smooth.

We might want to also consider adding a timeout to clear the background image if onload or onerror haven't triggered in a timely manner (2-5 seconds?), to prevent keeping the stale image visible for too long.

[andrey-git]

Timeout added,