Remove data set change animation in edit screen, fix concurrency bug …

…during fragment recreation

- Change AtomicBoolean to more appropriate job joins.

app/src/main/kotlin/com/maltaisn/notes/ui/edit/EditFragment.kt

@@ -28,7 +28,9 @@ import androidx.core.view.postDelayed
 import androidx.fragment.app.Fragment
 import androidx.navigation.fragment.findNavController
 import androidx.navigation.fragment.navArgs
+import androidx.recyclerview.widget.DefaultItemAnimator
 import androidx.recyclerview.widget.LinearLayoutManager
+import androidx.recyclerview.widget.RecyclerView
 import com.google.android.material.snackbar.Snackbar
 import com.maltaisn.notes.App
 import com.maltaisn.notes.hideKeyboard
@@ -116,7 +118,25 @@ class EditFragment : Fragment(), Toolbar.OnMenuItemClickListener, ConfirmDialog.
         val layoutManager = LinearLayoutManager(context)
         rcv.adapter = adapter
         rcv.layoutManager = layoutManager
-        rcv.itemAnimator?.changeDuration = 0
+        rcv.itemAnimator = object : DefaultItemAnimator() {
+            override fun animateAppearance(
+                viewHolder: RecyclerView.ViewHolder,
+                preLayoutInfo: ItemHolderInfo?,
+                postLayoutInfo: ItemHolderInfo
+            ): Boolean {
+                return if (preLayoutInfo != null && (preLayoutInfo.left != postLayoutInfo.left
+                            || preLayoutInfo.top != postLayoutInfo.top)
+                ) {
+                    // item move, handle normally
+                    super.animateAppearance(viewHolder, preLayoutInfo, postLayoutInfo)
+                } else {
+                    // do not animate new item appearance
+                    // this is mainly to avoid animating the whole list when fragment view is recreated.
+                    dispatchAddFinished(viewHolder)
+                    false
+                }
+            }
+        }
 
         setupViewModelObservers(adapter)
     }

app/src/main/kotlin/com/maltaisn/notes/ui/edit/EditViewModel.kt

