Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Reduce digest triggers when using $timeout #5829

Merged
merged 1 commit into from
Dec 5, 2016

Conversation

csvan
Copy link
Contributor

@csvan csvan commented Nov 23, 2016

Angular's $timeout takes a boolean 3rd parameter which, if false, causes the wrapped function to not run inside $apply. Since $timeout is used extensively in core, this parameter has been passed where possible in order to improve performance. Previously, the grid would trigger a large number of digests even when working normally, see issue: #5007.

This is very low-hanging fruit. It is possible the same investigation can be applied across the plugins as well.

Angular's $timeout takes a boolean 3rd parameter which
causes the wrapped function to not run inside $apply.
Since $timeout is used extensively in scope, this parameter
has been passed where possible in order to improve
performance, since the grid otherwise triggers a large
number of digests while functioning normally.

See issue: angular-ui#5007
@csvan
Copy link
Contributor Author

csvan commented Nov 28, 2016

Anyone around to take a look at this?

@mportuga mportuga merged commit e8af48d into angular-ui:master Dec 5, 2016
@mportuga
Copy link
Member

mportuga commented Mar 6, 2017

@MichalJakubeczy @csvan i will try to see if I can include at least part of these changes in the next release of UI-grid. The original version was causing some ui-problems. The most noticeable one that I remember, was a case where I had a toggle-able filter row with a pinned column. In that scenario, for whatever reason, there was either a delay between the pinned and the unpinned filter row display, or it was just partially displaying things. I seem to recall some other UI issues, but none quite as memorable.

@MichalJakubeczy
Copy link

@mportuga thanks for your answer. Any performance improvements would be awesome as we are struggling with that a bit. Anyway, when is a new release planned would it be angular 1.6.2 compatible?

@mportuga
Copy link
Member

mportuga commented Mar 8, 2017

We are trying to do a minor release soon with the PRs that we merged recently. I think there was one of them that adds some catch statements that make it work a bit better with angular 1.6, but it is not necessarily making it angular 1.6 compatible. Right now we are concentrating on performance and memory leaks since those are the big issues that we need fixed. But we would welcome any PR which makes 1.6 support possible.

mportuga pushed a commit to mportuga/ui-grid that referenced this pull request Mar 13, 2017
Re-implemented some of @csvan's work with PR angular-ui#5829 and improved refreshCanvas function.

5007
mportuga added a commit that referenced this pull request Mar 15, 2017
Re-implemented some of @csvan's work with PR #5829 and improved refreshCanvas function.
Also, update the All features tutorial to add a toggle button for the filter row and improved coverage in ui-grid.js

5007
var promise = $timeout(focusBySelector);
this.queue.push($timeout(focusBySelector));
var promise = $timeout(focusBySelector, 0, false);
this.queue.push($timeout(focusBySelector), 0, false);

Choose a reason for hiding this comment

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

@csvan there is a mistake here, should be:

this.queue.push($timeout(focusBySelector, 0, false));

right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's a mistake

vishalnarewade pushed a commit to vishalnarewade/ui-grid that referenced this pull request Nov 6, 2017
Re-implemented some of @csvan's work with PR angular-ui#5829 and improved refreshCanvas function.

5007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants