Created attachment 180495 [details] [review]
Do not bind gobject_get_data() and gobject_set_data()

They will basically cause a crash if misused, and you can always use a
python member attribute instead.

Do you guys have any opinion on this change for pygobject 3?

You need to set data when creating your own container but it is a void pointer so the python pointer gets set instead of the wrapped C pointer.  I agree it is useless in that case.

This makes sense to me, much better to use Python attributes. Now, right after a stable release, is a good time to push changes like these.

I updated your patch to apply to current master, and pushed. Thanks!

While I agree that using attributes is almost always a better idea than set/get_data there should at least be an opportunity for end users to migrate their apis first, with warnings. 

The implementation could perhaps be simplified to do a simple 

def set_data(self, key, value):
  self._pygobject_data[key] = value.

def get_data(self, key):
  return self._pygobject_data.get(key)

Could even be done in python, as the metaclass is pure-python code.

I'm reopening this.

(In reply to comment #5)
> 
> Could even be done in python, as the metaclass is pure-python code.

That's a great idea, specially if we raise a DeprecationWarning.

Or an even more evil variant;

    set_data = lambda *args : setattr(*args)
    get_data = lambda *args : getattr(*args)

Created attachment 214011 [details] [review]
(broken) attempt to add them back in the metaclass

Johan, can you please give me a hint what you mean with "do this in the metaclass"? I assumed you meant something like the attached patch attempt, but that doesn't work.

I thought impl, it in the metaclass itself would work?

(In reply to comment #9)
> I thought impl, it in the metaclass itself would work?

No, it seems GObjectMeta is not being called for GObject.GObject. Adding a print() to its constructor shows:

('GObjectMeta init', <class 'gi.repository.GObject.InitiallyUnowned'>, 'InitiallyUnowned')
('GObjectMeta init', <class 'test_gobject.A'>, 'A')
('GObjectMeta init', <class 'test_gobject.ContextTestObject'>, 'ContextTestObject')
('GObjectMeta init', <class 'test_gobject.TestObject'>, 'TestObject')
testGObjectModule (test_gobject.TestGObjectAPI) ... ok
testGetSetData (test_gobject.TestGObjectAPI) ... ERROR

======================================================================
ERROR: testGetSetData (test_gobject.TestGObjectAPI)
----------------------------------------------------------------------
Traceback (most recent call last):

+ Trace 230221

----------------------------------------------------------------------

I don't yet know how the metaclass works, doing some RTFS now..

Okay, GObject.Object it not using that metaclass, but gobject.GObject is, go figure. Perhaps we need an override here.

This broke Ubuntu's update-manager application, which was using get_data/set_data for bits and pieces of internal communication.  Nothing very important, and easy to fix, but some kind of deprecation cycle would have been nice.

As it's inordinately complicated to provide a python backwards compat shim, I reverted the original patch and added a deprecation warning for now:

$ jhbuild run python3 -c 'from gi.repository import GObject; o = GObject.Object(); o.set_data("foo", 1); print(o.get_data("foo"))'

** (process:28774): WARNING **: GObject.get_data() and set_data() are deprecated. Use normal Python attributes instead

** (process:28774): WARNING **: GObject.get_data() and set_data() are deprecated. Use normal Python attributes instead

http://git.gnome.org/browse/pygobject/commit/?id=73531fd7820bd1922347bd856298d68205a27877

We should reapply this in GNOME 3.8 then, when application authors have ported their stuff.

Patch status has been "accepted-commit_after_freeze" for 18 months now.
Which freeze does this exactly refer to? Any patch status update to set?

Andre, actually I only flipped it back to "accepted-commit-after-freeze" two days ago. This patch is targetted at GNOME 3.8, but that milestone does not exist in bugzilla yet. This was the next best thing as a "patch for next cycle" queue. But if it's confusing for the release team, I'll flip it back to "reviewed".

Comment on attachment 180495 [details] [review]
Do not bind gobject_get_data() and gobject_set_data()

(In reply to comment #15)
> Andre, actually I only flipped it back to "accepted-commit-after-freeze" two
> days ago. This patch is targetted at GNOME 3.8, but that milestone does not
> exist in bugzilla yet.

Well, there are no Milestones *at all* for pygobject it seems. :P

I see. I think it's quite appropriate then. :)

Ah, indeed. I added a "GNOME 3.8" milestone now, and target this bug to it.

Comment on attachment 180495 [details] [review]
Do not bind gobject_get_data() and gobject_set_data()

Now that we opened the 3.7.x cycle, I committed this, together with dropping the corresponding documentation.

Thanks!