Skip to content
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

Let Crystal user objects be created from g_gobject_new calls. #153

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hugopl
Copy link
Owner

@hugopl hugopl commented Jun 24, 2024

This is a breaking change, since it now requires all GObject subclasses to have a constructor without arguments.

What changed:

There are 2 thread local variables to inform if the object is being created in C land or Crystal land, we store the objects instance pointers there.

When the object is created in C land, the Crystal instance is created on GObject instance_init method.

When the object is created in Crystal land, the GObject instance_init method creates no Crystal instance.

This also fixes the use case of tryign to use a Crystal defined GObject property before the Wrapper gets fully initialized.

Fixes hugopl/gtk4.cr#69

@hugopl
Copy link
Owner Author

hugopl commented Jun 24, 2024

Weird... the tests don't fail on me locally.

@hugopl hugopl force-pushed the c-born-crystal-types branch 6 times, most recently from 14b81ef to 1457573 Compare June 24, 2024 18:47
@hugopl
Copy link
Owner Author

hugopl commented Jun 24, 2024

Tests now pass... but I noticed something that may be a compiler bug, if I change the class on c_born_crystal_objects_spec.cr to

private class UserObj < GObject::Object
  @[GObject::Property]
  property crystal_prop = ""
  getter crystal_attr : Int32 = 42

  def initialize(@hey = "")
    super()
  end
end

It doesn't initialize crystal_attr to 42 and the test fail.

@hugopl
Copy link
Owner Author

hugopl commented Jun 24, 2024

@BlobCodes , I remember you wanted this feature years ago, and it was only possible due to your work on toggle refs. If you have time, at your own pace, could you take a look on this and give me some feedback? Thanks!

src/bindings/g_object/object.cr Outdated Show resolved Hide resolved
src/bindings/g_object/object.cr Outdated Show resolved Hide resolved
src/bindings/g_object/object.cr Show resolved Hide resolved
@hugopl
Copy link
Owner Author

hugopl commented Jun 26, 2024

This MR fails to compile with abstract classes, mainly because GICrystal doesn't set any flags in the generated C Type if the Crystal class is abstract.

@hugopl hugopl force-pushed the c-born-crystal-types branch 3 times, most recently from daf25a0 to 3c4ebca Compare June 28, 2024 15:01
This is a breaking change, since it now requires all GObject subclasses
to have a constructor without arguments.

What changed:

There are 2 thread local variables to inform if the object is being created
in C land or Crystal land, we store the objects instance pointers there.

When the object is created in C land, the Crystal instance is created on
GObject instance_init method.

When the object is created in Crystal land, the GObject instance_init method
creates no Crystal instance.

This also fixes the use case of tryign to use a Crystal defined GObject property
before the Wrapper gets fully initialized.

Fixes hugopl/gtk4.cr#69
@hugopl
Copy link
Owner Author

hugopl commented Jun 28, 2024

Hmmm, CI was failing with old GObject libs, after updating the CI things start passing.

With the current patch things works as expected, but there's the breaking change that requires Crystal GObjects to define initialize(), after few minutes porting Tijolo to use this patch I saw how annoying is this, so I'll try to avoid this breaking change, i.e. If the type doesn't have a default constructor it cannot be created from C.

The challenge will be able to provide a good compiler error message in these cases.

If the object doens't have a default constructor and someone tries to
create it from C, a GLib fatal error is issued.
@hugopl
Copy link
Owner Author

hugopl commented Jun 28, 2024

I believe now it's ready to be merged. And it's no more a breaking change, since it doesn't require the Crystal object to have a default constructor. However I'll only merge this after I start using Tijolo with this for few days.

@BlobCodes
Copy link
Contributor

@BlobCodes , I remember you wanted this feature years ago, and it was only possible due to your work on toggle refs. If you have time, at your own pace, could you take a look on this and give me some feedback? Thanks!

It seems like you are initializing the entire object inside the _instance_init hook, which has a few disadvantages.

