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

Introduce predicate caching as an opt-in setting #5546

Merged
merged 4 commits into from
Apr 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion buildSrc/src/main/java/dependencies/Dependencies.kt
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ object Dependencies {
const val rarepebble_colorpicker = "com.github.martin-stone:hsv-alpha-color-picker-android:3.0.1"
const val commons_io = "commons-io:commons-io:2.5" // Commons 2.6+ introduce java.nio usage that we can't access until our minSdkVersion >= 26 (https://developer.android.com/reference/java/io/File#toPath())
const val opencsv = "com.opencsv:opencsv:5.7.1"
const val javarosa = "org.getodk:javarosa:4.1.0"
const val javarosa = "org.getodk:javarosa:4.2.0-SNAPSHOT"
const val javarosa_local = "org.getodk:javarosa:local"
const val karumi_dexter = "com.karumi:dexter:6.2.3"
const val zxing_android_embedded = "com.journeyapps:zxing-android-embedded:4.3.0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@

package org.odk.collect.android.instrumented.forms;

import static junit.framework.Assert.assertEquals;

import android.app.Application;

import androidx.test.core.app.ApplicationProvider;

import org.javarosa.core.model.FormDef;
import org.javarosa.form.api.FormEntryController;
import org.javarosa.form.api.FormEntryModel;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.RuleChain;
Expand All @@ -46,8 +50,6 @@

import timber.log.Timber;

import static junit.framework.Assert.assertEquals;

/**
* This test has been created in order to check indices while navigating through a form.
* It's especially important while navigating through a form that contains nested groups and if we
Expand All @@ -59,6 +61,13 @@
@RunWith(Parameterized.class)
public class FormNavigationTest {

private final FormLoaderTask.FormEntryControllerFactory formEntryControllerFactory = new FormLoaderTask.FormEntryControllerFactory() {
@Override
public FormEntryController create(FormDef formDef) {
return new FormEntryController(new FormEntryModel(formDef));
}
};

@Rule
public RuleChain copyFormChain = TestRuleChain.chain()
.around(new RunnableRule(() -> {
Expand Down Expand Up @@ -120,7 +129,7 @@ private void testIndices(String formName, String[] expectedIndices) throws Execu
Timber.i(e);
}

FormLoaderTask formLoaderTask = new FormLoaderTask(formPath(formName), null, null);
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath(formName), null, null, formEntryControllerFactory);
formLoaderTask.setFormLoaderListener(new FormLoaderListener() {
@Override
public void loadingComplete(FormLoaderTask task, FormDef fd, String warningMsg) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
package org.odk.collect.android.instrumented.forms;

import org.javarosa.core.model.FormDef;
import org.javarosa.core.reference.RootTranslator;
import org.javarosa.form.api.FormEntryController;
import org.javarosa.form.api.FormEntryModel;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.RuleChain;
import org.odk.collect.android.support.rules.CollectTestRule;
import org.odk.collect.android.support.rules.TestRuleChain;
import org.odk.collect.android.utilities.FormUtils;
import org.odk.collect.android.storage.StoragePathProvider;
import org.odk.collect.android.storage.StorageSubdirectory;
import org.odk.collect.android.support.rules.CollectTestRule;
import org.odk.collect.android.support.rules.TestRuleChain;
import org.odk.collect.android.tasks.FormLoaderTask;
import org.odk.collect.android.utilities.FileUtils;
import org.odk.collect.android.utilities.FormUtils;

import java.io.File;
import java.util.List;
Expand All @@ -21,6 +24,13 @@ public class FormUtilsTest {
private static final String BASIC_FORM = "basic.xml";
private final CollectTestRule rule = new CollectTestRule();

private final FormLoaderTask.FormEntryControllerFactory formEntryControllerFactory = new FormLoaderTask.FormEntryControllerFactory() {
@Override
public FormEntryController create(FormDef formDef) {
return new FormEntryController(new FormEntryModel(formDef));
}
};

@Rule
public RuleChain copyFormChain = TestRuleChain.chain()
.around(rule);
Expand All @@ -38,7 +48,7 @@ public void setUp() {
public void sessionRootTranslatorOrderDoesNotMatter() throws Exception {
final String formPath = new StoragePathProvider().getOdkDirPath(StorageSubdirectory.FORMS) + File.separator + BASIC_FORM;
// Load the form in order to populate the ReferenceManager
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null);
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null, formEntryControllerFactory);
formLoaderTask.execute(formPath).get();

final File formXml = new File(formPath);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
package org.odk.collect.android.instrumented.tasks;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.notNullValue;

import android.app.Application;

import androidx.test.core.app.ApplicationProvider;

import org.javarosa.core.model.FormDef;
import org.javarosa.form.api.FormEntryController;
import org.javarosa.form.api.FormEntryModel;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -12,20 +22,14 @@
import org.odk.collect.android.support.rules.RunnableRule;
import org.odk.collect.android.support.rules.TestRuleChain;
import org.odk.collect.android.tasks.FormLoaderTask;
import org.odk.collect.android.tasks.FormLoaderTask.FormEntryControllerFactory;
import org.odk.collect.projects.Project;

import java.io.File;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.notNullValue;

import android.app.Application;

import androidx.test.core.app.ApplicationProvider;

public class FormLoaderTaskTest {

private final StoragePathProvider storagePathProvider = new StoragePathProvider();
Expand All @@ -35,6 +39,13 @@ public class FormLoaderTaskTest {
private static final String SIMPLE_SEARCH_EXTERNAL_CSV_FILE = "simple-search-external-csv-fruits.csv";
private static final String SIMPLE_SEARCH_EXTERNAL_DB_FILE = "simple-search-external-csv-fruits.db";

private final FormEntryControllerFactory formEntryControllerFactory = new FormEntryControllerFactory() {
@Override
public FormEntryController create(FormDef formDef) {
return new FormEntryController(new FormEntryModel(formDef));
}
};

@Rule
public RuleChain copyFormChain = TestRuleChain.chain()
.around(new RunnableRule(() -> {
Expand All @@ -55,7 +66,7 @@ public class FormLoaderTaskTest {
@Test
public void loadFormWithSecondaryCSV() throws Exception {
final String formPath = storagePathProvider.getOdkDirPath(StorageSubdirectory.FORMS) + File.separator + SECONDARY_INSTANCE_EXTERNAL_CSV_FORM;
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null);
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null, formEntryControllerFactory);
FormLoaderTask.FECWrapper wrapper = formLoaderTask.execute(formPath).get();
Assert.assertNotNull(wrapper);
}
Expand All @@ -64,15 +75,15 @@ public void loadFormWithSecondaryCSV() throws Exception {
@Test
public void loadSearchFromExternalCSV() throws Exception {
final String formPath = storagePathProvider.getOdkDirPath(StorageSubdirectory.FORMS) + File.separator + SIMPLE_SEARCH_EXTERNAL_CSV_FORM;
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null);
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null, formEntryControllerFactory);
FormLoaderTask.FECWrapper wrapper = formLoaderTask.execute(formPath).get();
assertThat(wrapper, notNullValue());
}

@Test
public void loadSearchFromexternalCsvLeavesFileUnchanged() throws Exception {
final String formPath = storagePathProvider.getOdkDirPath(StorageSubdirectory.FORMS) + File.separator + SIMPLE_SEARCH_EXTERNAL_CSV_FORM;
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null);
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null, formEntryControllerFactory);
FormLoaderTask.FECWrapper wrapper = formLoaderTask.execute(formPath).get();
Assert.assertNotNull(wrapper);
Assert.assertNotNull(wrapper.getController());
Expand All @@ -87,7 +98,7 @@ public void loadSearchFromexternalCsvLeavesFileUnchanged() throws Exception {
public void loadSearchFromExternalCSVmultipleTimes() throws Exception {
final String formPath = storagePathProvider.getOdkDirPath(StorageSubdirectory.FORMS) + File.separator + SIMPLE_SEARCH_EXTERNAL_CSV_FORM;
// initial load with side effects
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null);
FormLoaderTask formLoaderTask = new FormLoaderTask(formPath, null, null, formEntryControllerFactory);
FormLoaderTask.FECWrapper wrapper = formLoaderTask.execute(formPath).get();
Assert.assertNotNull(wrapper);
Assert.assertNotNull(wrapper.getController());
Expand All @@ -98,7 +109,7 @@ public void loadSearchFromExternalCSVmultipleTimes() throws Exception {
long dbLastModified = dbFile.lastModified();

// subsequent load should succeed despite side effects from import
formLoaderTask = new FormLoaderTask(formPath, null, null);
formLoaderTask = new FormLoaderTask(formPath, null, null, formEntryControllerFactory);
wrapper = formLoaderTask.execute(formPath).get();
Assert.assertNotNull(wrapper);
Assert.assertNotNull(wrapper.getController());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,9 @@ enum AnimationType {
@Inject
public AudioHelperFactory audioHelperFactory;

@Inject
public FormLoaderTask.FormEntryControllerFactory formEntryControllerFactory;

private final LocationProvidersReceiver locationProvidersReceiver = new LocationProvidersReceiver();

private SwipeHandler swipeHandler;
Expand Down Expand Up @@ -634,7 +637,7 @@ private void loadForm() {
formEntryViewModel.refresh();
} else {
Timber.w("Reloading form and restoring state.");
formLoaderTask = new FormLoaderTask(instancePath, startingXPath, waitingXPath);
formLoaderTask = new FormLoaderTask(instancePath, startingXPath, waitingXPath, formEntryControllerFactory);
showIfNotShowing(FormLoadingDialogFragment.class, getSupportFragmentManager());
formLoaderTask.execute(formPath);
}
Expand Down Expand Up @@ -713,7 +716,7 @@ private void loadFromIntent(Intent intent) {
return;
}

formLoaderTask = new FormLoaderTask(instancePath, null, null);
formLoaderTask = new FormLoaderTask(instancePath, null, null, formEntryControllerFactory);
formLoaderTask.setFormLoaderListener(this);
showIfNotShowing(FormLoadingDialogFragment.class, getSupportFragmentManager());
formLoaderTask.execute(formPath);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package org.odk.collect.android.formmanagement

import org.javarosa.core.model.FormDef
import org.javarosa.entities.EntityFormFinalizationProcessor
import org.javarosa.form.api.FormEntryController
import org.javarosa.form.api.FormEntryModel
import org.odk.collect.android.tasks.FormLoaderTask.FormEntryControllerFactory
import org.odk.collect.settings.keys.ProjectKeys
import org.odk.collect.shared.settings.Settings

class CollectFormEntryControllerFactory constructor(private val settings: Settings) :
FormEntryControllerFactory {
override fun create(formDef: FormDef): FormEntryController {
return FormEntryController(FormEntryModel(formDef)).also {
it.addPostProcessor(EntityFormFinalizationProcessor())

if (!settings.getBoolean(ProjectKeys.KEY_PREDICATE_CACHING)) {
it.disablePredicateCaching()
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.odk.collect.android.formentry.media.AudioHelperFactory;
import org.odk.collect.android.formentry.media.ScreenContextAudioHelperFactory;
import org.odk.collect.android.formlists.blankformlist.BlankFormListViewModel;
import org.odk.collect.android.formmanagement.CollectFormEntryControllerFactory;
import org.odk.collect.android.formmanagement.FormDownloader;
import org.odk.collect.android.formmanagement.FormMetadataParser;
import org.odk.collect.android.formmanagement.FormSourceProvider;
Expand Down Expand Up @@ -84,6 +85,7 @@
import org.odk.collect.android.projects.ProjectDependencyProviderFactory;
import org.odk.collect.android.storage.StoragePathProvider;
import org.odk.collect.android.storage.StorageSubdirectory;
import org.odk.collect.android.tasks.FormLoaderTask;
import org.odk.collect.android.utilities.AdminPasswordProvider;
import org.odk.collect.android.utilities.AndroidUserAgent;
import org.odk.collect.android.utilities.ChangeLockProvider;
Expand Down Expand Up @@ -633,4 +635,9 @@ public BlankFormListViewModel.Factory providesBlankFormListViewModel(FormsReposi
public ImageCompressionController providesImageCompressorManager() {
return new ImageCompressionController(ImageCompressor.INSTANCE);
}

@Provides
public FormLoaderTask.FormEntryControllerFactory formEntryControllerFactory(SettingsProvider settingsProvider) {
return new CollectFormEntryControllerFactory(settingsProvider.getUnprotectedSettings());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ object Defaults {
hashMap[ProjectKeys.KEY_USGS_MAP_STYLE] = "topographic"
hashMap[ProjectKeys.KEY_GOOGLE_MAP_STYLE] = GoogleMap.MAP_TYPE_NORMAL.toString()
hashMap[ProjectKeys.KEY_MAPBOX_MAP_STYLE] = "mapbox://styles/mapbox/streets-v11"
// experimental_preferences.xml
hashMap[ProjectKeys.KEY_PREDICATE_CACHING] = false
return hashMap
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import android.database.SQLException;
import android.os.AsyncTask;

import androidx.annotation.NonNull;

import com.opencsv.CSVReader;
import com.opencsv.exceptions.CsvValidationException;

Expand All @@ -32,9 +34,7 @@
import org.javarosa.core.model.instance.TreeReference;
import org.javarosa.core.model.instance.utils.DefaultAnswerResolver;
import org.javarosa.core.reference.ReferenceManager;
import org.javarosa.entities.EntityFormFinalizationProcessor;
import org.javarosa.form.api.FormEntryController;
import org.javarosa.form.api.FormEntryModel;
import org.javarosa.xform.parse.XFormParser;
import org.javarosa.xform.util.XFormUtils;
import org.javarosa.xpath.XPathTypeMismatchException;
Expand Down Expand Up @@ -81,6 +81,7 @@ public class FormLoaderTask extends AsyncTask<String, String, FormLoaderTask.FEC
private String instancePath;
private final String xpath;
private final String waitingXPath;
private FormEntryControllerFactory formEntryControllerFactory;
private boolean pendingActivityResult;
private int requestCode;
private int resultCode;
Expand Down Expand Up @@ -112,10 +113,11 @@ protected void free() {

FECWrapper data;

public FormLoaderTask(String instancePath, String xpath, String waitingXPath) {
public FormLoaderTask(String instancePath, String xpath, String waitingXPath, FormEntryControllerFactory formEntryControllerFactory) {
this.instancePath = instancePath;
this.xpath = xpath;
this.waitingXPath = waitingXPath;
this.formEntryControllerFactory = formEntryControllerFactory;
}

/**
Expand Down Expand Up @@ -176,9 +178,7 @@ protected FECWrapper doInBackground(String... path) {
}

// create FormEntryController from formdef
final FormEntryModel fem = new FormEntryModel(formDef);
final FormEntryController fec = new FormEntryController(fem);
fec.addPostProcessor(new EntityFormFinalizationProcessor());
final FormEntryController fec = formEntryControllerFactory.create(formDef);

boolean usedSavepoint = false;

Expand Down Expand Up @@ -574,4 +574,8 @@ private void readCSV(File csv, String formHash, String pathHash) {
public FormDef getFormDef() {
return formDef;
}

public interface FormEntryControllerFactory {
FormEntryController create(@NonNull FormDef formDef);
}
}
4 changes: 4 additions & 0 deletions collect_app/src/main/res/xml/experimental_preferences.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
xmlns:app="http://schemas.android.com/apk/res-auto"
android:title="@string/experimental">

<SwitchPreferenceCompat
android:title="@string/predicate_caching"
android:key="predicate_caching" />

<Preference
android:icon="@drawable/ic_person_black_24dp"
android:title="@string/entities_title"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ object ProjectKeys {
const val KEY_BACKGROUND_LOCATION = "background_location"
const val KEY_BACKGROUND_RECORDING = "background_recording"

// experimental_preferences.xml
const val KEY_PREDICATE_CACHING = "predicate_caching"

// values
const val PROTOCOL_SERVER = "odk_default"
const val PROTOCOL_GOOGLE_SHEETS = "google_sheets"
Expand Down
1 change: 1 addition & 0 deletions strings/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1196,5 +1196,6 @@
<string name="crash_app">Crash app</string>
<!-- An uncaught exception is an exception that is not handled by ODK Collect and so will force the application to close -->
<string name="crash_app_summary">Force an uncaught exception causing the app to crash</string>
<string name="predicate_caching">Predicate caching</string>

</resources>