perf(plugins): decrease number of calls to translate all extensions only once #1359
Conversation
found a couple of places in the code where it was translating the same things more than once, for example
- `translateAllExtensions()` is calling translate on all extensions including a call to `translateColumnHeaders()` but that one was also calling GridMenu translates which was already translated, instead we can expect that `translateColumnHeaders()` should also indirectly translate both ColumnPicker/GridMenu only once
- `onLanguageChange` is calling `translateAllExtensions()` but was also calling `translateColumnHeaderTitleKeys()` and `translateColumnGroupKeys()`, however these last 2 were already covered by `translateColumnHeaders()` so they can be removed
- also a rare but potential infinite loop could happen if `translateColumnHeaders()` is provided with a lang, which then triggers a `onLanguageChange` which then calls back `translateColumnHeaders()` which could be come infinite, so we should make sure that we only trigger a lang change event when the lang is really different
- another small perf improvement, there's no need to check if the translateXYZ method exists on each extensions because we know it exists (that code was put in place when we were using the extenral 6pac/SlickGrid but now everything is internal and we know all these methods exists since it must follow the interface), e.g.
```diff
translateGridMenu() {
- this._gridMenuControl?.translateGridMenu?.();
+ this._gridMenuControl?.translateGridMenu();
}
```
|
@zewa666 here's is a first PR to fix multiple translate calls, I wasn't sure if I should name this PR as |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1359 +/- ##
========================================
- Coverage 99.7% 99.7% -0.0%
========================================
Files 199 199
Lines 21576 21564 -12
Branches 7202 7199 -3
========================================
- Hits 21496 21484 -12
Misses 73 73
Partials 7 7 ☔ View full report in Codecov by Sentry. |
- we already update the columnswith their translations inside `renderColumnHeaders()` so no need to retranslate the extensions again
ghiscoding
changed the title
ghiscoding
changed the title
|
its good to know the tracing of calls to invalidate but I wonder whether this method perhaps should be debounced so that multiple calls are batched into a single one |
I don't really want to add debounce since that would require 1 more cycle before the invalidate kicks in, and this PR is about translation calls, not really invalidate which I'm looking at to do in another PR. There's no call to invalidate at all in this current PR, I did mention about it in the description but it's mainly about the order of execution |
|
yeah I got that and the additional cycle is yet another argument. the core fix in itself makes absolut sense as there's no reason for that additional emit if lang stays the same |
|
I think the PR is ok, so let's merge and a follow up PR is coming for |
- this PR requires and follows Slickgrid-Universal ghiscoding/slickgrid-universal#1359
- this PR requires and follows Slickgrid-Universal ghiscoding/slickgrid-universal#1359
found a couple of places in the code where it was translating the same things more than once
translateAllExtensions()is calling translate on all extensions including a call totranslateColumnHeaders()but that one was also calling GridMenu translates which was already translated, instead we can expect thattranslateColumnHeaders()should also indirectly translate both ColumnPicker/GridMenu only once insiderenderColumnHeaders()when reassigning translatedcolumnstranslateColumnHeaders()will indirectly call agrid.invalidate()so I think this call should be made last intranslateAllExtensions()so that everything else had a chance to be translated prior to reaching the invalidateonLanguageChangeis callingtranslateAllExtensions()but was also callingtranslateColumnHeaderTitleKeys()andtranslateColumnGroupKeys(), however these last 2 were already covered bytranslateColumnHeaders()so they can be removedtranslateColumnHeaders()is provided with a lang, which then triggers aonLanguageChangewhich then calls backtranslateColumnHeaders()which could become infinite, so we should make sure that we only trigger a lang change event only when we see the lang being different6pac/SlickGriddependency, but now everything is internal and we know all these methods already exists since it must follow the interface), e.g.translateGridMenu() { - this._gridMenuControl?.translateGridMenu?.(); + this._gridMenuControl?.translateGridMenu(); }