Support Reordering Of Translations. Fixes #1179 #1398

papasmile · 2020-07-07T15:01:41Z

Contributing fully re-orderable translations using new action menu in settings in response to Issue #1179

New version and database migration to add 'Display Order' column to local translation information
New Action Bar menu on long-press:
- Move up
- Move down
- Existing delete button
Changes instantly show in text view:
- Translation list sorted according to user-defined order
- Texts below Ayah are sorted according to user-defined order
By default, new downloads are added at bottom of list of translations

Tested with:

Anrdoid 9
Galaxy S8
Android Studio 4

papasmile · 2020-07-07T15:34:50Z

Hi, dunno what is up with build, it says:

BUILD SUCCESSFUL in 5m 4s
171 actionable tasks: 171 executed
The command "./gradlew clean connectedMadaniDebugAndroidTest -PdisableCrashlytics" exited with 0.

Done. Your build exited with 1.

ozbek · 2020-07-08T01:31:16Z

Hi, dunno what is up with build [...]

Here, unit tests are failing:

> Task :app:compileMadaniReleaseUnitTestKotlin FAILED
e: /home/travis/build/quran/quran_android/app/src/test/java/com/quran/labs/androidquran/presenter/translation/BaseTranslationPresenterTest.kt: (36, 11): 'parseTranslationText' overrides nothing

papasmile · 2020-07-08T15:46:24Z

thx, @ozbek . Yay, unit tests! I've fixed plus added an assertion for new changes... build passing

ozbek

Please do not force push new updates, @ahmedre can squash-merge if necessary :)

ozbek · 2020-07-09T03:30:01Z

app/build.gradle

