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 all commits
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
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
7 changes: 7 additions & 0 deletions app/src/main/java/com/quran/labs/androidquran/common/LocalTranslationDisplaySort.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.quran.labs.androidquran.common

class LocalTranslationDisplaySort : Comparator<LocalTranslation> {
override fun compare(p0: LocalTranslation, p1: LocalTranslation): Int {
return p0.displayOrder.compareTo(p1.displayOrder);
}
}
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)
}
7 changes: 7 additions & 0 deletions app/src/main/java/com/quran/labs/androidquran/dao/translation/TranslationItemDisplaySort.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.quran.labs.androidquran.dao.translation

class TranslationItemDisplaySort : Comparator<TranslationItem> {
override fun compare(p0: TranslationItem, p1: TranslationItem): Int {
return p0.displayOrder.compareTo(p1.displayOrder);
}
}
59 changes: 44 additions & 15 deletions 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 @@ -72,23 +72,27 @@ public List<LocalTranslation> getTranslations() {
null, null, null, null, null,
TranslationsTable.ID + " ASC");
if (cursor != null) {
while (cursor.moveToNext()) {
int id = cursor.getInt(0);
String name = cursor.getString(1);
String translator = cursor.getString(2);
String translatorForeign = cursor.getString(3);
String filename = cursor.getString(4);
String url = cursor.getString(5);
String languageCode = cursor.getString(6);
int version = cursor.getInt(7);
int minimumVersion = cursor.getInt(8);

if (quranFileUtils.hasTranslation(context, filename)) {
items.add(new LocalTranslation(id, filename, name, translator,
translatorForeign, url, languageCode, version, minimumVersion));
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment below - try should move up though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Salam, @ahmedre, can you explain why would we need to close a null cursor?

Copy link
Contributor

Choose a reason for hiding this comment

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

wa3laikum alsalam - you don't, but you can't skip closing the cursor until you assign it either - so as a better pattern, you do something like:

Cursor cursor = null;
try {
  cursor = db.query()
  while (cursor.moveToNext()) {
  }
} finally {
  if (cursor != null) {
    cursor.close()
  }
}

cursor.close() can actually throw also, which is why we have the DatabaseUtils.closeCursor method which does that for you - it checks if it's null and if so does nothing - if it's not null, it tries to close it and swallows any exceptions it throws.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ok, @ahmedre, you just shuffled the null check, lol. I can't say that is necessarily "better"... actually it's a little more "traditional" e.g. to catch errors from cursor instantiation, in which case you'd need to keep a reference to it outside of try/catch. But that's neither here nor there.

However, I'm curious about DatabaseUtils.closeCursor. Why would we want to swallow cursor.close() and not db.query(). I was going for consistency (ie throw everything)... but are you just wanting to ignore cursor close errors specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

if cursor.close() fails, there's nothing i can do about it - there's nothing that can be fixed, etc so we swallow it. db.query() failing could be because of a bad query, missing column, etc and can fail for a number of reasons, and in all of these cases i want to know about it so i can fix it.

while (cursor.moveToNext()) {
int id = cursor.getInt(0);
String name = cursor.getString(1);
String translator = cursor.getString(2);
String translatorForeign = cursor.getString(3);
String filename = cursor.getString(4);
String url = cursor.getString(5);
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, displayOrder));
}
}
} finally {
cursor.close();
}
cursor.close();
}
items = Collections.unmodifiableList(items);
if (items.size() > 0) {
Expand All @@ -105,11 +109,35 @@ 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"
);
try {
if (cursor != null && cursor.moveToFirst()) {
displayOrder = cursor.getInt(0) + 1;
}
} finally {
if (cursor !=null) cursor.close();
}
}

ContentValues values = new ContentValues();
values.put(TranslationsTable.ID, translation.getId());
values.put(TranslationsTable.NAME, translation.getDisplayName());
Expand All @@ -121,6 +149,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
44 changes: 42 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,41 @@ 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
);
try {
if (translations != null && translations.moveToFirst()) {
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)) }
);
translations.moveToNext();
}
}
} finally {
if (translations != null) translations.close();
}
db.setTransactionSuccessful();
} finally {
db.endTransaction();
}
}
}

static class TranslationsTable {
Expand All @@ -82,5 +121,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.LocalTranslationDisplaySort;
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 LocalTranslationDisplaySort());

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