wordpress-mobile / AztecEditor-Android

A reusable native Android rich text editor component.
Mozilla Public License 2.0
675 stars 112 forks source link

Add the ability to check if changes have been made to the content of the editor #674

Closed daniloercoli closed 6 years ago

daniloercoli commented 6 years ago

This PR adds a utility method to AztecText that should be used to check wether the users have made changes to the content of the posts.

For example this html <b>bold <i>italic</i> bold</b> after being parsed to span and back to html will become <b>bold </b><b><i>italic</i></b><b> bold</b>. Reported in #136 - this is how Google originally implemented the parsing inside Html.java. We've also seen similar problem with img tags.

This PR keeps a copy of the initial content set to the editor and use it later to check if changes have been made to the post content. Tests are also available.

[wp-android details] This PR tries to fix a bad UX where the post is uploaded to the server even if the user has not made any changes to it.

Not sure if I missed something, so better to check with @0nko

mzorz commented 6 years ago

We could also check whether there's a zero length change history and avoid further checking if that's the case, wdyt @daniloercoli?

daniloercoli commented 6 years ago

We could also check whether there's a zero length change history and avoid further checking if that's the case, wdyt @daniloercoli?

I wanted to keep the check on changes as simple as possible, and tried to avoid checks on history, and other "edge" cases, since the performance drawback on checking if there are changes are minimal compared to all the other things the editor is doing in BG. (History can be disabled). Also the checks for hasChanges are executed on demand, when the host app call the method.

daniloercoli commented 6 years ago

Good call @0nko - I've made some changes to make sure to return the original content if there are no changes to the content.

mzorz commented 6 years ago

Another thing I'm thinking of is, instead of having initialEditorContent in memory all the time which could potentially lead to memory issues with big Posts, why don't we calculate a SHA256 digest on the content, keep that as initialEditorContentSHA and only compare that value to a new digest of toPlainHtml() in hasChanges()?

mzorz commented 6 years ago

instead of having initialEditorContent in memory all the time which could potentially lead to memory issues with big Posts, why don't we calculate a SHA256 digest on the content

@daniloercoli @0nko would you take a look into this? https://github.com/wordpress-mobile/AztecEditor-Android/compare/2272616baf7d4ecb949a511a90650c6ed9dee477...feature/add-has-changes-check-sha256?expand=1

This other branch checks for the hash and compares this way, so we don't need to keep a copy of the initial content all the time. I'd revert this last commit as well https://github.com/wordpress-mobile/AztecEditor-Android/pull/674/commits/1cdf83ea34c160458404d9d2e2036e65f9d13c9f and leave it up to the caller to just call hasChanged() and let them decide what is best for them, instead of us keeping a copy in memory (and disk) to circumvent this.

EDIT: updated the branch compare link, so it points to the modification to the specific last commit

mzorz commented 6 years ago

LGTM :shipit: