Closed 3rdvision closed 2 years ago
Woof, Woof!
Thank you @3rdvision for submitting the "#357 Debouncer for setting the initials" pull request 😎.
Please do not forget to review our internal guidelines:
Engaging in the development process in the best possible way helps it being efficient and fast.
Your friend, Tobias (Platforme's mascot)
Woof, Woof!
Oops! @3rdvision you may have to fix some errors:
Please address these issues to continue with the Pull Request merge process.
Your friend, Tobias (Platforme's mascot)
Just to check, did you explore a possible solution where a cancel flag is used to stop the update promise before the existence of
loadedCallback
?
I did made an implementation of it but it didn't offer much performance difference. The problem is update will await any previous updates here after it cancels pending updates here and there wasn't a significant performance improvement, maybe about from 6.2s to 5.2s while using the debouncer it's about 6.2s to 3.1s.
It's also difficult to distinguish set initials update. In some places we have reason specifying set initials but because we also trigger initials_extra to notify the client that the initials changed after setInitialsExtra
, we have a bind for initials_extra and in other places which will do a setInitials later without including params.reason="set initials_extra" and even thought we could refactor to always track it, from what I've tried, that experience didn't hold significant performance improvements versus this one.
Just to check, did you explore a possible solution where a cancel flag is used to stop the update promise before the existence of
loadedCallback
?I did made an implementation of it but it didn't offer much performance difference. The problem is update will await any previous updates here after it cancels pending updates here and there wasn't a significant performance improvement, maybe about from 6.2s to 5.2s while using the debouncer it's about 6.2s to 3.1s.
It's also difficult to distinguish set initials update. In some places we have reason specifying set initials but because we also trigger initials_extra to notify the client that the initials changed after
setInitialsExtra
, we have a bind for initials_extra and in other places which will do a setInitials later without including params.reason="set initials_extra" and even thought we could refactor to always track it, from what I've tried, that experience didn't hold significant performance improvements versus this one.
What I was sugesting was end the updatePromise earlier on cancel, since if the update process did yet not reach the new Promise.....
that is saved as loadedCallback
when the cancel is called, the whole update is made. With the approach I mentioned, this does not happen. For example, when writting a whole word, only the last request is completed (with the whole word), instead of the first and then the last.
Example inside _update
closure in update
:
...
// verifies if the model currently loaded in the Ripe Instance can
// render the frame to be display and if that's not the case "ignores"
// the current request for update
if (frame && !this.owner.hasFrame(frame)) {
this.trigger("not_loaded");
return false;
}
if (this.cancelFlag) return;
// builds the URL of the image using the frame hacking approach
// this should provide us with the new values
const url = this.owner._getImageURL({
frame: frame,
...
});
...
The cancelFlag
is set as true in the cancel
method and as false after the cancel call in update
. If the previous update has not yet reached the assignment of this._loadedCallback
, the _update
method would return instead of executing until the end.
Closing this in favor of a more performant approach https://github.com/ripe-tech/ripe-sdk/pull/366
initialsDebounce=275
forsetInitials
andsetInitialsExtra
.- Adapt tests
Notes:
update
was the first approach but failed to be optimal as the eventinitials_extra
triggers for anupdate
with a nullparams.reason
and it's not possible to adapt it without a huge refactor.