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.

Sign up for GitHub

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 Reordering Of Translations. Fixes #1179 #1398

Merged
merged 5 commits into from
Sep 29, 2020
Merged

Support Reordering Of Translations. Fixes #1179 #1398

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b6c4a9
Support Reordering Of Translations. Fixes #1179
papasmile Apr 25, 2020
b8ac99e
cleanup
papasmile Jul 16, 2020
25dd889
Initialize Cursor
papasmile Jul 17, 2020
b7f5aca
cleanup and multi-move
papasmile Jul 23, 2020
604b9bf
spelling
papasmile Jul 24, 2020
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
4 changes: 2 additions & 2 deletions app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ android {
defaultConfig {
minSdkVersion deps.android.build.minSdkVersion
targetSdkVersion deps.android.build.targetSdkVersion
versionCode 3010
versionName "3.0.1"
versionCode 3100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert these changes. (Most probably you used them to test an upgrade?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops yes

versionName "3.1.0"
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
multiDexEnabled true
}
Expand Down
8 changes: 8 additions & 0 deletions app/src/main/java/com/quran/labs/androidquran/common/LocalTransationDiplaySort.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.quran.labs.androidquran.common

class LocalTransationDiplaySort : Comparator<LocalTranslation> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who ate the s? :)

Copy link
Contributor Author

@papasmile papasmile Jul 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the 'l' too you mean? yea oops correcting spelling...

override fun compare(p0: LocalTranslation?, p1: LocalTranslation?): Int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really expect these to be nullable? because if not, we can simplify things here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, good call

if (p0 == null || p1 == null) return 0;
return p0.displayOrder.compareTo(p1.displayOrder);
}
}
3 changes: 2 additions & 1 deletion app/src/main/java/com/quran/labs/androidquran/common/LocalTranslation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ data class LocalTranslation(
val url: String = "",
val languageCode: String? = "",
val version: Int = 1,
val minimumVersion: Int = 2) {
val minimumVersion: Int = 2,
val displayOrder: Int = -1) {

fun getTranslatorName(): String {
return when {
Expand Down
1 change: 1 addition & 0 deletions app/src/main/java/com/quran/labs/androidquran/common/TranslationMetadata.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ import com.quran.data.model.SuraAyah
data class TranslationMetadata(val sura: Int,
val ayah: Int,
val text: CharSequence,
val localTranslationId: Int? = null,
val link: SuraAyah? = null)
3 changes: 2 additions & 1 deletion app/src/main/java/com/quran/labs/androidquran/dao/translation/Translation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ data class Translation(val id: Int,
val saveTo: String,
val languageCode: String,
val translator: String? = "",
@Json(name = "translatorForeign") val translatorNameLocalized: String? = "") {
@Json(name = "translatorForeign") val translatorNameLocalized: String? = "",
val displayOrder: Int = -1) {

fun withSchema(schema: Int) = copy(minimumVersion = schema)
}
5 changes: 4 additions & 1 deletion app/src/main/java/com/quran/labs/androidquran/dao/translation/TranslationItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package com.quran.labs.androidquran.dao.translation

data class TranslationItem @JvmOverloads constructor(val translation: Translation,
val localVersion: Int = 0) : TranslationRowData {
val localVersion: Int = 0,
val displayOrder: Int = -1) : TranslationRowData {

override fun isSeparator() = false

Expand All @@ -16,4 +17,6 @@ data class TranslationItem @JvmOverloads constructor(val translation: Translatio
fun withTranslationRemoved() = this.copy(localVersion = 0)

fun withTranslationVersion(version: Int) = this.copy(localVersion = version)

fun withDisplayOrder(newDisplayOrder: Int) = this.copy(displayOrder = newDisplayOrder)
}
8 changes: 8 additions & 0 deletions app/src/main/java/com/quran/labs/androidquran/dao/translation/TranslationItemDiplaySort.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.quran.labs.androidquran.dao.translation

class TranslationItemDiplaySort : Comparator<TranslationItem> {
override fun compare(p0: TranslationItem?, p1: TranslationItem?): Int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, can these actually be nullable?

if (p0 == null || p1 == null) return 0;
return p0.displayOrder.compareTo(p1.displayOrder);
}
}
24 changes: 23 additions & 1 deletion app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,11 @@ public List<LocalTranslation> getTranslations() {
String languageCode = cursor.getString(6);
int version = cursor.getInt(7);
int minimumVersion = cursor.getInt(8);
int displayOrder = cursor.getInt(9);

if (quranFileUtils.hasTranslation(context, filename)) {
items.add(new LocalTranslation(id, filename, name, translator,
translatorForeign, url, languageCode, version, minimumVersion));
translatorForeign, url, languageCode, version, minimumVersion, displayOrder));
}
}
cursor.close();
Expand All @@ -105,11 +106,31 @@ public void deleteTranslationByFile(String filename) {
public boolean writeTranslationUpdates(List<TranslationItem> updates) {
boolean result = true;
db.beginTransaction();

try {
for (int i = 0, updatesSize = updates.size(); i < updatesSize; i++) {
TranslationItem item = updates.get(i);
if (item.exists()) {
int displayOrder = 0;

final Translation translation = item.getTranslation();

if (item.getDisplayOrder() > -1) {
displayOrder = item.getDisplayOrder();
} else {
// get next highest display order
Cursor cursor = db.query(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, consider closing this cursor.

Copy link
Contributor Author

@papasmile papasmile Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, yes to both cursor comments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should do this in a try / finally because if an exception is thrown due to the query, we will leak the cursor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry to nitpick, the try/catch should be before the cursor assignment even - so like:

Cursor cursor = null;
try {
  cursor = ..
} finally {
  DatabasUtils.closeCursor(cursor);
}

TranslationsTable.TABLE_NAME,
new String[] {TranslationsTable.DISPLAY_ORDER},
null, null, null, null,
TranslationsTable.DISPLAY_ORDER + " DESC",
"1"
);
if (cursor != null && cursor.moveToNext()) {
displayOrder = cursor.getInt(0) + 1;
}
}

ContentValues values = new ContentValues();
values.put(TranslationsTable.ID, translation.getId());
values.put(TranslationsTable.NAME, translation.getDisplayName());
Expand All @@ -121,6 +142,7 @@ public boolean writeTranslationUpdates(List<TranslationItem> updates) {
values.put(TranslationsTable.LANGUAGE_CODE, translation.getLanguageCode());
values.put(TranslationsTable.VERSION, item.getLocalVersion());
values.put(TranslationsTable.MINIMUM_REQUIRED_VERSION, translation.getMinimumVersion());
values.put(TranslationsTable.DISPLAY_ORDER, displayOrder);

db.replace(TranslationsTable.TABLE_NAME, null, values);
} else {
Expand Down
37 changes: 35 additions & 2 deletions app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.quran.labs.androidquran.database;

import android.content.ContentValues;
import android.content.Context;
import android.database.Cursor;
import android.database.sqlite.SQLiteDatabase;
import android.database.sqlite.SQLiteOpenHelper;

Expand All @@ -11,7 +13,7 @@
class TranslationsDBHelper extends SQLiteOpenHelper {

private static final String DB_NAME = "translations.db";
private static final int DB_VERSION = 4;
private static final int DB_VERSION = 5;
private static final String CREATE_TRANSLATIONS_TABLE =
"CREATE TABLE " + TranslationsTable.TABLE_NAME + "(" +
TranslationsTable.ID + " integer primary key, " +
Expand All @@ -22,7 +24,9 @@ class TranslationsDBHelper extends SQLiteOpenHelper {
TranslationsTable.URL + " varchar, " +
TranslationsTable.LANGUAGE_CODE + " varchar, " +
TranslationsTable.VERSION + " integer not null default 0," +
TranslationsTable.MINIMUM_REQUIRED_VERSION + " integer not null default 0);";
TranslationsTable.MINIMUM_REQUIRED_VERSION + " integer not null default 0, " +
TranslationsTable.DISPLAY_ORDER + " integer not null default -1 " +
");";

@Inject
TranslationsDBHelper(Context context) {
Expand Down Expand Up @@ -69,6 +73,34 @@ public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
db.endTransaction();
}
}
if (oldVersion < 5) {
// Add display order column and add arbitrary order to existing translations
db.beginTransaction();
try {
db.execSQL("ALTER TABLE "
+ TranslationsTable.TABLE_NAME
+ " ADD COLUMN "
+ TranslationsTable.DISPLAY_ORDER
+ " integer not null default -1"
);
Cursor translations = db.query(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure if db.endTransaction() auto-closes previous cursors, but you may need to close this cursor as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - also, as a hack, can we just use the row id as the value here so we don't have to do an upload loop?
i.e. something like

UPDATE translations SET display_order = id

TranslationsTable.TABLE_NAME, new String[] { TranslationsTable.ID }, null, null, null, null, null
);
for (int i = 0; i < translations.getCount(); i++) {
ContentValues values = new ContentValues();
values.put(TranslationsTable.DISPLAY_ORDER, i);
db.update(
TranslationsTable.TABLE_NAME,
values,
TranslationsTable.ID + " = ?",
new String[] { String.valueOf(translations.getInt(0)) }
);
}
db.setTransactionSuccessful();
} finally {
db.endTransaction();
}
}
}

static class TranslationsTable {
Expand All @@ -82,5 +114,6 @@ static class TranslationsTable {
static final String LANGUAGE_CODE = "languageCode";
static final String VERSION = "version";
static final String MINIMUM_REQUIRED_VERSION = "minimumRequiredVersion";
static final String DISPLAY_ORDER = "userDisplayOrder";
}
}
8 changes: 5 additions & 3 deletions ...c/main/java/com/quran/labs/androidquran/presenter/translation/BaseTranslationPresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,16 @@ internal open class BaseTranslationPresenter<T> internal constructor(
if (element != null) {
// replace with "" when a translation doesn't load to keep translations aligned
val ayahTranslations = quranTexts.mapIndexed { index: Int, quranText: QuranText? ->
val translationMinVersion = translationInfo.getOrNull(index)?.minimumVersion ?: 0
val translation = translationInfo.getOrNull(index);
val translationMinVersion = translation?.minimumVersion ?: 0
val translationId = translation?.id ?: -1;
val shouldProcess =
translationMinVersion >= TranslationUtil.MINIMUM_PROCESSING_VERSION
val text = quranText ?: QuranText(element.sura, element.ayah, "")
if (shouldProcess) {
translationUtil.parseTranslationText(text)
translationUtil.parseTranslationText(text, translationId)
} else {
TranslationMetadata(element.sura, element.ayah, text.text)
TranslationMetadata(element.sura, element.ayah, text.text, translationId)
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...n/java/com/quran/labs/androidquran/presenter/translation/TranslationManagerPresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ private List<TranslationItem> mergeWithServerTranslations(List<Translation> serv
override = new TranslationItem(translation.withSchema(versions.second), versions.first);
}
} else {
item = new TranslationItem(translation, local.getVersion());
item = new TranslationItem(translation, local.getVersion(), local.getDisplayOrder());
}
} else {
item = new TranslationItem(translation);
Expand Down
16 changes: 11 additions & 5 deletions app/src/main/java/com/quran/labs/androidquran/ui/PagerActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.quran.labs.androidquran.QuranPreferenceActivity;
import com.quran.labs.androidquran.R;
import com.quran.labs.androidquran.SearchActivity;
import com.quran.labs.androidquran.common.LocalTransationDiplaySort;
import com.quran.labs.androidquran.common.LocalTranslation;
import com.quran.labs.androidquran.common.audio.QariItem;
import com.quran.labs.androidquran.di.component.activity.PagerActivityComponent;
Expand Down Expand Up @@ -86,6 +87,8 @@
import com.quran.labs.androidquran.widgets.SlidingUpPanelLayout;

import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -1354,10 +1357,13 @@ private void requestTranslationsList() {
.subscribeWith(new DisposableSingleObserver<List<LocalTranslation>>() {
@Override
public void onSuccess(List<LocalTranslation> translationList) {
int items = translationList.size();
final List<LocalTranslation> sortedTranslations = new ArrayList<>(translationList);
Collections.sort(sortedTranslations, new LocalTransationDiplaySort());

int items = sortedTranslations.size();
String[] titles = new String[items];
for (int i = 0; i < items; i++) {
LocalTranslation item = translationList.get(i);
LocalTranslation item = sortedTranslations.get(i);
if (!TextUtils.isEmpty(item.getTranslatorForeign())) {
titles[i] = item.getTranslatorForeign();
} else if (!TextUtils.isEmpty(item.getTranslator())) {
Expand All @@ -1371,16 +1377,16 @@ public void onSuccess(List<LocalTranslation> translationList) {
if (currentActiveTranslations.isEmpty() && items > 0) {
currentActiveTranslations = new HashSet<>();
for (int i = 0; i < items; i++) {
currentActiveTranslations.add(translationList.get(i).getFilename());
currentActiveTranslations.add(sortedTranslations.get(i).getFilename());
}
}
activeTranslations = currentActiveTranslations;

if (translationsSpinnerAdapter != null) {
translationsSpinnerAdapter.updateItems(titles, translationList, activeTranslations);
translationsSpinnerAdapter.updateItems(titles, sortedTranslations, activeTranslations);
}
translationItems = titles;
translations = translationList;
translations = sortedTranslations;

if (showingTranslation) {
// Since translation items have changed, need to
Expand Down
Loading