Check change history size to test for content changes in Post#6610
Check change history size to test for content changes in Post#6610
Conversation
| } | ||
|
|
||
| public boolean hasAnyChanges() { | ||
| return (content.history.getHistoryEnabled() && !content.history.getHistoryList().isEmpty()); |
There was a problem hiding this comment.
I'm fairly new to Aztec so I have to ask: when would history not be enabled?
There was a problem hiding this comment.
Good question! I think never in WPandroid, but given it is a possibility (that Aztec allows to enable/disable the feature) I thought it would be better to future-proof it and make the check anyway
There was a problem hiding this comment.
In that case, maybe this method should be renamed to hasHistory()?
There was a problem hiding this comment.
sounds good 👍 , changed in 8035d97
|
Good call on only tackling this for Aztec, and for relying on the history rather than string comparison to determine if anything has changed. 💯 I wonder, though, why this is branched from |
:D fair enough, changing the base branch to |
| } | ||
|
|
||
| if (contentChanged) { | ||
| contentChanged = PostUtils.updatePostContentIfDifferent(mPost, content); |
There was a problem hiding this comment.
I think we can skip the call to updatePostContentIfDifferent() since all that does is update the passed post model if the content is changed, and we already know the content is changed. Seems like we could just use mPost.setContent(content) if we know the content has changed and skip the potentially expensive string comparison.
There was a problem hiding this comment.
Yeah, that's why I was asking about history - if it was disabled, the changes would never be saved. Adding that check probably isn't necessary but I feel safer having it there :)
|
Looks good! |
Fixes #6599 partially (on Aztec only).
In investigating this, we realized the problem was that both Aztec and the Visual editors do change the content even if the user doesn't do anything with it, as follows.
Aztec
Using the Aztec editor to open a post that contains 1 media item (at least) and has been written using wp-admin or Calypso, this is how the original Post content looks like:
Original content
When getting this content into Aztec and then exiting the editor, re-creating the html back using
AztecText.toHtml(), the produced content looks like this:Content as rendered by the editor
Look at the two
imgtags, I'll put them together here:So two things are different:
classproperty within the string is different (wp-adminplaces it in the end of the tag, while Aztec places it at the beginning)/>. (BTW don't know whether this is a bug or not, as the Visual Editor behaves the same way as we'll see).Visual Editor
Original content
Content rendered by the editor
=> Here, the only difference is the wrong closing of the
<imgtag (again without the/>).The way we were using to test if something changed is by means of a string comparison between the original and new content. Because the string actually changes, we mark the Post as
Locally changed.The proposed solution on Aztec
Now, trying to compare whether two HTML expressions are semantically identical is a difficult thing to achieve, and probably fairly off the scope of this issue in particular. So what this PR does, is it takes advantage of the change history capability placed in Aztec, which keeps track of a change history and makes it possible to undo/redo changes. If the user does anything with the content, it will be tracked in its history tracker. So in order to learn if the content has been edited at all, all we need to do is checking if the change history size is any different than zero.
The proposed solution on the Visual Editor
None that I can think of without incurring in a significant effort. I think we could leave it as is, as eventually it will be replaced by Aztec at some point.
To test:
cc @nbradbury @aforcier