From 25ccee37322cd3acdb7db017174933a40d2b07e2 Mon Sep 17 00:00:00 2001 From: Jarvis Lin Date: Fri, 16 Feb 2024 16:07:15 +0800 Subject: [PATCH 1/7] Fix the thread issue by using DiffUtil --- .../ui/notifications/adapters/NotesAdapter.kt | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt index 29abdf952946..f3fd4bf16188 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt @@ -20,6 +20,7 @@ import androidx.core.text.BidiFormatter import androidx.core.view.ViewCompat import androidx.core.view.isVisible import androidx.core.widget.ImageViewCompat +import androidx.recyclerview.widget.DiffUtil import androidx.recyclerview.widget.RecyclerView import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -99,15 +100,22 @@ class NotesAdapter( currentFilter = newFilter } - @SuppressLint("NotifyDataSetChanged") fun addAll(notes: List) { - try { - filteredNotes.clear() - filteredNotes.addAll(buildFilteredNotesList(notes, currentFilter)) - } finally { - notifyDataSetChanged() - dataLoadedListener.onDataLoaded(itemCount) - } + val newNotes = buildFilteredNotesList(notes, currentFilter) + val result = DiffUtil.calculateDiff(object : DiffUtil.Callback() { + override fun getOldListSize(): Int = filteredNotes.size + override fun getNewListSize(): Int = newNotes.size + override fun areItemsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean = + filteredNotes[oldItemPosition].id == newNotes[newItemPosition].id + + override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean = + filteredNotes[oldItemPosition].json.toString() == newNotes[newItemPosition].json.toString() + }) + + filteredNotes.clear() + filteredNotes.addAll(newNotes) + result.dispatchUpdatesTo(this) + dataLoadedListener.onDataLoaded(itemCount) } override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): NoteViewHolder = From e69e8e77bcdd0d8d8b808e1786a207b0b75dd3fa Mon Sep 17 00:00:00 2001 From: Jarvis Lin Date: Fri, 16 Feb 2024 17:17:34 +0800 Subject: [PATCH 2/7] Reduce API requests --- .../ui/notifications/NotificationsListFragmentPage.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt index 36d1a996cbde..7dc86fc49d0f 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt @@ -117,14 +117,14 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) (requireActivity().application as WordPress).component().inject(this) - shouldRefreshNotifications = true + arguments?.let { + tabPosition = it.getInt(KEY_TAB_POSITION, All.ordinal) + } + shouldRefreshNotifications = tabPosition == 0 } override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) - arguments?.let { - tabPosition = it.getInt(KEY_TAB_POSITION, All.ordinal) - } notesAdapter = NotesAdapter( requireActivity(), this, null, inlineActionEvents = viewModel.inlineActionEvents).apply { this.setOnNoteClickListener(mOnNoteClickListener) From 2e18cfa079b8c6b9c97ab8abc0022de6be1cfe82 Mon Sep 17 00:00:00 2001 From: Jarvis Lin Date: Fri, 16 Feb 2024 17:21:07 +0800 Subject: [PATCH 3/7] Refactor the deprecated AsyncTask --- .../NotificationsListFragmentPage.kt | 6 +-- .../ui/notifications/adapters/NotesAdapter.kt | 46 ++++++++----------- 2 files changed, 22 insertions(+), 30 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt index 7dc86fc49d0f..c4978ebe7c38 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt @@ -150,7 +150,7 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l override fun onDestroyView() { super.onDestroyView() - notesAdapter.cancelReloadNotesTask() + notesAdapter.cancelReloadLocalNotes() swipeToRefreshHelper = null binding?.notificationsList?.adapter = null binding?.notificationsList?.removeCallbacks(showNewUnseenNotificationsRunnable) @@ -187,7 +187,7 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l binding?.hideNewNotificationsBar() EventBus.getDefault().post(NotificationsUnseenStatus(false)) if (accountStore.hasAccessToken()) { - notesAdapter.reloadNotesFromDBAsync() + notesAdapter.reloadLocalNotes() if (shouldRefreshNotifications) { fetchNotesFromRemote() } @@ -459,7 +459,7 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l if (!isAdded) { return } - notesAdapter.reloadNotesFromDBAsync() + notesAdapter.reloadLocalNotes() if (event.hasUnseenNotes) { binding?.showNewUnseenNotificationsUI() } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt index f3fd4bf16188..449e4780f21c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt @@ -2,10 +2,8 @@ package org.wordpress.android.ui.notifications.adapters -import android.annotation.SuppressLint import android.content.Context import android.content.res.ColorStateList -import android.os.AsyncTask import android.text.Spanned import android.text.TextUtils import android.view.LayoutInflater @@ -24,8 +22,10 @@ import androidx.recyclerview.widget.DiffUtil import androidx.recyclerview.widget.RecyclerView import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import org.wordpress.android.R import org.wordpress.android.WordPress import org.wordpress.android.datasets.NotificationsTable @@ -57,6 +57,7 @@ class NotesAdapter( private val textIndentSize: Int private val dataLoadedListener: DataLoadedListener private val onLoadMoreListener: OnLoadMoreListener? + private val coroutineScope = CoroutineScope(Dispatchers.IO) val filteredNotes = ArrayList() @Inject @@ -85,7 +86,7 @@ class NotesAdapter( var currentFilter = FILTERS.FILTER_ALL private set - private var reloadNotesFromDBTask: ReloadNotesFromDBTask? = null + private var reloadLocalNotesJob: Job? = null interface DataLoadedListener { fun onDataLoaded(itemsCount: Int) @@ -100,6 +101,9 @@ class NotesAdapter( currentFilter = newFilter } + /** + * Add a note to the adapter and notify the change + */ fun addAll(notes: List) { val newNotes = buildFilteredNotesList(notes, currentFilter) val result = DiffUtil.calculateDiff(object : DiffUtil.Callback() { @@ -111,7 +115,7 @@ class NotesAdapter( override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean = filteredNotes[oldItemPosition].json.toString() == newNotes[newItemPosition].json.toString() }) - + filteredNotes.clear() filteredNotes.addAll(newNotes) result.dispatchUpdatesTo(this) @@ -129,7 +133,6 @@ class NotesAdapter( private fun isValidPosition(position: Int): Boolean = position >= 0 && position < filteredNotes.size - override fun getItemCount(): Int = filteredNotes.size private val Note.timeGroup @@ -286,17 +289,19 @@ class NotesAdapter( onNoteClickListener = mNoteClickListener } - fun cancelReloadNotesTask() { - if (reloadNotesFromDBTask != null && reloadNotesFromDBTask!!.status != AsyncTask.Status.FINISHED) { - reloadNotesFromDBTask!!.cancel(true) - reloadNotesFromDBTask = null - } + fun cancelReloadLocalNotes() { + reloadLocalNotesJob?.cancel() } - fun reloadNotesFromDBAsync() { - cancelReloadNotesTask() - reloadNotesFromDBTask = ReloadNotesFromDBTask() - reloadNotesFromDBTask!!.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR) + /** + * Reload the notes from local database and update the adapter + */ + fun reloadLocalNotes() { + cancelReloadLocalNotes() + reloadLocalNotesJob = coroutineScope.launch { + val notes = NotificationsTable.getLatestNotes() + withContext(Dispatchers.Main) { addAll(notes) } + } } /** @@ -310,17 +315,6 @@ class NotesAdapter( } } - @SuppressLint("StaticFieldLeak") - private inner class ReloadNotesFromDBTask : AsyncTask>() { - override fun doInBackground(vararg voids: Void?): ArrayList { - return NotificationsTable.getLatestNotes() - } - - override fun onPostExecute(notes: ArrayList) { - addAll(notes) - } - } - inner class NoteViewHolder(view: View) : RecyclerView.ViewHolder(view) { val contentView: View val headerText: TextView @@ -338,8 +332,6 @@ class NotesAdapter( val unreadNotificationView: View val actionIcon: ImageView - val coroutineScope = CoroutineScope(Dispatchers.Main) - init { contentView = checkNotNull(view.findViewById(R.id.note_content_container)) headerText = checkNotNull(view.findViewById(R.id.header_text)) From cc3dfe14f308b008ab1f27f26fd78b37a72f121f Mon Sep 17 00:00:00 2001 From: Jarvis Lin Date: Fri, 16 Feb 2024 18:24:46 +0800 Subject: [PATCH 4/7] Refactor with Coroutine --- .../NotificationsListFragmentPage.kt | 74 ++++++++++--------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt index c4978ebe7c38..e5e48c5f7141 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt @@ -17,8 +17,11 @@ import androidx.recyclerview.widget.LinearLayoutManager import androidx.recyclerview.widget.RecyclerView import androidx.recyclerview.widget.RecyclerView.OnScrollListener import dagger.hilt.android.AndroidEntryPoint +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import org.greenrobot.eventbus.EventBus import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode.MAIN @@ -75,7 +78,6 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l private lateinit var notesAdapter: NotesAdapter private var swipeToRefreshHelper: SwipeToRefreshHelper? = null private var isAnimatingOutNewNotificationsBar = false - private var shouldRefreshNotifications = false private var tabPosition = 0 private val viewModel: NotificationsListViewModel by viewModels() @@ -103,7 +105,6 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l @Suppress("DEPRECATION", "OVERRIDE_DEPRECATION") override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?) { if (requestCode == RequestCodes.NOTE_DETAIL) { - shouldRefreshNotifications = false if (resultCode == Activity.RESULT_OK) { val noteId = data?.getStringExtra(NotificationsListFragment.NOTE_MODERATE_ID_EXTRA) val newStatus = data?.getStringExtra(NotificationsListFragment.NOTE_MODERATE_STATUS_EXTRA) @@ -120,7 +121,6 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l arguments?.let { tabPosition = it.getInt(KEY_TAB_POSITION, All.ordinal) } - shouldRefreshNotifications = tabPosition == 0 } override fun onViewCreated(view: View, savedInstanceState: Bundle?) { @@ -137,7 +137,7 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l notificationsList.adapter = notesAdapter swipeToRefreshHelper = WPSwipeToRefreshHelper.buildSwipeToRefreshHelper(notificationsRefresh) { hideNewNotificationsBar() - fetchNotesFromRemote() + fetchRemoteNotes() } layoutNewNotificatons.visibility = View.GONE layoutNewNotificatons.setOnClickListener { onScrollToTop() } @@ -146,6 +146,13 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l viewModel.updatedNote.observe(viewLifecycleOwner) { notesAdapter.updateNote(it) } + + if (tabPosition == All.ordinal) { + swipeToRefreshHelper?.isRefreshing = true + fetchRemoteNotes() + } else { + notesAdapter.reloadLocalNotes() + } } override fun onDestroyView() { @@ -173,11 +180,6 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l } } - override fun onPause() { - super.onPause() - shouldRefreshNotifications = true - } - override fun getScrollableViewForUniqueIdProvision(): View? { return binding?.notificationsList } @@ -186,12 +188,6 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l super.onResume() binding?.hideNewNotificationsBar() EventBus.getDefault().post(NotificationsUnseenStatus(false)) - if (accountStore.hasAccessToken()) { - notesAdapter.reloadLocalNotes() - if (shouldRefreshNotifications) { - fetchNotesFromRemote() - } - } } override fun onSaveInstanceState(outState: Bundle) { @@ -248,11 +244,15 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l private fun NotificationsListFragmentPageBinding.clearPendingNotificationsItemsOnUI() { hideNewNotificationsBar() EventBus.getDefault().post(NotificationsUnseenStatus(false)) - NotificationsActions.updateNotesSeenTimestamp() - Thread { gcmMessageHandler.removeAllNotifications(activity) }.start() + lifecycleScope.launch { + withContext(Dispatchers.IO) { + NotificationsActions.updateNotesSeenTimestamp() + gcmMessageHandler.removeAllNotifications(activity) + } + } } - private fun fetchNotesFromRemote() { + private fun fetchRemoteNotes() { if (!isAdded) { return } @@ -405,12 +405,14 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l } } - private fun updateNote(noteId: String, status: CommentStatus) { - val note = NotificationsTable.getNoteById(noteId) - if (note != null) { - note.localStatus = status.toString() - NotificationsTable.saveNote(note) - EventBus.getDefault().post(NotificationsChanged()) + private fun updateNote(noteId: String, status: CommentStatus) = lifecycleScope.launch { + withContext(Dispatchers.IO) { + val note = NotificationsTable.getNoteById(noteId) + if (note != null) { + note.localStatus = status.toString() + NotificationsTable.saveNote(note) + EventBus.getDefault().post(NotificationsChanged()) + } } } @@ -439,22 +441,26 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l @Subscribe(sticky = true, threadMode = MAIN) fun onEventMainThread(event: NoteLikeOrModerationStatusChanged) { - NotificationsActions.downloadNoteAndUpdateDB( - event.noteId, - { - EventBus.getDefault() - .removeStickyEvent( + lifecycleScope.launch { + withContext(Dispatchers.IO) { + NotificationsActions.downloadNoteAndUpdateDB( + event.noteId, + { + EventBus.getDefault() + .removeStickyEvent( + NoteLikeOrModerationStatusChanged::class.java + ) + } + ) { + EventBus.getDefault().removeStickyEvent( NoteLikeOrModerationStatusChanged::class.java ) + } } - ) { - EventBus.getDefault().removeStickyEvent( - NoteLikeOrModerationStatusChanged::class.java - ) } } - @Subscribe(threadMode = MAIN) + @Subscribe(sticky = true, threadMode = MAIN) fun onEventMainThread(event: NotificationsChanged) { if (!isAdded) { return From 81c415591af80d65a66039c8df677d01d60c08e5 Mon Sep 17 00:00:00 2001 From: Jarvis Lin Date: Fri, 16 Feb 2024 19:00:51 +0800 Subject: [PATCH 5/7] Make refactors --- .../ui/notifications/NotificationsListFragmentPage.kt | 6 +++--- .../android/ui/notifications/adapters/NotesAdapter.kt | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt index e5e48c5f7141..ea5951b1d9c9 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt @@ -118,13 +118,13 @@ class NotificationsListFragmentPage : ViewPagerFragment(R.layout.notifications_l override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) (requireActivity().application as WordPress).component().inject(this) - arguments?.let { - tabPosition = it.getInt(KEY_TAB_POSITION, All.ordinal) - } } override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) + arguments?.let { + tabPosition = it.getInt(KEY_TAB_POSITION, All.ordinal) + } notesAdapter = NotesAdapter( requireActivity(), this, null, inlineActionEvents = viewModel.inlineActionEvents).apply { this.setOnNoteClickListener(mOnNoteClickListener) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt index 449e4780f21c..6a4f6f5514bf 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt @@ -102,7 +102,7 @@ class NotesAdapter( } /** - * Add a note to the adapter and notify the change + * Add notes to the adapter and notify the change */ fun addAll(notes: List) { val newNotes = buildFilteredNotesList(notes, currentFilter) From 055a34af4aa1c9410071f0cf0a3393a56c5a8dea Mon Sep 17 00:00:00 2001 From: Jarvis Lin Date: Fri, 16 Feb 2024 19:06:56 +0800 Subject: [PATCH 6/7] Use the background thread for calculating the diff --- .../android/ui/notifications/adapters/NotesAdapter.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt index 6a4f6f5514bf..8655c88063c2 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt @@ -104,7 +104,7 @@ class NotesAdapter( /** * Add notes to the adapter and notify the change */ - fun addAll(notes: List) { + fun addAll(notes: List) = coroutineScope.launch { val newNotes = buildFilteredNotesList(notes, currentFilter) val result = DiffUtil.calculateDiff(object : DiffUtil.Callback() { override fun getOldListSize(): Int = filteredNotes.size @@ -115,11 +115,13 @@ class NotesAdapter( override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean = filteredNotes[oldItemPosition].json.toString() == newNotes[newItemPosition].json.toString() }) - + filteredNotes.clear() filteredNotes.addAll(newNotes) - result.dispatchUpdatesTo(this) - dataLoadedListener.onDataLoaded(itemCount) + withContext(Dispatchers.Main) { + result.dispatchUpdatesTo(this@NotesAdapter) + dataLoadedListener.onDataLoaded(itemCount) + } } override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): NoteViewHolder = From 75b431df6d07d3093ec9f353daa317148410e03e Mon Sep 17 00:00:00 2001 From: Jarvis Lin Date: Fri, 16 Feb 2024 19:11:15 +0800 Subject: [PATCH 7/7] Remove a redundant context swicher --- .../wordpress/android/ui/notifications/adapters/NotesAdapter.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt index 8655c88063c2..9770c061875d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt @@ -302,7 +302,7 @@ class NotesAdapter( cancelReloadLocalNotes() reloadLocalNotesJob = coroutineScope.launch { val notes = NotificationsTable.getLatestNotes() - withContext(Dispatchers.Main) { addAll(notes) } + addAll(notes) } }