GNOME Bugzilla – Bug 702390
NotifyNotification: fix annotation for add_action()
Last modified: 2013-10-09 08:28:07 UTC
Using GFreeFunc instead of GDestroyNotify was a mistake, but (scope async) is even a bigger mistake (one that can crash apps, if an action is invoked multiple times)
Created attachment 246943 [details] [review] NotifyNotification: fix annotation for add_action()
Review of attachment 246943 [details] [review]: This looks right.
Attachment 246943 [details] pushed as 2b4ab4d - NotifyNotification: fix annotation for add_action()
Why did that drop the (allow-none) from user_data? That looks unrelated, and is an API/ABI change.
(In reply to comment #4) > Why did that drop the (allow-none) from user_data? That looks unrelated, and is > an API/ABI change. Not really. user_data is always virtually allow-none, in that every binding implements it in a different way, and it doesn't map directly to C. It was dropped because it didn't make sense there.
Who is right here? Now this is an incompatibility between Ubuntu (which reverted this patch) and $OTHERS https://bugs.launchpad.net/ubuntu/+source/libnotify/+bug/1223401 https://bugzilla.gnome.org/show_bug.cgi?id=709597#c8
(In reply to comment #6) > Who is right here? > > Now this is an incompatibility between Ubuntu (which reverted this patch) and > $OTHERS > > https://bugs.launchpad.net/ubuntu/+source/libnotify/+bug/1223401 > https://bugzilla.gnome.org/show_bug.cgi?id=709597#c8 I'm sorry, but the change is right for libnotify. Anything else is a misunderstanding of how gobject-introspection works and a memory hazard. If pygobject wants to keep the old signature for compatibility, it should use an override. But IMHO, the fact that the old function (which would leak memory or crash the application!) was available is a pygobject bug, and the fact that user_data allow-none has any influence is another pygobject bug.
The change is "right", it just makes all clients using the api stop working ... incompatible changes are no-win for everyone. You get some apps ported and that are working with the new lib and buggy with the old and some other in the reverse situation. Rather than changing the api, what about deprecating this one and adding a fixed api with a different name, this way you keep old clients working and those who want to benefit from the fix can just port to the new name...
Again, this is not an incompatible change, this is a bugfix. That it was "working" before is the bug. Pygobject is not the only user of introspection, and we should not keep broken APIs for one user.
(In reply to comment #9) > Again, this is not an incompatible change, this is a bugfix. That it was > "working" before is the bug. > > Pygobject is not the only user of introspection, and we should not keep broken > APIs for one user. remember when it was, and we wouldn't break its API?
> this is not an incompatible change sorry, but what is an incompatible change, in your opinion, then? The reason we reverted the commit in Ubuntu is that the number of arguments changing for python client and would let to working got to bail out on an "invalid number or argument error"...
> The reason we reverted the commit in Ubuntu I thought that would be only a temporary workaround until we port all the consumers? We certainly shouldn't release with that, as it would cement an API incompatibility with Ubuntu and everything else, which would be even worse than the API change introduced here.
> I thought that would be only a temporary workaround until we port all the consumers? Right, we should still take the hit and do a transition (I'm thinking changing the binary name of the package in Ubuntu to do a proper transition), it just didn't happen this cycle > We certainly shouldn't release with that, as it would cement an API > incompatibility with Ubuntu and everything else, which would be even worse than > the API change introduced here. We have been releasing with that API for cycles, it's already "cemented" ... it's basically the same effect that if we didn't take on the new version this cycle (which we did after ffe)...