-
-
Notifications
You must be signed in to change notification settings - Fork 414
Ensure that unload event is called before unloading of functions regardless of order in script. #8250
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
base: dev/patch
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than hardcoding this comparator in, I think an alternative option would be to unload based on Structure priority (in reverse order to loading)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this won't work if the function is in a different script from the unload event structure. This needs to be reworked so that all structures across all unloading scripts are sorted together (similar to how it is done in the loading phase).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is almost ready, great work!
|
|
||
| } | ||
|
|
||
| public record LoadingStructure (LoadingScriptInfo loadingScriptInfo, Structure structure) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'd be fine to define this within the method itself since it isn't used elsewhere
| * Script Unloading Methods | ||
| */ | ||
|
|
||
| public record UnloadingStructure (Script script, Structure structure) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. This can be defined in the method.
| Comparator<UnloadingStructure> unloadComparator = Comparator.comparing(unloadingStructure -> unloadingStructure.structure().getPriority()); | ||
| unloadComparator = unloadComparator.reversed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only used once in the stream operations, I don't think it needs to be separately declared anymore.
| for (Structure structure : structures) | ||
| structure.postUnload(); | ||
| structure.postUnload(); | ||
| parser.setInactive(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved right after the loop so that it is only called once.

Problem
functions appearing before on unload event is unloaded before event is called.
Solution
When unloading a script unload event is moved to front of list so that it is triggered before unloading of functions.
Testing Completed
I have tested it on version 1.21.10.
Supporting Information
Completes: #8233
Related: none