angular-ui / ui-tinymce

AngularUI wrapper for TinyMCE
MIT License
488 stars 371 forks source link

debounce bug #256

Closed joshribakoff closed 8 years ago

joshribakoff commented 8 years ago

I have a page where the user edits a model & hits "save".

<textarea ui-tinymce="tinymceOptions" ng-model="block.html"></textarea>
    {{block}}

As I type, I do not see {{block}} update until I stop typing for about 1 second. Sometimes the user will hit save before the "debounce" updates the scope, and they lose a bunch of work. Why would it use debounce instead of throttle?

Found this in the source:

/Debounce update and save action
        var debouncedUpdate = (function(debouncedUpdateDelay) {
          var debouncedUpdateTimer;
          return function(ed) {
            $timeout.cancel(debouncedUpdateTimer);
             debouncedUpdateTimer = $timeout(function() {
              return (function(ed) {
                if (ed.isDirty()) {
                  ed.save();
                  updateView(ed);
                }
              })(ed);
            }, debouncedUpdateDelay);
          };
        })(400);

At the very least let the user change the value from 400 to a custom value, to prevent users from saving the old scope before it updates. I feel it really should not be using throttling, not debounce. The users can do that with ng-model-options. Or if throttling is necessary, that would be "Acceptable" but debouce causes major issues.

douglasg14b commented 8 years ago

Discovering the same issue here. If you try and "submit" changes in the TinyMCE text area some of those changes are not yet in the model if it's shortly after I've been typing. This is especially disrupting when using TinyMCE for a chat app.

If there was a callback to cancel the debounce and force a save, that would be nice. I suppose making a call to updateView could do the trick. Though it's a bit deceiving as the view is already updated, and the model is what doesn't reflect the changes.

douglasg14b commented 8 years ago

A fix for this, modify the ed.on function to check for a debounce option.

        ed.on('ExecCommand change NodeChange ObjectResized', function() {
          if(!options.debounce){
              updateView(ed);
              return;
          }
          debouncedUpdate(ed);              
        });

If no debounce option has been included, then it defualts to not debouncing. If a debounce: true property is included in the tinymceOptions then it will debounce. This could of course be turned around to default to debouncing.

deeg commented 8 years ago

Thanks for everyone's input.

I like the proposed fix of making debouncing optional. I will try to get to all of the open PRs tonight. Sorry for the delay.

deeg commented 8 years ago

I ended up leaving debouncing on as default, by allowing you to disable it through options.

I am going to open another ticket to investigate changing to throttle instead of debounce, and allow debounce through ng-model-options.

shyamal890 commented 8 years ago

@deeg would be great if ng-model-options can be supported.

deeg commented 8 years ago

@shyamal890, please try to refrain from commenting on closed issues as they can easily get lost.

There is already another ticket for this #252 which you opened. I am happy to review PRs for implementing this feature!