@@ -17,8 +17,8 @@ android {
  defaultConfig {
    minSdkVersion deps.android.build.minSdkVersion
    targetSdkVersion deps.android.build.targetSdkVersion
-    versionCode 3010
-    versionName "3.0.1"
+    versionCode 3100


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

ozbek · 2020-07-09T03:30:48Z

app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBAdapter.java

+            displayOrder = item.getDisplayOrder();
+          } else {
+            // get next highest display order
+            Cursor cursor = db.query(


Please, consider closing this cursor.

oops, yes to both cursor comments

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

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

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

ozbek · 2020-07-09T03:32:46Z

app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBHelper.java

+            + TranslationsTable.DISPLAY_ORDER
+            + " integer not null default -1"
+        );
+        Cursor translations = db.query(


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

+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

ozbek · 2020-07-09T03:39:47Z

app/src/main/java/com/quran/labs/androidquran/ui/adapter/TranslationsAdapter.java

+  private final UnicastSubject<TranslationRowData> onClickRankUpSubject = UnicastSubject.create();
+  private final UnicastSubject<TranslationRowData> onClickRankDownSubject = UnicastSubject.create();
+
+  private final AppCompatActivity activity;


Why do you need an activity here? It is generally a bad idea to have a reference to an activity.
There should be a better way of achieving your goal. Maybe replace with an interface?

OK, I admit programmer laziness since were already using Context. Happy to make that a cleaner interface instead...

ozbek · 2020-07-09T03:41:08Z

app/src/main/java/com/quran/labs/androidquran/ui/adapter/TranslationsAdapter.java

+
+  private class ModeCallback implements ActionMode.Callback  {
+    @Override
+    public boolean onCreateActionMode ( ActionMode mode, Menu menu )


Please consider following the existing coding style.

@ozbek I have not figured out how project's code style works yet... is there an actual formatter config I am overlooking? Or is there just something(s) specific that you'd like to stick to?

Nothing serious, it's just how you have put the braces and use of white-space.

ozbek · 2020-07-09T03:52:49Z

app/src/main/res/values/strings.xml

@@ -237,6 +237,10 @@
  <string name="export_data_error">Error exporting data</string>
  <string name="exported_data">Data exported to %1$s</string>

+  <string name="dtm_move_up">Move Up</string>


Out of curiosity, what does "dtm" stand for? :)

lol 'downloaded translation menu' :) just needed to make it unique

I don't think there is a need for unique names here, make them generic (simply use move_up, move_down and delete_translation?). We might as well re-use them elsewhere in the future.

papasmile · 2020-07-16T13:42:01Z

Asslaam alaykum, @ozbek I think I have addressed all your feedback.

ozbek · 2020-07-17T07:21:21Z

Wa alaikum assalam. Jazak Allah khair for your efforts.

The app is crashing on a fresh install, would you please take a look?

Stack

E/AndroidRuntime: FATAL EXCEPTION: main
 Process: com.quran.labs.androidquran.debug, PID: 26812
 java.lang.RuntimeException: Unable to start activity ComponentInfo{com.quran.labs.androidquran.debug/com.quran.labs.androidquran.ui.QuranActivity}: android.database.CursorIndexOutOfBoundsException: Index -1 requested, with a size of 7
  at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3270)
  at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3409)
  at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:83)
  at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135)
  at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95)
  at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016)
  at android.os.Handler.dispatchMessage(Handler.java:107)
  at android.os.Looper.loop(Looper.java:214)
  at android.app.ActivityThread.main(ActivityThread.java:7356)
  at java.lang.reflect.Method.invoke(Native Method)
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
  Caused by: android.database.CursorIndexOutOfBoundsException: Index -1 requested, with a size of 7
  at android.database.AbstractCursor.checkPosition(AbstractCursor.java:515)
  at android.database.AbstractWindowedCursor.checkPosition(AbstractWindowedCursor.java:138)
  at android.database.AbstractWindowedCursor.getInt(AbstractWindowedCursor.java:70)
  at com.quran.labs.androidquran.database.TranslationsDBHelper.onUpgrade(TranslationsDBHelper.java:96)
  at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:417)
  at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:317)
  at com.quran.labs.androidquran.database.TranslationsDBAdapter.<init>(TranslationsDBAdapter.java:41)
  at com.quran.labs.androidquran.database.TranslationsDBAdapter_Factory.newInstance(TranslationsDBAdapter_Factory.java:41)
  at com.quran.labs.androidquran.database.TranslationsDBAdapter_Factory.get(TranslationsDBAdapter_Factory.java:30)
  at com.quran.labs.androidquran.database.TranslationsDBAdapter_Factory.get(TranslationsDBAdapter_Factory.java:9)
  at dagger.internal.DoubleCheck.get(DoubleCheck.java:47)
  at com.quran.labs.androidquran.presenter.translation.TranslationManagerPresenter_Factory.get(TranslationManagerPresenter_Factory.java:40)
  at com.quran.labs.androidquran.presenter.translation.TranslationManagerPresenter_Factory.get(TranslationManagerPresenter_Factory.java:12)
  at dagger.internal.DoubleCheck.get(DoubleCheck.java:47)
  at com.quran.labs.androidquran.di.component.application.DaggerApplicationComponent.injectQuranActivity(DaggerApplicationComponent.java:494)
  at com.quran.labs.androidquran.di.component.application.DaggerApplicationComponent.inject(DaggerApplicationComponent.java:395)
  at com.quran.labs.androidquran.ui.QuranActivity.onCreate(QuranActivity.kt:98)
  at android.app.Activity.performCreate(Activity.java:7825)
  at android.app.Activity.performCreate(Activity.java:7814)
  at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1306)
  at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3245)
  at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3409) 
  at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:83) 
  at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) 
  at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) 
  at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016) 
  at android.os.Handler.dispatchMessage(Handler.java:107) 
  at android.os.Looper.loop(Looper.java:214) 
  at android.app.ActivityThread.main(ActivityThread.java:7356) 
  at java.lang.reflect.Method.invoke(Native Method) 
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) 
  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

papasmile · 2020-07-17T13:03:18Z

Salam, @ozbek yikes, I dunno why that * wasn't crashing * for me but I see I forgot to initialize cursor. Added that and tested a reinstall, still good for me... please give another go.

ozbek · 2020-07-20T05:51:46Z

Salam.

Just tried it out and there are two UX issues that I can see:

Every time up/down button is clicked, selection is cleared: this is especially inconvenient if there are many translations and you are trying to move the firs/last item to the bottom/top.
~~Select a downloaded translation -> Observe that up/down/delete menus appear -> Swipe-to-refresh -> Selection cleared, but the menus are still there.~~ Must have gotten confused somewhere at that time, cannot reproduce now )

papasmile · 2020-07-20T13:03:40Z

Salam, @ozbek, great. Second one sounds like a bug, will fix. First was was intentionally a feature to keep things simple but happy to change it up to what you suggest.

ahmedre · 2020-07-21T23:33:14Z

