search

romgrk / node-gtk

GTK+ bindings for NodeJS (via GObject introspection)
MIT License
494 stars 42 forks source link

fix(value): handle transfer-full, in arguments properly #303

Open peat-psuwit opened 3 years ago

peat-psuwit commented 3 years ago

This is done by making V8ToGIArgument() accept the transfer status and react accordingly. I does not make the new argument optional; this forces me into thinking about the appropriate value to put in for various callsites of this function.

Closes #303

romgrk commented 3 years ago

@peat-psuwit sorry for the delay, was traveling a bit this summer.

Overall this looks good, one thing though. For all the changes it makes, the PR only has a single test. Is it really the only case affected by these changes?

peat-psuwit commented 3 years ago

For all the changes it makes, the PR only has a single test. Is it really the only case affected by these changes?

Well, the thing is that I can't find more APIs in GStreamer, GLib or GTK that would be affected by this. I would prefer to have at least an equivalent test but for GObject. I considered adding an introspectable library for testing, but I'm not sure if it can be done using node-gyp system

romgrk commented 3 years ago

Alright. There is #269 open to add libgimarshallingtest which does that kind of stuff, we can add more tests once the lib is used for testing. I'll merge as soon as you fix the last two nitpicks :]

romgrk commented 3 years ago

How is this one related to #302? Do you want to include the memory ownership thing here?

peat-psuwit commented 3 years ago

It's a different problem. This one try to prevent use-after-free for transfer-full, in argument, while #302 concerns about memory leakage. Both are memory problems, but of different kinds...

This one can be merged without the fix for #302.

romgrk commented 3 years ago

Got it. You can fix the last comment up here and it's ready for merging.