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

Added Realm.isEmpty() #1713

Merged
merged 1 commit into from Nov 3, 2015
Merged

Added Realm.isEmpty() #1713

merged 1 commit into from Nov 3, 2015

Conversation

@dalinaum
Copy link
Contributor

dalinaum commented Nov 2, 2015

Please review this @realm/java

JNIEnv*, jobject, jlong nativeGroupPtr)
{
Group* grp = G(nativeGroupPtr);
const std::string table_prefix = "class_";

This comment has been minimized.

Copy link
@beeender

beeender Nov 2, 2015

Contributor

Maybe this one should be in a common header? Like util.hpp?

This comment has been minimized.

Copy link
@cmelchior

cmelchior Nov 2, 2015

Contributor

Currently Table.java also has a constant with this: Table.TABLE_PREFIX. Having it as an accessible constant from C++ would probably be nice, so we only have it defined 1 place.

This comment has been minimized.

Copy link
@kneth

kneth Nov 2, 2015

Member

Yes, it is declared in io.realm.internal.Table but apparently javah does not generate macros for string constants.

This comment has been minimized.

Copy link
@dalinaum

dalinaum Nov 2, 2015

Author Contributor

I tried the following code:

    jclass class_id = GetClass(env, class_name);
    if (class_id == NULL) {
        return false;
    }
    jfieldID field_id = env->GetStaticFieldID(class_id, "TABLE_PREFIX", "[Ljava/lang/String;");
    if (field_id == NULL) {
        ThrowException(env, NoSuchField, class_name);
        return false;
    }
    jstring jstring_table_name = (jstring) env->GetStaticObjectField(class_id, field_id);
    if (jstring_table_name == NULL) {
        ThrowException(env, NoSuchField, class_name);
        return false;
    }
    const char *table_prefix_c_str = env->GetStringUTFChars(jstring_table_name, 0);
    const std::string table_prefix(table_prefix_c_str);

And I got the following error:

Caused by: java.lang.ClassNotFoundException: Didn't find class "Ljava.lang.String" on path: DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file "/data/app/io.realm.test-1/base.apk", zip file "/data/app/io.realm.test-1/base.apk"],nativeLibraryDirectories=[/data/app/io.realm.test-1/lib/x86, /data/app/io.realm.test-1/lib/x86, /vendor/lib, /system/lib]]

Is there any solution to use Table.TABLE_PREFIX correctly?

*
* @return {@code true} if empty, @{code false} otherwise.
*/
public boolean isEmptyTables() {

This comment has been minimized.

Copy link
@beeender

beeender Nov 2, 2015

Contributor

It seems Group.size() and Group.isEmpty() are useless now? Shall we just refactor them instead of creating a new method?

This comment has been minimized.

Copy link
@dalinaum

dalinaum Nov 2, 2015

Author Contributor

I don't know whether Group.size() and Group.isEmpty() are useless. Is it useless? Should I mark them as deprecated or just remove them?

This comment has been minimized.

Copy link
@cmelchior

cmelchior Nov 2, 2015

Contributor

I think there is a distinction between an empty group (no tables) and a group with tables but no objects. I would keep Group.isEmpty() and Group.size() for now as we might need them for other things.

I am not a big fan of this method name as it seems a bit ambiguous to me. Maybe isObjectTablesEmpty instead?

This comment has been minimized.

Copy link
@beeender

beeender Nov 2, 2015

Contributor

👍 ^^


assertTrue(emptyRealm.isEmpty());

emptyRealm.beginTransaction();

This comment has been minimized.

Copy link
@cmelchior

cmelchior Nov 2, 2015

Contributor

This section looks like a copy of the above. Just remove it?

This comment has been minimized.

Copy link
@dalinaum

dalinaum Nov 2, 2015

Author Contributor

I think we need this section to check assertFalse(emptyRealm.isEmpty()) on line 2306.

This comment has been minimized.

Copy link
@cmelchior

cmelchior Nov 3, 2015

Contributor

Ah yes, sorry.

assertFalse(emptyRealm.isEmpty());

emptyRealm.close();
Realm.deleteRealm(realmConfig);

This comment has been minimized.

Copy link
@cmelchior

cmelchior Nov 2, 2015

Contributor

No need to delete the file. We don't do this in other unit tests. Closing the Realm is enough.

This comment has been minimized.

Copy link
@dalinaum

dalinaum Nov 2, 2015

Author Contributor

I see. I should delete this.

@@ -226,6 +226,15 @@ public void writeToFile(File file, byte[] key) throws IOException {
return nativeWriteToMem(nativePtr);
}

/*
* Check if the Group contains any objects.

This comment has been minimized.

Copy link
@cmelchior

cmelchior Nov 2, 2015

Contributor

Also add that this only checked for class_ tables or non-metadata tables, e.g. this would return true even if the pk table contained information.

@cmelchior cmelchior added the Review label Nov 2, 2015

for (size_t i = 0; i < grp->size(); ++i) {
ConstTableRef table = grp->get_table(i);
const std::string table_name = table->get_name();

This comment has been minimized.

Copy link
@kneth

kneth Nov 2, 2015

Member

No need for std::.

for (size_t i = 0; i < grp->size(); ++i) {
ConstTableRef table = grp->get_table(i);
const std::string table_name = table->get_name();
if (table_name.size() < table_prefix_length || table_name.compare(0, table_prefix_length, table_prefix) != 0) {

This comment has been minimized.

Copy link
@emanuelez

emanuelez Nov 2, 2015

Contributor

How about something like:

if (table_name.compare(0, table_prefix_length, table_prefix) == 0 && !table.is_empty()) {
   return false;
}
@dalinaum dalinaum force-pushed the lk-realm-is-empty branch 3 times, most recently from 9ec87cc to 3b331c9 Nov 2, 2015
@dalinaum

This comment has been minimized.

Copy link
Contributor Author

dalinaum commented Nov 2, 2015

Thanks for reviews and please review this again @realm/java

PS: @cmelchior I wonder whether I understand your proposal #1716 (comment) correctly.

@@ -584,4 +584,6 @@ inline jobject NewFloat(JNIEnv* env, float value)
return env->NewObject(java_lang_float, java_lang_float_init, value);
}

extern const char * const TABLE_PREFIX;

This comment has been minimized.

Copy link
@beeender

beeender Nov 3, 2015

Contributor

-> extern const char*, remove the space.

@@ -31,6 +31,8 @@ using std::string;
int trace_level = 0;
const char *log_tag = "REALM";

const char * const TABLE_PREFIX = "class_";

This comment has been minimized.

Copy link
@beeender

beeender Nov 3, 2015

Contributor

-> extern const char*, remove the space.

@beeender

This comment has been minimized.

Copy link
Contributor

beeender commented Nov 3, 2015

Except the formatting issue, 👍

@@ -226,6 +226,17 @@ public void writeToFile(File file, byte[] key) throws IOException {
return nativeWriteToMem(nativePtr);
}

/*
* Check if the Group contains any objects. It only checked for "class_"

This comment has been minimized.

Copy link
@cmelchior

cmelchior Nov 3, 2015

Contributor

checked -> checks

@cmelchior

This comment has been minimized.

Copy link
Contributor

cmelchior commented Nov 3, 2015

Looks good. Except for the comments 👍

Closes #766
@dalinaum dalinaum force-pushed the lk-realm-is-empty branch from 3b331c9 to 4453787 Nov 3, 2015
dalinaum added a commit that referenced this pull request Nov 3, 2015
@dalinaum dalinaum merged commit fb6eaa4 into master Nov 3, 2015
1 check passed
1 check passed
default Build finished. 732 tests run, 0 skipped, 0 failed.
Details
@dalinaum dalinaum removed the Review label Nov 3, 2015
@dalinaum dalinaum deleted the lk-realm-is-empty branch Nov 3, 2015
emanuelez added a commit that referenced this pull request Nov 11, 2015
This reverts commit fb6eaa4, reversing
changes made to 4031387.

Conflicts:
	changelog.txt
	realm/realm-library/src/androidTest/java/io/realm/RealmTest.java
	realm/realm-library/src/main/java/io/realm/BaseRealm.java
	realm/realm-library/src/main/java/io/realm/internal/Group.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.