add RealmCursor #53

larsgrefer · 2016-06-16T19:57:39Z

fix #6

is it correct to open pull requests against the master branch, or should I PR against the releases branch instead?

dalinaum · 2016-06-17T10:33:33Z

adapters/src/main/java/io/realm/RealmCursor.java

@@ -0,0 +1,601 @@
+package io.realm;


Missing license header

cmelchior · 2016-06-22T09:35:29Z

Hi @larsgrefer
Sorry for being late getting to this. Thanks a lot for this PR, but for issues like this that are kinda large in scope, it might have been better if you reached out before doing the work since there is a number of issues with the code that could have been addressed before you did the work 😢

A few general comments:

Any reason you implemented the Cursor from scratch? Work was already begun here: https://github.com/realm/realm-java/pull/1090/files and it would probably have been easier to start from there. Especially you are missing tests for everything.
Have you checked that {@inheritDoc} is actually working when inheriting from Android classes, we had problems with this before.
Mising javadoc for the class header. Cursors based on a RealmResults has a few differences that we should very clearly document for now. 1) It doesn't work across processes (=cannot be used in ContentProviders shared between apps) 2) Following links is problematic as normal cursors doesn't allow that 3) It doesn't work across threads.

If you want to move forward with this PR there is a number of things that needs to be discussed/clarified first:

I'm fine with leaving out interprocess support for now. It will be extremely complicated to get right.
We need to figure out how linked objects should be supported if at all. See the description in RealmCursor realm-java#1090. Any comments you have would be helpful.
We need to figure out if we should disable auto-update for Cursors. I have a feeling that auto-update will work really poorly with how Cursors are currently used in Android. Comments are welcome as well.

First steps should as a minimum be porting all the unit tests over.

/cc @realm/java

kneth · 2016-06-22T13:31:35Z

adapters/src/main/java/io/realm/RealmCursor.java

+     */
+    @Override
+    public int getColumnIndexOrThrow(@NonNull String columnName) throws IllegalArgumentException {
+        if (data == null)


Please use curly brackets

larsgrefer · 2016-06-24T19:01:59Z

@cmelchior

Any reason you implemented the Cursor from scratch?

I've seen this too late

Have you checked that {@inheritDoc} is actually working when inheriting from Android classes, we had problems with this before.

What kind of problems do you mean?
I've updated the javadoc options in order to link against https://developer.android.com/reference, but their server sends a X-Frame-Options: "SAMEORIGIN" header

I'm fine with leaving out interprocess support for now. It will be extremely complicated to get right.

👍

We need to figure out how linked objects should be supported if at all.

I think this should not be supported.
Any existing code, which expects a Cursor, should only expect simple tabular data.

First steps should as a minimum be porting all the unit tests over.

I will do this.

kneth · 2016-06-27T06:13:25Z

adapters/src/main/java/io/realm/RealmCursor.java

-                if (aLong >= Integer.MAX_VALUE)
-                    return Integer.MAX_VALUE;
-                return (int) aLong;
+                if (aLong > Integer.MIN_VALUE && aLong < Integer.MAX_VALUE)


Is it acceptable that if the value is out of range, nothing (undefined) is returned?

I think this code path will throw with canNotConvert() since there is no break.

Edit> You are right. This can be re-written for clarification.

True. But do we wish the same message for the two cases: out of range, and wrong type?

That's good point. I think this is updated and you'd perhaps want to comment on new commits.

cmelchior · 2016-06-29T10:50:55Z

adapters/src/androidTest/java/io/realm/RealmCursorTest.java

+
+import io.realm.entity.AllJavaTypes;
+
+public class RealmCursorTest extends AndroidTestCase {


All of these should be converted to JUnit4 since that is what we are writing all new tests with. This also include removing the test prefix. See https://github.com/realm/realm-java/blob/master/CONTRIBUTING.md#unit-tests

cmelchior · 2016-06-29T11:04:19Z

Our original implementation also had the concept of setting an "_id" column, which is required for a number of Android framework classes. I would like for this functionality to be present as well.

Unit tests are still failing btw:
Test Result (2 failures / +2)
io.realm.RealmCursorTest.testRegisterContentObserverRealmChanged
io.realm.RealmCursorTest.testUnregisterContentObserver

larsgrefer · 2016-06-29T13:46:23Z

Unit tests are still failing btw:
Test Result (2 failures / +2)
io.realm.RealmCursorTest.testRegisterContentObserverRealmChanged
io.realm.RealmCursorTest.testUnregisterContentObserver

The tests are not running in Looper-Threads so they can't add ChangeListeners

beeender · 2016-07-01T05:50:03Z

Testing failed with below information

testStarted io.realm.RealmCursorTest#registerDataSetObserver_closed
testFailed FAILURE/java.lang.IllegalStateException: You can't register a listener from a non-Looper thread 
    at io.realm.RealmResults.addChangeListener(RealmResults.java:929)
    at io.realm.RealmCursor.registerDataSetObserver(RealmCursor.java:680)
    at io.realm.RealmCursorTest.registerDataSetObserver_closed(RealmCursorTest.java:489)
    at java.lang.reflect.Method.invoke(Native Method)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:27)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
    at android.support.test.internal.runner.TestExecutor.execute(TestExecutor.java:54)
    at android.support.test.runner.AndroidJUnitRunner.onStart(AndroidJUnitRunner.java:240)
    at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1879)

