angular-ui / ui-ace

This directive allows you to add ACE editor elements.
http://angular-ui.github.io/ui-ace
MIT License
578 stars 172 forks source link

Use $evalAsync instead of $apply to make sure ngModel gets updated once editor content changes #77

Closed rebornix closed 9 years ago

rebornix commented 9 years ago

Currently we are using !scope.$$phase && !scope.$root.$$phase to avoid to call $apply if there is already a digest loop running. The ngModel binded to editor content would not get updated until users modify the content once again. If users try to access the ngModel binded to editor content, he would get an incorrect answer.

We can push our ngModel updating task into $evalAsync queue.It would be executed before rendering the DOM and it won't conflict with digest loop.

Attach angularjs event loop workflow.

workflow

Thanks.

rebornix commented 9 years ago

Build failure was introduced before my change.

SummerSun commented 9 years ago

:+1: :+1:

formulahendry commented 9 years ago

Make sense!

PowerKiKi commented 9 years ago

ui-ace declare a dependency on Angular ~1.x, but I believe $evalAsync is not available in the 1.0 branch. So dependency should be updated accordingly in this PR.

rebornix commented 9 years ago

@PowerKiKi I have updated angularjs dependency in bower.json and readme. The build is failing, I'll try to fix it.

PowerKiKi commented 9 years ago

I am not sure what to think of that. Specifying a devDependency on "angular-mocks": "1.3.0" make bower resolves to angular to 1.3.0. So basically everything is on 1.3. Should we just set a dependency on angular 1.3 ? or keep this "in-between" situation ? I am not sure what is the best solution here...

@douglasduteil would you have an opinion on this ?

rebornix commented 9 years ago

Finally I figure this out. From Angular-mocks 1.3.1 (and AngularJS 1.3.x), we have to provide the <div ui-ace> element with controller ngModel explicitly, otherwise it would not be loaded as we declare ng-model as optional in link phrase.

return {
      restrict: 'EA',
      require: '?ngModel',
      link: function (scope, elm, attrs, ngModel) {

@PowerKiKi I have just reverted bower.json back and our dependency is Angularjs >=1.1 from now on, not just 1.3.

douglasduteil commented 9 years ago

Hi That's right @PowerKiKi the bower dev dependencies is wrong. @rebornix I'm surprise to see that much modification just for fixing the ngModel behavior I'll try to spend some time updating this repo in the week or the next.

rebornix commented 9 years ago

@douglasduteil good to know you can have a look at this repo. I'll keep updating this pr if I find better ways to get rid of build failure and this bug.