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.

Sign up for GitHub

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

Retry to download an image if a download error occured after an upload #3803

Closed
wants to merge 5 commits into from
Closed

Retry to download an image if a download error occured after an upload #3803

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4387667
add missing comment
maxme Feb 29, 2016
6b4d174
fix wordpress-mobile/WordPress-Editor-Android#300: Retry download onE…
maxme Feb 29, 2016
d7dd4f6
Add back image swapping onError
maxme Mar 2, 2016
ef2bcb1
fix function call errors
maxme Mar 2, 2016
7fba5d6
use a remoteUrl attribute to avoid seeing broken image if download fa…
maxme Mar 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 43 additions & 16 deletions libs/editor/libs/editor-common/assets/ZSSRichTextEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,29 +919,55 @@ ZSSEditor.replaceLocalImageWithRemoteImage = function(imageNodeIdentifier, remot
var image = new Image;

image.onload = function () {
imageNode.attr('src', image.src);
imageNode.addClass("wp-image-" + remoteImageId);
ZSSEditor.markImageUploadDone(imageNodeIdentifier);
var joinedArguments = ZSSEditor.getJoinedFocusedFieldIdAndCaretArguments();
ZSSEditor.callback("callback-input", joinedArguments);

ZSSEditor.finishLocalImageSwap(image, imageNode, imageNodeIdentifier, remoteImageId)
image.classList.add("image-loaded");
console.log("Image Loaded!");
}

image.onerror = function () {
// Even on an error, we swap the image for the time being. This is because private
// blogs are currently failing to download images due to access privilege issues.
//
imageNode.attr('src', image.src);
imageNode.addClass("wp-image-" + remoteImageId);
ZSSEditor.markImageUploadDone(imageNodeIdentifier);
var joinedArguments = ZSSEditor.getJoinedFocusedFieldIdAndCaretArguments();
ZSSEditor.callback("callback-input", joinedArguments);

// Add a remoteUrl attribute, remoteUrl and src must be swapped before publishing.
image.setAttribute('remoteurl', image.src);
// Try to reload the image on error.
ZSSEditor.tryToReload(image, imageNode, imageNodeIdentifier, remoteImageId, 1);
}

image.src = remoteImageUrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This was needed to make sure the image inside the container (the 'real' image, not the one we load into memory using the image object) is set to the remote URL. With this line removed, if the image fails to load, the local image isn't swapped out, resulting in a 'loaded'-looking image with a local (Android filesystem) src, which can be uploaded.

We should keep this, so that even if the image fails and never loads for some reason, the post is still uploaded with a remote URL. The comment doesn't make its use clear though, I think we can add some clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow-up note: this is a little trickier than I initially realized, since we'll be showing the user a dead image icon for however long it takes to load the image from the server. This might only be for 500ms, but it's still not ideal. I still think that's better than risking uploading the post with the (wrong) local image url.

A permanent solution might be to keep the line removed as you had it, but make sure the local src is swapped for the remote one if the user attempts to upload before the image has been loaded from the server from onerror retries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might only be for 500ms, but it's still not ideal. I still think that's better than risking uploading the post with the (wrong) local image url.

Yes, it's a bit weird, but I think it's better to not change the HTML source to fix a graphic glitch - we could end up breaking the post.

Here a demo with a 5 secs delay:
https://cloudup.com/cErFyGdP1GX

A permanent solution might be to keep the line removed as you had it, but make sure the local src is swapped for the remote one if the user attempts to upload before the image has been loaded from the server from onerror retries.

What would be the best solution to mark it "unswapped yet", add a class to the img?

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the best solution to mark it "unswapped yet", add a class to the img?

I think a good solution would be adding a remoteUrl attribute, set to the remote URL once we have it. Then we add an extra step to ZSSEditor.removeVisualFormatting() to find any image tags with that attribute and swap out the src for remoteUrl. That should cover the case where the image never loads before publishing.

};

ZSSEditor.finishLocalImageSwap = function(image, imageNode, imageNodeIdentifier, remoteImageId) {
imageNode.addClass("wp-image-" + remoteImageId);
if (image.getAttribute("remoteurl")) {
imageNode.attr('src', image.getAttribute("remoteurl"));
} else {
imageNode.attr('src', image.src);
}
ZSSEditor.markImageUploadDone(imageNodeIdentifier);
var joinedArguments = ZSSEditor.getJoinedFocusedFieldIdAndCaretArguments();
ZSSEditor.callback("callback-input", joinedArguments);
image.onerror = null;
}

ZSSEditor.reloadImage = function(image, imageNode, imageNodeIdentifier, remoteImageId, nCall) {
if (image.classList.contains("image-loaded")) {
return;
}
console.log("Reloading image:" + nCall + " - " + image.src);
image.onerror = ZSSEditor.tryToReload(image, imageNode, imageNodeIdentifier, remoteImageId, nCall + 1);
// Force reloading by updating image src
image.src = image.getAttribute("remoteurl");
}

ZSSEditor.tryToReload = function (image, imageNode, imageNodeIdentifier, remoteImageId, nCall) {
if (nCall > 8) { // 7 tries: 22500 ms total
ZSSEditor.finishLocalImageSwap(image, imageNode, imageNodeIdentifier, remoteImageId);
return;
}
image.onerror = null;
console.log("Image not loaded");
// reload the image with a variable delay: 500ms, 1000ms, 1500ms, 2000ms, etc.
setTimeout(ZSSEditor.reloadImage, nCall * 500, image, imageNode, imageNodeIdentifier, remoteImageId, nCall);
}

/**
* @brief Update the progress indicator for the image identified with the value in progress.
*
Expand Down Expand Up @@ -994,7 +1020,8 @@ ZSSEditor.markImageUploadDone = function(imageNodeIdentifier) {
// Wrap link around image
var linkTag = '<a href="' + imageNode.attr("src") + '"></a>';
imageNode.wrap(linkTag);

// We invoke the sendImageReplacedCallback with a delay to avoid for
// it to be ignored by the webview because of the previous callback being done.
var thisObj = this;
setTimeout(function() { thisObj.sendImageReplacedCallback(imageNodeIdentifier);}, 500);
};
Expand Down