-
Notifications
You must be signed in to change notification settings - Fork 2
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
Tweaks & fixes #52
Tweaks & fixes #52
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.
LGTM
Rebase against new sec fixes and this can be merged. |
Should be good now I think |
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.
@LukeWasTakenn, if this was tested in-game. All good to be merged.
Just one small thing that should eliminate an unnecessary call.
client/prompt.lua
Outdated
promptIsOpen = false | ||
cb({}) | ||
cb({ body = json.encode({}) }) |
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 I missed this the first time I reviewed
cb({ body = json.encode({}) }) | |
cb({ body = '{}' }) |
Should eliminate unnecessary encode call.
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.
Not sure why Dunak chose to use RegisterRawNuiCallback in the first place actually, The other week we found that unregistering the raw nui callback would for some reason a trigger completely unrelated event 🤷
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.
RegisterRawNuiCallback is being used in conjunction with UnregisterRawNuiCallback to eliminate possible memory leaky behavior in the underlying ResourceUI container.
You can see the following for more information.
citizenfx/fivem#1045
* Use the correct nui callback name when unregistering
Fixes nil user settings error caused by early usage of settings when not acquired from NUI.
I want to merge this to main so it makes pe-ui at least usable before any pending breaking commits.
Dependencies were upgraded too