-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Conversation
Weird... the tests don't fail on me locally. |
14b81ef
to
1457573
Compare
Tests now pass... but I noticed something that may be a compiler bug, if I change the class on 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. |
@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! |
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. |
daf25a0
to
3c4ebca
Compare
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
3c4ebca
to
de6a787
Compare
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 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.
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. |
It seems like you are initializing the entire object inside the There are multiple steps to the initialization of a GObject:
For example, if our GObject is a templated I think we should do the following to better match
|
@@ -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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
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 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.
Because I forgot that I would write a new method, I had only the Thanks for the feed back! I'll re-work on this patch and ping you when there's something worth to double check. |
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.
This is the problem, I want to allow the user to write any possible constructor, so on
Maybe my requirements for the desired API are too high. |
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