Conversation
…ned in the edditor, and compares to a current list when the user exits, to determine if contents have changed and post needs be saved
| } else if (!((AztecEditorFragment)mEditorFragment).isHistoryEnabled()){ | ||
| // if history is not enabled, then we can only confirm whether there's been a content change | ||
| // by comparing content | ||
| boolean contentChanged = compareCurrentMediaMarkedUploadingToOriginal(content); |
There was a problem hiding this comment.
It feels like this whole block could be simplified to:
boolean contentChanged;
if (compareCurrentMediaMarkedUploadingToOriginal(content)) {
contentChanged = true;
} else if (mEditorFragment instanceof AztecEditorFragment
&& ((AztecEditorFragment) mEditorFragment).isHistoryEnabled()) {
contentChanged = ((AztecEditorFragment) mEditorFragment).hasHistory();
} else {
contentChanged = mPost.getContent().compareTo(content) != 0;
}
if (contentChanged) {
mPost.setContent(content);
}
There was a problem hiding this comment.
Excellent - just tested it in all 3 cases (images, video, and just user changes through the editor). Replaced the code with your block in ccf116b
| private boolean compareCurrentMediaMarkedUploadingToOriginal(String newContent) { | ||
| List<String> currentUploadingMedia = AztecEditorFragment.getMediaMarkedUploadingInPostContent(this, newContent); | ||
| Collections.sort(currentUploadingMedia); | ||
| return !mMediaMarkedUploadingOnStartIds.equals(currentUploadingMedia); |
There was a problem hiding this comment.
Since mMediaMarkedUploadingOnStartIds is left unchanged here, won't this method return true every time it's called if changes were detected the first time this is called?
There was a problem hiding this comment.
As long as changes happen in between the user opened the Editor (which is when mMediaMarkedUploadingOnStartIds is populated) and exiting the Editor, that's exactly the window we want to cover, so yes it'd be fine. Unless I'm not seeing some side effect of it that could potentially be harmful?
There was a problem hiding this comment.
If a media item finished uploading between the start of the activity and the first time updatePostContentNewEditor is called, then every time it's called afterwards it will also return true. Looking at the flow from savePostAsync > updatePostObject > updatePostContentNewEditor, won't this mean that the post could be updated (and uploaded) unnecessarily?
… avoid unnecessary further Post saves
|
|
Fixes #6654
The reason it was failing as described in #6654 is because we look for changes only by checking whether the Aztec editor has a Change History (change introduced in #6610). Given the completed uploads actually do change the URL but they do so in the background, thus not affecting Aztec’s Change History, we think there’s nothing new and thus we don’t save the latest Post content (with the updated remote URLS for media that have been uploaded).
It is not trivial to understand which changes need to be saved, given a dumb string comparison can return different strings for semantically identical HTML content (for instance, the order in which a div class appears makes the string different but the HTML piece is no different from each other).
So, this PR tackles this by relying on what we do know: when the Editor is opened, we know which media items in the Post content are marked as being uploaded. Same thing when the Editor is closed: when the user exits the editor, and we have any change in uploads for this Post (i.e. the existing uploads when we opened the Editor have now completed when we try to exit the Editor), then we need to save the Post content as a change in the media items URL has happened.
To test:
FOR IMAGES:
FOR VIDEOS:
cc @nbradbury