@@ -52,14 +52,14 @@ import com.maltaisn.notes.ui.note.ShownDateField
 import com.maltaisn.notes.ui.send
 import com.squareup.inject.assisted.Assisted
 import com.squareup.inject.assisted.AssistedInject
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.NonCancellable
 import kotlinx.coroutines.launch
 import java.util.Collections
 import java.util.Date
 
 /**
  * View model for the edit note screen.
- * TODO status, pinned, reminder could be livedata, except that livedata has nullable type... flow?
  */
 class EditViewModel @AssistedInject constructor(
     private val notesRepository: NotesRepository,
@@ -106,7 +106,15 @@ class EditViewModel @AssistedInject constructor(
 
     /**
      * The currently displayed list items created in [updateListItems].
-     * While this list is mutable, any in place changes should be reported to the adapter!
+     *
+     * While this list is mutable, any in place changes should be reported to the adapter! This is used in the case
+     * of moving items, where the view model updates the list but the adapter already knows of the move.
+     *
+     * Note the in the case of list items, a specific item has no identity. Its position and its content
+     * can change at any time, so it can be associated with a stable ID. This is problematic for item diff callback
+     * and animations, which would rely on ID. Instead, the diff callback was made to rely on identity so that
+     * add/remove animations take place correctly. The recycler view was also set up to not animate item appearance,
+     * so that if the list is completely recreated, no animation will occur despite all items identity being lost.
      */
     private var listItems: MutableList<EditListItem> = mutableListOf()
         set(value) {
@@ -182,6 +190,8 @@ class EditViewModel @AssistedInject constructor(
     private val isNoteInTrash: Boolean
         get() = status == NoteStatus.DELETED
 
+    private var updateNoteJob: Job? = null
+
     init {
         if (KEY_NOTE_ID in savedStateHandle) {
             viewModelScope.launch {
@@ -205,6 +215,10 @@ class EditViewModel @AssistedInject constructor(
      */
     fun start(noteId: Long = Note.NO_ID, labelId: Long = Label.NO_ID) {
         viewModelScope.launch {
+            // If fragment was very briefly destroyed then recreated, it's possible that this job is launched
+            // before the job to save the note on fragment destruction is called.
+            updateNoteJob?.join()
+
             val isFirstStart = (note == BLANK_NOTE)
 
             // Try to get note by ID with its labels.
@@ -268,8 +282,8 @@ class EditViewModel @AssistedInject constructor(
         // Update note
         updateNote()
 
-        // NonCancellable to avoid save being cancelled if called right before view model destruction
-        viewModelScope.launch(NonCancellable) {
+        // NonCancellable to avoid save being cancelled if called right before fragment destruction
+        updateNoteJob = viewModelScope.launch(NonCancellable) {
             // Compare previously saved note from database with new one.
             // It is possible that note will be null here, if:
             // - Back arrow is clicked, saving note.
@@ -288,6 +302,7 @@ class EditViewModel @AssistedInject constructor(
                 }
 
                 notesRepository.updateNote(note)
+                updateNoteJob = null
             }
         }
     }
@@ -560,9 +575,6 @@ class EditViewModel @AssistedInject constructor(
      * [updateNote] might need to be called first for that data to be up-to-date.
      */
     private fun updateListItems() {
-        // TODO avoid full list refresh somehow?
-        //  - on note type change
-        //  - on fragment view recreation
         listItems = createListItems()
     }
 

app/src/main/kotlin/com/maltaisn/notes/ui/home/HomeViewModel.kt

@@ -82,9 +82,7 @@ class HomeViewModel @AssistedInject constructor(
         get() = _showEmptyTrashDialogEvent
 
     init {
-        viewModelScope.launch {
-            restoreState()
-        }
+        restoreState()
     }
 
     fun setDestination(destination: HomeDestination) {

app/src/main/kotlin/com/maltaisn/notes/ui/labels/LabelViewModel.kt

@@ -34,8 +34,6 @@ import com.squareup.inject.assisted.AssistedInject
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.flow.collect
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.yield
-import java.util.concurrent.atomic.AtomicBoolean
 
 class LabelViewModel @AssistedInject constructor(
     private val labelsRepository: LabelsRepository,
@@ -109,17 +107,16 @@ class LabelViewModel @AssistedInject constructor(
     private var renamingLabel = false
 
     private var labelsListJob: Job? = null
-
-    private val stateRestored = AtomicBoolean(false)
+    private var restoreStateJob: Job? = null
 
     init {
-        viewModelScope.launch {
+        restoreStateJob = viewModelScope.launch {
             // Restore state
             noteIds = savedStateHandle.get<List<Long>>(KEY_NOTE_IDS).orEmpty()
             selectedLabelIds += savedStateHandle.get<List<Long>>(KEY_SELECTED_IDS).orEmpty()
             selectedLabels += selectedLabelIds.mapNotNull { labelsRepository.getLabelById(it) }
             renamingLabel = savedStateHandle[KEY_RENAMING_LABEL] ?: false
-            stateRestored.set(true)
+            restoreStateJob = null
         }
     }
 
@@ -148,9 +145,7 @@ class LabelViewModel @AssistedInject constructor(
             labelsRepository.getAllLabelsByUsage().collect { labels ->
                 // Since state restoration is suspending, ensure that state is properly restored
                 // (e.g. selection) before setting list items, or selection may be lost.
-                while (!stateRestored.get()) {
-                    yield()
-                }
+                restoreStateJob?.join()
 
                 if (renamingLabel) {
                     // List was updated after renaming label, this can only be due to label rename.
@@ -295,5 +290,4 @@ class LabelViewModel @AssistedInject constructor(
         private const val KEY_SELECTED_IDS = "selected_ids"
         private const val KEY_RENAMING_LABEL = "renaming_label"
     }
-
 }

app/src/main/kotlin/com/maltaisn/notes/ui/note/NoteViewModel.kt

@@ -38,10 +38,9 @@ import com.maltaisn.notes.ui.note.adapter.NoteItem
 import com.maltaisn.notes.ui.note.adapter.NoteListItem
 import com.maltaisn.notes.ui.note.adapter.NoteListLayoutMode
 import com.maltaisn.notes.ui.send
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.yield
 import java.util.Date
-import java.util.concurrent.atomic.AtomicBoolean
 
 /**
  * This view model provides common behavior for home and search view models.
@@ -132,7 +131,7 @@ abstract class NoteViewModel(
     val showDeleteConfirmEvent: LiveData<Event<Unit>>
         get() = _showDeletedForeverConfirmEvent
 
-    private var stateRestored = AtomicBoolean(false)
+    private var restoreStateJob: Job? = null
 
     init {
         // Initialize list layout to saved value.
@@ -145,19 +144,19 @@ abstract class NoteViewModel(
      * Notice that state restoration is suspending, so when initializing the child view model,
      * [waitForRestoredState] must be called to wait for state restoration to be complete.
      */
-    protected open suspend fun restoreState() {
-        // Restore saved selected notes
-        selectedNoteIds += savedStateHandle.get<List<Long>>(KEY_SELECTED_IDS)
-            .orEmpty().toMutableSet()
-        _selectedNotes += selectedNoteIds.mapNotNull { notesRepository.getNoteById(it) }
-        updateNoteSelection()
-        stateRestored.set(true)
+    protected open fun restoreState() {
+        restoreStateJob = viewModelScope.launch {
+            // Restore saved selected notes
+            selectedNoteIds += savedStateHandle.get<List<Long>>(KEY_SELECTED_IDS)
+                .orEmpty().toMutableSet()
+            _selectedNotes += selectedNoteIds.mapNotNull { notesRepository.getNoteById(it) }
+            updateNoteSelection()
+            restoreStateJob = null
+        }
     }
 
     protected suspend fun waitForRestoredState() {
-        while (!stateRestored.get()) {
-            yield()
-        }
+        restoreStateJob?.join()
     }
 
     /**

app/src/main/kotlin/com/maltaisn/notes/ui/notification/NotificationActivity.kt

@@ -18,12 +18,10 @@ package com.maltaisn.notes.ui.notification
 
 import android.content.Intent
 import android.os.Bundle
-import android.util.Log
 import androidx.appcompat.app.AppCompatActivity
 import androidx.core.app.NotificationManagerCompat
 import androidx.navigation.findNavController
 import com.maltaisn.notes.App
-import com.maltaisn.notes.TAG
 import com.maltaisn.notes.model.entity.Note
 import com.maltaisn.notes.navigateSafe
 import com.maltaisn.notes.receiver.AlarmReceiver
@@ -68,7 +66,6 @@ class NotificationActivity : AppCompatActivity() {
             when (intent.action) {
                 INTENT_ACTION_POSTPONE -> {
                     val noteId = intent.getLongExtra(AlarmReceiver.EXTRA_NOTE_ID, Note.NO_ID)
-                    Log.d(TAG, "note ID for postpone is $noteId")
                     NotificationManagerCompat.from(this).cancel(noteId.toInt())
                     viewModel.onPostponeClicked(noteId)
                 }

app/src/main/kotlin/com/maltaisn/notes/ui/reminder/ReminderDialog.kt

@@ -19,7 +19,6 @@ package com.maltaisn.notes.ui.reminder
 import android.app.Dialog
 import android.content.DialogInterface
 import android.os.Bundle
-import android.util.Log
 import android.view.LayoutInflater
 import androidx.appcompat.app.AlertDialog
 import androidx.core.view.isVisible
@@ -28,7 +27,6 @@ import androidx.navigation.fragment.findNavController
 import androidx.navigation.fragment.navArgs
 import com.google.android.material.dialog.MaterialAlertDialogBuilder
 import com.maltaisn.notes.App
-import com.maltaisn.notes.TAG
 import com.maltaisn.notes.contains
 import com.maltaisn.notes.navigateSafe
 import com.maltaisn.notes.setMaxWidth
@@ -67,7 +65,6 @@ class ReminderDialog : DialogFragment(), RecurrenceListCallback, RecurrencePicke
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
         (requireContext().applicationContext as App).appComponent.inject(this)
-        Log.d(TAG, "Creating ReminderDialog fragment")
     }
 
     override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {