Skip to content
This repository has been archived by the owner on Mar 5, 2021. It is now read-only.

Remove scheduled save when during asset deletion, closes #90 #109

Conversation

chrisbubernak
Copy link
Contributor

Added code inside the asset deletion method to remove the pending callback from the scheduled save.

added code inside asset deletion method to remove the pending callback
fo its scheduled save
@chrisbubernak
Copy link
Contributor Author

So my fix stops the callback from happening but I noticed that the object containing the scheduled saves never actually has entries removed from it until the server goes offline, all that happens is the values in it get null-ed out. Is this intentional? Seem's kind of like a memory leak in a sense because in the pathological case (adding tons of sprites /w images to your project) this is just going to grow and hang around for ever. Is there some purpose for that? If not, is it something work fixing?

@@ -215,6 +215,14 @@ export default class RemoteProjectClient extends BaseRemoteClient {
}
}

// Cancel any scheduled save for this asset
let scheduledSaveCallback = this.server.scheduledSaveCallbacks[`assets:${entry.id}`];
if (scheduledSaveCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer (scheduledSaveCallback != null)

@elisee
Copy link
Contributor

elisee commented Feb 21, 2016

Gave this some thought. Since assets on disk will be preserved in the trashedAssets folder, I feel like we should not cancel the scheduled save, but rather force it to happen right away (before or after moving the folder elsewhere)? What does everyone think?

So my fix stops the callback from happening but I noticed that the object containing the scheduled saves never actually has entries removed from it until the server goes offline, all that happens is the values in it get null-ed out. Is this intentional?

Yeah that looks wrong indeed. I can't think of a reason why we would rather null them out than deleting them.

@chrisbubernak
Copy link
Contributor Author

Gave this some thought. Since assets on disk will be preserved in the trashedAssets folder, I feel like we should not cancel the scheduled save, but rather force it to happen right away (before or after moving the folder elsewhere)?

@elisee just to clarify we still want to remove the save from the scheduledsaves object but we just want to actually do the save then instead of just forgetting about it, correct?

@elisee
Copy link
Contributor

elisee commented Feb 28, 2016

@chrisbubernak Yes that sounds like the correct approach. But since saves are async operations, we probably need to move the asset folder first and ensure that the save happens in the trashedAssets path.

(This probably all ties in with #67 but we need not worry too much about it for now)

@bilou84
Copy link
Contributor

bilou84 commented Jul 2, 2016

So we are not deleting the scheduledCallback entry because it also store the last time it was saved.
But in the case of trashing the entry, I think it's fine to delete it.

Thanks for helping out with that!

@bilou84 bilou84 closed this Jul 2, 2016
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.

3 participants