Save callback is triggered too often (when nothing changes)

[Reinmar]

The save callback is executed twice with the same content in a short period of time. I think that this is due to a flawed implementation of the throttle() function.

Disclaimer: I never remember which is throttling and which is debouncing, so I'll use the word throttle, but please read what's the expected semantics of it and don't focus on the name.

The throttling in autosave should work like this:

• We don't want to save characters typed one after another, so we wait until someone stopped typing for minimum X and we save the content.
• However, if someone is typing quickly for a longer period, we need to backup that in the meantime. So, if time Y passed since the first change of a series of changes, we save that anyway.

X may equal Y, but ideally they should be independent. It makes sense to make Y longer so you don't constantly save things while a person is typing. Typically, I'd set X=1s, Y=5s (or even more).

How the current implementation seems to work is this:

• First change occured – save the content.
• Don't save the content for the next X.

And the first point makes for the first "Saving... foo" while the second for another "Saving... foo".

[Reinmar]

Example how typing is now saved:

Saving the first character doesn't make sense.

[ma2ciek]

We don't want to save characters typed one after another, so we wait until someone stopped typing for minimum X and we save the content.
However, if someone is typing quickly for a longer period, we need to backup that in the meantime. So, if time Y passed since the first change of a series of changes, we save that anyway.

I implemented throttling, while that's more like debouncing with the maximum waiting time set. The current implementation can be changed of course.

Your proposal combined with https://github.com/ckeditor/ckeditor5-autosave/issues/9 will make the server calls rare.

As I remember, that was a reason before for using the throttle and synchronous first call, but now it makes little sense.

[pjasiun]

What we want to have is debounce, see:

(via https://css-tricks.com/debouncing-throttling-explained-examples/)

Save should be executed when the user stops editing (~1-2sec after the last change event, need to be tested, it should not be too often but also the user should not wait noticeably long for the saving action).

In the F2F talk, we agreed that in the first implementation we do not need any save action while the user is editing (change action is fired often). We believe that the user does pause and it is the best moment to save the data. However, the pending action should be added as soon as the user do the first change, so he will see a warning when trying to exit with data not saved.

[Reinmar]

So you're right regarding what debounce is :D I'll never remember which is which :P

[Reinmar]

BTW, does it mean that we may be able to use lodash's implementation? Would be nice if we could.

[ma2ciek]

BTW, does it mean that we may be able to use lodash's implementation? Would be nice if we could.

I think so. That's because there will be only one saving action at a time. The plugin might be in only 3 states: no action, debounced saving, waiting for the server response.

I'll describe the scenario below:

When there's no action and a new change:data event fires, the Autosave plugin changes its state to debounced saving and calls the debounced save method. It adds new action to the pending action manager.

When the plugin is in the debounced saving state and a new change:data event fires, it only calls the debounced save method.

When the save method is called, the plugin changes state to the waiting for the server response, calls the provided adapter.save method and waits for the response. In this state when the change:data happens, it stores the information, that new change happens in the meantime.

Once it gets the response from the server, it can perform two actions:

If there's no stored information about a new change, it changes the state to the first, no action and removes the action from the pending manager.

If there's an information about a new change, it changes the state to the debounced saving and calls the debounced save method.

Does it make sense for you?

[ma2ciek]

Unfortunately, we have some old lodash lib in the ckeditor5-utils/lib/lodash, which contains the debounce function, that can't be mocked using fake timers.

The old debounce uses now function, that comes from now = Date.now, so after sinon mocks the Date.now, the now function remains untouched: https://github.com/ckeditor/ckeditor5-utils/blob/98508b0c15044ef870e2301edc1a0f2f41a8aeb1/src/lib/lodash/debounce.js

The current implementation uses just Date.now(): https://github.com/lodash/lodash/blob/48511837573d99138fc43b7e9f04eea7b01839fe/debounce.js
We have this problem in a few places already:

• https://github.com/ckeditor/ckeditor5-engine/blob/00fbf7faba81b8e9c3caa3558a22e5a1b66ff17a/tests/view/observer/fakeselectionobserver.js#L112
• https://github.com/ckeditor/ckeditor5-engine/blob/00fbf7faba81b8e9c3caa3558a22e5a1b66ff17a/tests/view/observer/selectionobserver.js#L253
• https://github.com/ckeditor/ckeditor5-ui/blob/7df7018e5ca1d423d7aafb8d4d106def4019d000/tests/toolbar/balloon/balloontoolbar.js#L105

I'll write these tests using some short timeouts, but we should consider updating this lib or make it an external dependency.

[pjasiun]

Unfortunately, we have some old lodash lib in the ckeditor5-utils/lib/lodash, which contains the debounce function, that can't be mocked using fake timers.

Can we just update lodash version we use?

[ma2ciek]

Can we just update lodash version we use?

It'd be perfect, at least for now. I can check if tests will pass with the updated library.