fix: Combobox ConVar change callback deletion #1075
Conversation
Fixes #1068 We need to remove the callback in a timer, because otherwise the ConVar change callback code will throw an error while looping over the callbacks. This happens, because the callback is removed from the same table that is iterated over. Thus, the table size changes while iterating over it and leads to a nil callback as the last entry. See https://github.com/Facepunch/garrysmod/blob/1b512930d1f8fb1acf6235e584c4ec1ff84e9362/garrysmod/lua/includes/modules/cvars.lua#L44 and https://github.com/Facepunch/garrysmod/blob/1b512930d1f8fb1acf6235e584c4ec1ff84e9362/garrysmod/lua/includes/modules/cvars.lua#L97 here the code uses table.Remove, which causes the table to be reindexed and resized, this is the root cause for the problem. We should thus avoid removing callbacks in the callback itself.
|
Interesting and makes sense. |
|
Too late tim was here already |
well it is already merged :D |
|
I would still like to comment on this: I did think about using a timer, but I decided against it as I though 0 tick timers are an "ugly" solution. The "oldValue" was merely a nice to have in my PR I guess as you may want the previous value directly from the combobox and not have to get the conVar first, which is more CPU intensive as stated in the wiki. |
|
Yes, this is only a fix for the callbacks themselves. But please open up a new PR for the language fix, if you can spare the time! Id love to have that one in too. |
|
Reagding the uglyness: In my opinion are 0 second timer fine, they are a solid way to make sure something is executed in the next tick |
I give you that the concept is probably flawed, yes. Maybe it's just my concept of "ugly" that makes it weird in my mind. If I get to fix the lang thingy: Should I maybe still add the "oldValue" too? This still gives advantages I think. |
|
What advantage exactly? I did not really see any need for it, but maybe i just did not get it correctly. |
Maybe I'm just dumb, but I did not see another way of getting the old value except for reading out the conVar value itself. Which would be with "GetConVar". The GMOD-Wiki to that states: I mean in daily use this would probably not make that much of a difference, but IMO giving back the "oldValue" in the function itself is cleaner then needing to get it from the convar. |
|
The wiki's comment is out of date, in the code itself on the FP repo you can see it is caching the reference to the convar |
|
"Performance" is super negligible here as its not called often, but sure go ahead and fix the language percentage. |
Fixes #1068
We need to remove the callback in a timer, because otherwise the ConVar change callback code
will throw an error while looping over the callbacks.
This happens, because the callback is removed from the same table that is iterated over.
Thus, the table size changes while iterating over it and leads to a nil callback as the last entry.
See https://github.com/Facepunch/garrysmod/blob/1b512930d1f8fb1acf6235e584c4ec1ff84e9362/garrysmod/lua/includes/modules/cvars.lua#L44
and
https://github.com/Facepunch/garrysmod/blob/1b512930d1f8fb1acf6235e584c4ec1ff84e9362/garrysmod/lua/includes/modules/cvars.lua#L97
here the code uses table.Remove, which causes the table to be reindexed and resized, this is the root cause for the problem.
We should thus avoid removing callbacks in the callback itself.