Detect content changed

[hypest]

Addresses part of #182

Fit the Bridge interface with a extra boolean to denote if the returned html is changed compared to the original html the editor was started with. Parent app can then optimize for not the case where the content didn't change (e.g. not marking the post as changed, save it and so on).

Changes in this PR

• State adopted the initialHtmlHash member. Reducer code adapted to support it.
• Html change detection is using md5 hashes to compare the html versions
• BlockType and StateType moved to own file. Similar with a couple of state util functions.
• Bridge middleware compares html hashes on BLOCK.SERIALIZE_ALL, to report to parent app

To test A:

App should compile and run as usual. No visible feature change.

To test B (Android only):

• Use the gb/detect-content-change branch from wpandroid. Usual submodule update, yarn install, yarn start:reset dance to launch Metro.
• yarn wpandroid to launch wpandroid
• Open a Gutenberg post that has no local changes. Don't make any changes and go "Back".
• Notice the post retaining the status it had before, without marking the post as having local changes.

cc @etoledom , @pinarol as this includes Bridge changes to pass the "changed" flag.

[koke]

I wonder why not use the same approach that the editor store uses with withChangeDetection.

I know we've used md5 hashes in the past because it was convenient and we didn't have a good way to keep track of changes, but having redux in place and a util that already does this, it seems like a better choice and less wasteful.

[hypest]

I wonder why not use the same approach that the editor store uses with withChangeDetection.

I guess no particular reason to avoid withChangeDetection other than it seems to depend on assumptions on the Reducer and the State schema itself that seem not true for the mobile project atm. We currently have a couple of state fields (refresh, fullparse that we use for signaling instead of state per se. I think those will trigger the isDirty flag without the html content actually changing.

Then again, I can be wrong but, withChangeDetection seems to detect changes only between two consecutive states (one action). So, even if after a few actions the final html is the same as the original, isDirty will have been triggered. Can you confirm?

[koke]

It looks like you can configure which actions to ignore. I think it would stay dirty if you end up with the same content after a few edits but that’s ok with me

[hypest]

I'd be careful to avoid changing the interface with the main app at this point. We have content-based change detection in Aztec-Android atm and I'd lean on keeping that until we re-attack this problem when we port the GB-web's store, soon.

That said, @diegoreymendez let me ask, what's the current pattern on iOS and Aztec to report if the post has changes or not? Can you share some info on that? Thanks!

[koke]

After some chat, I'm fine with the approach for the time being, although I'd like to revisit this in the future when we have better integration with Gutenberg in the store layer and some more time to spare.

[hypest]

I'd like to revisit this in the future when we have better integration with Gutenberg in the store layer and some more time to spare.

Similar to other cases, I opened a ticket to keep it in our radar: #227

[pinarol]

All OK. I've tested the changes via Android app

Test 1:

• Open a blog post and don't change anything on it
• Tap back and go back to the posts page
• Verify there's no "Local changes" indication on the post summary

Test 2:

• Open a blog post and change its title
• Tap back and go back to the posts page
• Verify there's "Local changes" indication on the edited post summary

[pinarol]

just an idea: we can make use of ES6 classes for State to make use of constructors, getter&setters, for example every time the html is set we can update blocks and initialHtmlHash inside the class. or we can define a constructor that is taking html as input and update blocks and initialHtmlHash in that constructor. again this is just an idea current version is also ok for me.

[hypest]

Let me see if I understand the proposal: Will that class update its internal vars?

If yes, I'm not sure that's what we want for Redux. My understanding is that the reducer is supposed to return a new "state" object and avoid mutating the old one, isn't it? In that case, I'm not sure a State class would be too useful since we would essentially only use its constructor, and that's pretty much what html2State performs.

Let me know if there is something in the proposal that I'm missing, thanks!

[hypest]

Oh, and btw, we might don't want to spend too much time iterating on this since we will be using GB's stores soon #226 anyway.

[pinarol]

You are right about mutating. we could figure it out though maybe there's an easy way to clone a class object. I'll investigate this further on a more free time and If I discover a useful practice I will post a p2 about it.

[hypest]

Thanks for the review @pinarol !

Test 2:

Open a blog post and change its title

Good idea of a test! You might also want to try editing something inside the post content since, as it currently stands, the title itself is a different input field, not rendered by the RN app.

[pinarol]

Extra testing:

Test 1:

Try changing
image/code/paragraph/heading blocks

Change only one of them at a time
Return back and see that "Local changes" indication IS there
Publish the changes and see "Local changes" has gone
Try changing next block and repeat steps

Test 2:

Open the post
Focus on different block but don't change the content
Return back to the post list
See that "Local changes" indication is NOT there

Test 3:

Open the post
Change the order of blocks
Return back to the post list
See that "Local changes" indication IS there

[pinarol]

Thanks for the review @pinarol !

Test 2:
Open a blog post and change its title

Good idea of a test! You might also want to try editing something inside the post content since, as it currently stands, the title itself is a different input field, not rendered by the RN app.

Right! totally missed that! Thanks for pointing out.

[hypest]

Thanks for the thorough testing @pinarol ! Will go ahead and merge now 🙇‍♂️!