Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(ng-model-options): Canceling debounces now forces a view reset #7011

Closed
wants to merge 1 commit into from

Conversation

lrlopez
Copy link
Contributor

@lrlopez lrlopez commented Apr 5, 2014

This PR forces a view reset when canceling a pending debounce in order to fix some unexpected behaviors.

Closes #6994

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7011)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@@ -1701,10 +1701,13 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* This method should be called before directly update a debounced model from the scope in
* order to prevent unintended future changes of the model value because of a delayed event.
*/
this.$cancelDebounce = function() {
this.$cancelDebounce = function(skipRender) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should rename this skipReset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I'll also document the new parameter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about skipViewValReset to make it more explicit?

@lrlopez
Copy link
Contributor Author

lrlopez commented Apr 5, 2014

Since $resetModelValue actually resets the view and the input value and doesn't touch the model we should rename it...

@petebacondarwin
Copy link
Contributor

Yes you are probably right... $resetViewValue(), or simply $resetView()?

@lrlopez
Copy link
Contributor Author

lrlopez commented Apr 5, 2014

Both would be right... But $resetView() is shorter, so I'd choose the later 😉

@@ -1783,6 +1786,20 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
// model -> value
var ctrl = this;

this.$resetModelValue = function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$resetView code shares a lot with ngModelWatch. In fact the only difference is that ngModelWatch only updates $viewValue and calls $render when there is an actual change. We could add a parameter to $resetView which could allow enabling the check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was wondering about that. Something like like forceRender?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just call this fn from ngModelWatch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ngModelWatch is a bit different since it tests that the $modelValue and $viewValue actually changed (this fn is forcing it). We could add a force param here and call this from ngModelWatch. But I was wondering if it is even needed to re-run the $formatters, we could just invoke $render...

@lrlopez
Copy link
Contributor Author

lrlopez commented Apr 5, 2014

May be another name is better. What about $syncModelToView? Is what the functions actually does

@shahata
Copy link
Contributor

shahata commented Apr 5, 2014

I don't see why we need to re-run the $formatters instead of simply calling $render. I'm not even sure we need to expose a new API, it might be better to just invoke $render directly for those cases. Also I'm a bit disappointed with this since I said I'm on this PR.

@IgorMinar
Copy link
Contributor

@shahata is right. it should be enough to call $render when debounce is canceled.

Also please coordinate the effort better if possible.

@petebacondarwin
Copy link
Contributor

@shahata - sorry, we are not trying to subvert your efforts. @lrlopez and I were chatting and thought it would be easier to discuss if there was a PR. You are definitely going to get to fix this.

@petebacondarwin
Copy link
Contributor

@shahata - can you join http://gitter.im

@petebacondarwin
Copy link
Contributor

We do need you involved in this

@petebacondarwin
Copy link
Contributor

@lrlopez lrlopez closed this Apr 5, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Programmatic updates of the model in ngModelOptions
5 participants