salam 3alaikum,
tested this (though didn't look at the code yet) - agree with @ozbek about the first one also - i tried this with a list of 5 translations and it was a bit of trouble.

nice to have (but definitely not required for merging this) - one other thing is that would be nice is if we're adding multi-selection mode, it would be great that if a second item is selected, instead of canceling the multi-selection mode, just hide the up/down buttons and leave the deletion to work for multiple items.

ahmedre · 2020-07-21T23:34:03Z

app/src/main/java/com/quran/labs/androidquran/common/LocalTransationDiplaySort.kt

+package com.quran.labs.androidquran.common
+
+class LocalTransationDiplaySort : Comparator<LocalTranslation> {
+  override fun compare(p0: LocalTranslation?, p1: LocalTranslation?): Int {


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

yup, good call

ahmedre · 2020-07-21T23:35:15Z

app/src/main/java/com/quran/labs/androidquran/dao/translation/TranslationItemDiplaySort.kt

+package com.quran.labs.androidquran.dao.translation
+
+class TranslationItemDiplaySort : Comparator<TranslationItem> {
+  override fun compare(p0: TranslationItem?, p1: TranslationItem?): Int {


similarly, can these actually be nullable?

ahmedre · 2020-07-21T23:35:59Z

app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBAdapter.java

+            displayOrder = item.getDisplayOrder();
+          } else {
+            // get next highest display order
+            Cursor cursor = db.query(


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

ahmedre · 2020-07-21T23:39:02Z

app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBHelper.java

+            + TranslationsTable.DISPLAY_ORDER
+            + " integer not null default -1"
+        );
+        Cursor translations = db.query(


+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

papasmile · 2020-07-22T16:42:59Z

Salam, @ozbek, I cannot reproduce that behavior on S8, selection stays even with refresh. Can you tell me what phone/android version you have?

papasmile · 2020-07-22T16:55:55Z

Salam, @ahmedre, appreciate your review and will make some updates. I like your suggestion for multi-select delete; but I think it will be some effort to expand my existing changes so I suggest we open up another issue (now or once merged).

papasmile · 2020-07-22T17:20:04Z

Also @ahmedre re: #1398 (comment) there is somewhat of an edge case in present logic, that is, if items get deleted in the middle of user's display order there is a 'gap' and they will end up having to move something twice; a choice I made for performance. But it also means using 'id' as display order could result in a lot of 'gaps'... so would suggest to keep a simple for loop.

ahmedre · 2020-07-22T21:29:00Z

won't the gap exist anyway irrespective of the upgrade logic today? like anytime i delete one in the middle, i'd either have to update everything or just expect that a gap would exist? if so, can we just rely on sort order for ordering only but not for moving?

papasmile · 2020-07-23T13:31:25Z

Salam, @ahmedre , I am in fact relying on sort order for moving, as well. What I mean is that when we do an upgrade there are no sort/display order values yet. So taking advantage of that I make them all contiguous so there are no gaps. I hope that make sense (you are also welcome to IM me on Slack if that's easier :-))

papasmile · 2020-07-23T15:44:01Z

Salam, I think I've addressed all feedback. Latest commit includes:

Adding finally block for all Translation db cursors (including previous code)
Moving does not de-select
Simplified comparators

papasmile · 2020-07-23T15:44:23Z

Please try it out!

ozbek · 2020-07-24T08:25:00Z

app/src/main/java/com/quran/labs/androidquran/common/LocalTransationDiplaySort.kt

@@ -0,0 +1,7 @@
+package com.quran.labs.androidquran.common
+
+class LocalTransationDiplaySort : Comparator<LocalTranslation> {


Who ate the s? :)

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

ahmedre

if we wanted to be more efficient, one thing we could (and we probably don't need to do this right now since this is a rare operation anyway), we could avoid updating all items by leaving gaps in between the items - so instead of display order 1, 2, 3, 4 - we could do display order 1000, 2000, 3000, 4000. if item 4 moves to the very beginning, we just update it to 999 and don't need to update any of the others.

ahmedre · 2020-07-24T14:13:46Z

app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBAdapter.java

+            displayOrder = item.getDisplayOrder();
+          } else {
+            // get next highest display order
+            Cursor cursor = db.query(


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

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

ahmedre · 2020-07-24T14:14:01Z

app/src/main/java/com/quran/labs/androidquran/database/TranslationsDBAdapter.java

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


see comment below - try should move up though

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

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.

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?

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.

ahmedre · 2020-07-24T14:17:51Z

app/src/main/java/com/quran/labs/androidquran/ui/TranslationManagerActivity.java

+    List<TranslationItem> sortedDownloads = sortedDownloadedItems();
+    if (sortedDownloads.indexOf(targetItem) + 1 < sortedDownloads.size()) { // ignore last item in list
+      TranslationItem updatedItem = targetItem.withDisplayOrder(targetItem.getDisplayOrder() + 1);
+      ArrayList<TranslationItem> toUpdate = new ArrayList<>();


question - why do we have to do this? why not assume that the result from sortDownloadedItems() is the sort ordering - so basically when you choose item 3 and move up, you just swap positions 2 and 3 without having to do these loops.

Again, for what was mentioned previously that there may be gaps when an item is deleted. Alternatively, shifting logic could be moved to download routine... but that seems a lot trickier at first glance.

ahmedre · 2020-07-24T14:18:17Z

app/src/main/java/com/quran/labs/androidquran/ui/TranslationManagerActivity.java

+    TranslationItem targetItem = (TranslationItem) targetRow;
+    List<TranslationItem> sortedDownloads = sortedDownloadedItems();
+    if (sortedDownloads.indexOf(targetItem) > 0) { // ignore first item in list
+      ArrayList<TranslationItem> toUpdate  = new ArrayList<>();


ahmedre · 2020-07-24T14:24:12Z

app/src/main/java/com/quran/labs/androidquran/ui/TranslationManagerActivity.java

+      if (!toUpdate.isEmpty()) {
+        if (selectionListener != null) selectionListener.handleSelection(updatedItem);
+        for(TranslationItem toUpdateItem : toUpdate){
+          updateTranslationItem(toUpdateItem);
+        }
+        generateListItems();
+      }


this is expensive because every move is a database query when it doesn't need to be. we should just update when action mode is closed.

Hm, updateTranslationItem updates both our activity data and database in one go. But, yea, it's probably not necessary... then we'll have to break up updateTranslationItem for use in moving translations.

papasmile · 2020-07-27T13:25:26Z

Salam, don't quite understand build failure... is there away to re-run a build?

ahmedre · 2020-08-01T11:05:55Z

wa3laikum alsalam,
don't worry about this failure - sometimes running the ui tests is flakey.
walsalam 3alaikum.

ahmedre · 2020-08-08T13:34:57Z

@papasmile masha'Allah this is really close to merging - can we see about being more efficient instead of calling the database calls per move?

ahmedre · 2020-09-29T05:51:30Z

jazakumAllah khair - made some of these changes and merged.

papasmile · 2020-10-21T02:45:42Z

asslaam alaykum, @ahmedre , et al, yay, yes was just on a few last changes and got majorly busy! Thanks much for accepting and may He Azza wa Jal accept from all of us!

papasmile force-pushed the 1179_order_translations branch from 6e2c6b2 to aa72ffa Compare July 7, 2020 15:18

Support Reordering Of Translations. Fixes quran#1179

4b6c4a9

papasmile force-pushed the 1179_order_translations branch from aa72ffa to 4b6c4a9 Compare July 8, 2020 15:29

ozbek suggested changes Jul 9, 2020

View reviewed changes

cleanup

b8ac99e

Initialize Cursor

25dd889

ahmedre reviewed Jul 21, 2020

View reviewed changes

cleanup and multi-move

b7f5aca

ozbek reviewed Jul 24, 2020

View reviewed changes

spelling

604b9bf

ahmedre reviewed Jul 24, 2020

View reviewed changes

ahmedre merged commit 604b9bf into quran:master Sep 29, 2020

papasmile deleted the 1179_order_translations branch October 21, 2020 02:45

papasmile restored the 1179_order_translations branch October 21, 2020 02:45

		@@ -0,0 +1,7 @@
		package com.quran.labs.androidquran.common

		class LocalTransationDiplaySort : Comparator<LocalTranslation> {

Support Reordering Of Translations. Fixes #1179 #1398

Support Reordering Of Translations. Fixes #1179 #1398

Conversation

papasmile commented Jul 7, 2020 • edited Loading

papasmile commented Jul 7, 2020

ozbek commented Jul 8, 2020

papasmile commented Jul 8, 2020

ozbek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papasmile Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozbek Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

papasmile commented Jul 16, 2020

ozbek commented Jul 17, 2020

papasmile commented Jul 17, 2020

ozbek commented Jul 20, 2020 • edited Loading

papasmile commented Jul 20, 2020

ahmedre commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papasmile commented Jul 22, 2020

papasmile commented Jul 22, 2020

papasmile commented Jul 22, 2020

ahmedre commented Jul 22, 2020

papasmile commented Jul 23, 2020

papasmile commented Jul 23, 2020

papasmile commented Jul 23, 2020

Choose a reason for hiding this comment

papasmile Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

ahmedre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papasmile commented Jul 27, 2020

ahmedre commented Aug 1, 2020

ahmedre commented Aug 8, 2020

ahmedre commented Sep 29, 2020

papasmile commented Oct 21, 2020

papasmile commented Jul 7, 2020 •

edited

Loading

papasmile Jul 9, 2020 •

edited

Loading

ozbek Jul 10, 2020 •

edited

Loading

ozbek commented Jul 20, 2020 •

edited

Loading

papasmile Jul 24, 2020 •

edited

Loading