testEnded io.realm.RealmCursorTest#registerDataSetObserver_closed
testStarted io.realm.RealmCursorTest#registerContentObserver_realmChanged
testFailed FAILURE/java.lang.IllegalStateException: You can't register a listener from a non-Looper thread 
    at io.realm.RealmResults.addChangeListener(RealmResults.java:929)
    at io.realm.RealmCursor.registerContentObserver(RealmCursor.java:648)
    at io.realm.RealmCursorTest.registerContentObserver_realmChanged(RealmCursorTest.java:504)
    at java.lang.reflect.Method.invoke(Native Method)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:27)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
    at android.support.test.internal.runner.TestExecutor.execute(TestExecutor.java:54)
    at android.support.test.runner.AndroidJUnitRunner.onStart(AndroidJUnitRunner.java:240)
    at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1879)

larsgrefer · 2016-07-01T19:12:15Z

@beeender same reason as in #53 (comment)

to solve this, I would need these classes from realm-java:
https://github.com/realm/realm-java/tree/master/realm/realm-library/src/androidTest/java/io/realm/rule

Shall I copy the classes or do you want to build a new artifact which contains them?

cmelchior · 2016-07-05T07:39:27Z

@larsgrefer Copying the rules is fine with me.

into cursor

dalinaum reviewed Jun 17, 2016
View reviewed changes

larsgrefer and others added 2 commits June 17, 2016 18:09

add RealmCursor

b3d0001

add header

1c2c216

larsgrefer force-pushed the cursor branch from 49d3d41 to 1c2c216 Compare June 17, 2016 16:10

cmelchior added S:Review T-Feature labels Jun 22, 2016

kneth reviewed Jun 22, 2016
View reviewed changes

larsgrefer added 3 commits June 24, 2016 20:03

adjust conversion

aa8b088

add curly brackets

e249335

improve javadoc

3c7e569

add Tests

add4130

kneth reviewed Jun 27, 2016
View reviewed changes

fix switches

524bb65

cmelchior reviewed Jun 29, 2016
View reviewed changes

larsgrefer added 2 commits July 1, 2016 00:21

make data unchangeable

31d802e

convert tests to junit4

3ec6dd7

add a bit javadoc

6c08738

use the realm schema to resolve column names and indices

7c3c5a4

Merge remote-tracking branch 'upstream/master' into cursor

443216e

cmelchior mentioned this pull request Jul 8, 2016

RealmCursor realm/realm-java#1090

Closed

larsgrefer added 2 commits October 3, 2016 01:46

Merge branch 'master' of https://github.com/realm/realm-android-adapters

a368904

into cursor

run tests on uiThread

268448b

larsgrefer force-pushed the cursor branch from 1ec9ec4 to 268448b Compare October 3, 2016 16:19

larsgrefer closed this May 2, 2017

bmunkholm removed the S:Review label May 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add RealmCursor #53

add RealmCursor #53

larsgrefer commented Jun 16, 2016

dalinaum Jun 17, 2016

larsgrefer Jun 19, 2016

cmelchior commented Jun 22, 2016 •

edited

Loading

kneth Jun 22, 2016

larsgrefer commented Jun 24, 2016

kneth Jun 27, 2016

stk1m1 Jun 27, 2016 •

edited

Loading

kneth Jun 27, 2016

stk1m1 Jun 27, 2016

cmelchior Jun 29, 2016

cmelchior commented Jun 29, 2016

larsgrefer commented Jun 29, 2016

beeender commented Jul 1, 2016

larsgrefer commented Jul 1, 2016

cmelchior commented Jul 5, 2016


		import io.realm.entity.AllJavaTypes;

		public class RealmCursorTest extends AndroidTestCase {

add RealmCursor #53

add RealmCursor #53

Conversation

larsgrefer commented Jun 16, 2016

dalinaum Jun 17, 2016

Choose a reason for hiding this comment

larsgrefer Jun 19, 2016

Choose a reason for hiding this comment

cmelchior commented Jun 22, 2016 • edited Loading

kneth Jun 22, 2016

Choose a reason for hiding this comment

larsgrefer commented Jun 24, 2016

kneth Jun 27, 2016

Choose a reason for hiding this comment

stk1m1 Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

kneth Jun 27, 2016

Choose a reason for hiding this comment

stk1m1 Jun 27, 2016

Choose a reason for hiding this comment

cmelchior Jun 29, 2016

Choose a reason for hiding this comment

cmelchior commented Jun 29, 2016

larsgrefer commented Jun 29, 2016

beeender commented Jul 1, 2016

larsgrefer commented Jul 1, 2016

cmelchior commented Jul 5, 2016

cmelchior commented Jun 22, 2016 •

edited

Loading

stk1m1 Jun 27, 2016 •

edited

Loading