Array of primitives: DynamicRealm support by cmelchior · Pull Request #5351 · realm/realm-java

cmelchior · 2017-09-29T06:59:22Z

Part of #5031
This PR adds support for primitive arrays in DynamicRealmObject

TODO

Cleanup
Fix RealmSchema unit tests so they use DynamicRealm instead
Determine naming. See Array of Primitives: RealmObjectSchema support #5329 . I already made several mistakes with the overloaded dynamicRealm.getList(), so I'm probably starting to lean towards having a seperate get<Primitive/Value>List() method instead ... People seem to like the overloaded methods best, so lets keep those.

…st-dynamicrealm

…st-dynamicrealm # Conflicts: # realm/realm-library/src/main/java/io/realm/DynamicRealmObject.java

…st-dynamicrealm

cmelchior · 2017-10-01T19:05:12Z

Ready for review @realm/java

Zhuinden · 2017-10-01T20:14:02Z

        }
+        final ManagedListOperator<?> operator = getOperator(proxyState.getRealm$realm(), osList, elementType, elementClass);
+
+        if (list.isManaged() && osList.size() == list.size()) {


When does this happen exactly? Is there a test for it? Is osList.size == list.size sufficient to determine that this is the case ❓

No, but if the size() is different we know with 100% that the lists are not the same, in which case we can use clear() + insertAll() which is much faster than manually replacing each element in the the list.

If they are the same size() we don't know for sure, so we have to revert to manually replacing each element.

beeender · 2017-10-02T10:45:56Z

    /**
     * Returns the {@link RealmList} of {@link DynamicRealmObject}s being linked from the given field.
+     * <p>
+     * If the list only contain primitive types, use {@link #getList(String, Class)} instead.


contain -> contains

beeender · 2017-10-02T10:48:26Z

+        schema.setRequired("foo", false);
+        list = obj.getList("foo", byte[].class);
+        assertEquals(0, list.get(0).length);
+        assertArrayEquals(new byte[] {1, 2, 3}, list.get(1));


Do you want to call

// Cleanup schema.removeField("foo");

here as well?

It is actually no need, the tearDown will do that. So https://github.com/realm/realm-java/pull/5351/files#diff-4767794a7ded2dc2e3cc9447650ac1c7R838 can be removed as well.

Zhuinden · 2017-10-02T13:06:19Z

-    public <T> RealmList<T> getValueList(String fieldName, Class<T> valueClass) {
-        // TODO implement this
-        return null;
+    public <E> RealmList<E> getList(String fieldName, Class<E> elementType) {


I think #5329 (comment) applies here as well (that primitiveType is a better name)

Why primitive is better? It's not a primitive type at least in Java.

Technically because the feature is called "array of primitives".

The term element does not portray that this is not meant to be used for explicit RealmModel classes.

Originally I would have gone with the terminology "value list" but it wouldn't make sense for end users who don't read the commits 😮

"array of primitives" is just a internal feature name and I don't think we are using it in our documentation /API.

The problem is that no terminology is 100% correct without also being too vague:

elementType: Correct, but doesn't describe the difference between the RealmModel variant and this very well.
valueType: Date and byte[] isn't value types. Nor may others be in the future
primitiveType Date/byte[] isn't primitive types. Technically neither are any of the boxed types.

valueType is probably the one with the least amount of "wrongness", but IMO it is also a pretty specialized concept which I'm guessing not a lot know about. Primitive types is hopefylly someone even starting out with Java knows about.

Hence primitiveType

We don't need to use the same word with core.

You don't need to, but I think the other alternatives allow for more confusion.

In fact, value type seems to mean something completely different...

We don't need to use the same word with core.The most important thing is what is the most natural for Java developers.

True, but what is that? :)

Especially, if there isn't a clear benefit_ to choosing another word, we should probably go with what is otherwise used in Core/Bindings/Issues.

@cmelchior All candidates don't describe the difference between the RealmModel variant and expected ones well.
I could't find the reason to choose more incorrect one...

But I can accept your decision.

I think it ended up staying elementType.

nhachicha

small comment

nhachicha · 2017-10-02T21:26:46Z

                        break;
+                    case LIST_INTEGER:
+                        try {
+                            dObj.setNull(NullTypes.FIELD_INTEGER_LIST_NULL);


why should this fail? FIELD_INTEGER_LIST_NULL is supporting null value

Yes, but this is setting the field itself to null, not the values in the list. The field itself isn't allowed to be null.

Add DynamicRealm support for Array of primitives.

1d12cc8

realm-ci assigned cmelchior Sep 29, 2017

cmelchior mentioned this pull request Oct 1, 2017

Primitive List feature(Part12: DynamicRealm) #5221

Closed

cmelchior added 6 commits October 1, 2017 12:26

Merge branch 'cm/cm/primitivelist-schema-support' into cm/primitiveli…

eaad966

…st-dynamicrealm

Merge branch 'cm/cm/primitivelist-schema-support' into cm/primitiveli…

c2fb9f2

…st-dynamicrealm

Check list value conversion

d44f8aa

Merge branch 'cm/cm/primitivelist-schema-support' into cm/primitiveli…

23a97c0

…st-dynamicrealm # Conflicts: # realm/realm-library/src/main/java/io/realm/DynamicRealmObject.java

Cleanup unit tests

462dca9

Merge branch 'cm/cm/primitivelist-schema-support' into cm/primitiveli…

89ff973

…st-dynamicrealm

Fix merge mistake + add test for setting objects own list

3310af0

Zhuinden reviewed Oct 1, 2017

View reviewed changes

cmelchior added 3 commits October 2, 2017 10:52

Fix binary data not being transformed correctly

32101f9

Update RealmObjectSchemaTests.java

7712f3b

Remove duplicated test

1242a56

beeender approved these changes Oct 2, 2017

View reviewed changes

cmelchior mentioned this pull request Oct 2, 2017

Array of Primitives: RealmObjectSchema support #5329

Merged

4 tasks

Fix language

a00f9c2

Zhuinden reviewed Oct 2, 2017

View reviewed changes

nhachicha approved these changes Oct 2, 2017

View reviewed changes

cmelchior merged commit 3fa8e2e into cm/cm/primitivelist-schema-support Oct 3, 2017

cmelchior deleted the cm/primitivelist-dynamicrealm branch October 3, 2017 08:49

github-actions Bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Conversation

cmelchior commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmelchior commented Oct 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zhuinden Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaki50 Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zhuinden Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhachicha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cmelchior commented Sep 29, 2017 •

edited

Loading

Zhuinden Oct 2, 2017 •

edited

Loading

zaki50 Oct 2, 2017 •

edited

Loading

Zhuinden Oct 2, 2017 •

edited

Loading