There are multiple steps to the initialization of a GObject:

  • _instance_init hook is called bottom-up (GObject.init() -> Gtk::Widget.init() -> MyObject.init())
    This hook is supposed to initialize any memory needed during the construction process
  • All GObject properties marked with CONSTRUCT or CONSTRUCT_ONLY flags are applied one-by-one
  • vfunc constructed is called, chained (MyObject.constructed() is supposed to call Gtk::Widget.constructed(), which is ...)
    Objects are supposed to do more advanced logic here.
  • Regular GObject properties are applied one-by-one

For example, if our GObject is a templated Gtk::Widget, the template is initialized during Gtk::Widgets constructed vfunc.
This means that there is currently a big behavioral difference between objects created from C or from Crystal:
Objects created from Crystal already have their template initialized, while Objects created from C aren't allowed to query for template objects yet.


I think we should do the following to better match GObjects behaviour:

  • Create a protected def initialize in GObject, which calls the previous constructed vfunc
    This would allow a super() call to initialize the inherited GObject, matching normal crystal behaviour
  • Automatically map any initialize params to CONSTRUCT_ONLY GObject parameters
  • Only initialize memory of crystal object in _instance_init, call initialize inside vfunc constructed
  • Always create crystal GObjects through C, instead of having two separate flows

@@ -119,6 +127,32 @@ module GICrystal
end
end

# When creating an user defined GObject from C, the GObject instance is stored here, so the Crystal
# constructor uses it instead of call `g_object_new`
@[ThreadLocal]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could cause the application to crash as ThreadLocal values aren't tracked by the GC

Also, having a global value for constructing objects seems very hacky.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is very hacky :-P.

BTW I didn't know that about thread local variables in Crystal, thanks.

@[ThreadLocal]
class_property g_object_being_created : Pointer(Void) = Pointer(Void).null

@[ThreadLocal]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


# When creating an user defined GObject from Crystal, the Crystal instance is stored here, so the
# GObject `instance_init` doesn't instantiate another Crystal object.
@[ThreadLocal]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@hugopl
Copy link
Owner Author

hugopl commented Jun 30, 2024

When I started with the idea of always initialize the Crystal instance in the instance_init, but I guess I was not able to make custom constructors work, now I see that I can just use the method_added macro we already have and add a self.new everytime I see a def initialize.

I felt something was wrong and I was unsure of how many time my code was setting the qdata depending on the 3 use cases of object construction.

  • Foo.new
  • Foo.new(prop: value)
  • Lib.g_object_new

Because I forgot that I would write a new method, I had only the self.new() available, so the use case Foo.new(prop: value) always have the Crystal instance first... anyway, writing a new method for each initialize found on that macro will reduce this to the base 1 use case.

Thanks for the feed back! I'll re-work on this patch and ping you when there's something worth to double check.

@hugopl hugopl marked this pull request as draft June 30, 2024 00:24
@hugopl
Copy link
Owner Author

hugopl commented Jul 11, 2024

I think we should do the following to better match GObjects behaviour:

  • Create a protected def initialize in GObject, which calls the previous constructed vfunc
    This would allow a super() call to initialize the inherited GObject, matching normal crystal behaviour
  • Automatically map any initialize params to CONSTRUCT_ONLY GObject parameters

I don't like this, IMO it's ok to have constructor params that has nothing to do with GObject properties. I also don't think is useful to support construct only Crystal gobject properties, but I may be wrong.

  • Only initialize memory of crystal object in _instance_init, call initialize inside vfunc constructed

This is the problem, I want to allow the user to write any possible constructor, so on constructed vfunc I would need to known what Crystal constructor was called, so I could do the correct initialize call. IIRC this is why the Crystal instance is created in more than one place, something I agree is bad... maybe I can fix that creating closures that call the correct initialize func, but it would involve the use of hacky thread local variables again.

  • Always create crystal GObjects through C, instead of having two separate flows

Maybe my requirements for the desired API are too high.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GICrystal::ObjectCollectedError when trying to bind to a property from a UiTemplate
2 participants