Make media_player hide image it can't load. #208
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,8 +248,33 @@ | |
|
|
||
| playerObjChanged: function (playerObj) { | ||
| var picture = playerObj.stateObj.attributes.entity_picture; | ||
| var dummy; | ||
| if (picture) { | ||
| dummy = document.createElement('IMG'); | ||
|
Contributor
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. Actually, wouldn't this be cached too?
Contributor
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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.
Contributor
Author
Contributor
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. Registering events is just setting properties, where with polymer it has to propagate through bindings.
Member
There was a problem hiding this comment. 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.onload = function () { | ||
| this.$.cover.style.backgroundImage = 'url(' + picture + ')'; | ||
| dummy.onerror = dummy.onload = null; | ||
| dummy.src = ''; | ||
| dummy = null; | ||
| }.bind(this); | ||
| dummy.onerror = function () { | ||
| this.$.cover.style.backgroundImage = ''; | ||
| this.toggleClass('no-cover', true, this.$.cover.parentElement); | ||
| dummy.onerror = dummy.onload = null; | ||
| dummy.src = ''; | ||
| dummy = null; | ||
| }.bind(this); | ||
| if (this._timeout_id) { | ||
| clearTimeout(this._timeout_id); | ||
| } | ||
| this._timeout_id = setTimeout(function () { | ||
| if (dummy) { | ||
| // Background load still inflight. Clear real image. | ||
| this.$.cover.style.backgroundImage = ''; | ||
| } | ||
| this._timeout_id = null; | ||
| }, 5000); | ||
| dummy.src = picture; | ||
| } else { | ||
| this.$.cover.style.backgroundImage = ''; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.