-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support subclassing RLMObjects #940
Conversation
@@ -165,6 +165,13 @@ static inline void RLMSetValue(__unsafe_unretained RLMObject *obj, NSUInteger co | |||
obj->_row.nullify_link(colIndex); | |||
} | |||
else { | |||
// make sure it is the correct type | |||
if (![[obj.objectSchema.properties[colIndex] objectClassName] isEqualToString:val.objectSchema.className]) { |
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 looks kinda slow, but I guess arrays do the same thing and it's not a major slowdown there. Would be nice to find a nicer solution for checking the type (maybe store the Class
in the object schema and just check pointer equality?).
[Ari replied with this, which I accidentally deleted when I just wanted to add my own comment. Sorry!]
Checking the class doesn't work as we have accessor classes. After an object is added to a realm, we could check that the target table index is equal to the table index of the object being inserted. That would avoid objc but would require a few method calls.
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.
Checking for either the accessor or standalone class would probably work.
This isn't something important; it's just something that bugs me every time I see it.
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.
I agree with you it would be nice to improve this. Another thing to consider is for the dynamic interface where we may have vanilla RLMObjects, so I don't think class comparison will work. The only non-string comparison I can think of is comparing table indexes.
Another option is to use uniqued strings for classnames, so that we can compare classname strings using pointer equality. This would require a lot of code change but could be worth it if this is something we have to do all over the place.
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.
It would be really useful to get the dynamic interface working soon. Can I ask the reason why we want vanilla RLMObjects there? I understood that it had something to do with migrations but I can't really see why it would be needed there either.
I assume it'll just (not) work without changes, but it'd be nice to have a test for trying to add a subclass to a RLMArray. |
@@ -5,6 +5,9 @@ | |||
|
|||
### Enhancements | |||
|
|||
* Support subclassing RLMObject models. Although you can now use subclasses | |||
as you an other objects, polymorphic behavior is not supported (i.e. setting |
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.
*can
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.
Does this mean the following isn't supported?
class Parent: RLMObject {
dynamic var child = Child()
}
class Child: Parent {
dynamic var name = ""
}
This is setting a property (child
) to an instance of a subclass (Child
). It seems like an arbitrary limitation and am curious as to why that's not supported.
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.
You can do that. If you define a Parent property, you can't set it to a Child instance.
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.
Parent
and Child
are entirely separate tables, so child
would have to be a mixed property.
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.
Ok, I was confused by the wording. I'll try to think of a way to clarify.
@tgoyne we do try to add a subclass to an array here: https://github.com/realm/realm-cocoa/pull/940/files#diff-41cbd0b877f90f73a468d53cda02df13R461 It throws when the child type doesn't match the array type exactly |
|
||
[realm commitWriteTransaction]; | ||
|
||
// ensuere creation in proper table |
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.
*ensure
Ah, I missed that. I was going to complain about the lack of docs changes but apparently we never actually mentioned that subclassing wasn't allowed in the header docs. The guide needs to be updated, of course. 👍 |
@@ -5,6 +5,9 @@ | |||
|
|||
### Enhancements | |||
|
|||
* Support subclassing RLMObject models. Although you can now persist subclasses | |||
polymorphic behavior is not supported (i.e. setting a property to an | |||
instance of a subclass). |
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.
"instance of its subclass" makes it a bit clearer that we're not talking about subclasses in general, just a subclass of the property type.
Support subclassing RLMObjects
Thank you! 👍 👍 👍 |
Does not support polymorphic behavior.
@tgoyne