fix: Remove progressbar from status timelines (#208)

Previously the middle-of-screen progress spinner and the spinner that appears on a swipe-to-refresh could get out of sync. Fix this by removing the middle-of-screen progress spinner from relevant fragments, as the swipe-to-refresh spinner shows the user that an operation is in progress, and also clues them in to the fact that a swipe-to-refresh is possible (by using the common UX control). Fixes #75
pachli · Oct 30, 2023 · 6aa4eab · 6aa4eab
1 parent c3f68ba
commit 6aa4eab
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 80 deletions.
diff --git a/app/lint-baseline.xml b/app/lint-baseline.xml
@@ -2555,7 +2555,7 @@
         errorLine2="                            ~~~~~~~">
         <location
             file="src/main/java/app/pachli/components/conversation/ConversationsFragment.kt"
-            line="148"
+            line="152"
             column="29"/>
     </issue>
 
@@ -2566,7 +2566,7 @@
         errorLine2="                                    ~~~~~~~">
         <location
             file="src/main/java/app/pachli/components/conversation/ConversationsFragment.kt"
-            line="150"
+            line="154"
             column="37"/>
     </issue>
 

diff --git a/app/src/main/java/app/pachli/components/account/media/AccountMediaFragment.kt b/app/src/main/java/app/pachli/components/account/media/AccountMediaFragment.kt
@@ -30,6 +30,7 @@ import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.lifecycleScope
 import androidx.paging.LoadState
 import androidx.recyclerview.widget.GridLayoutManager
+import androidx.swiperefreshlayout.widget.SwipeRefreshLayout.OnRefreshListener
 import app.pachli.R
 import app.pachli.ViewMediaActivity
 import app.pachli.databinding.FragmentTimelineBinding
@@ -59,6 +60,7 @@ import javax.inject.Inject
 @AndroidEntryPoint
 class AccountMediaFragment :
     Fragment(R.layout.fragment_timeline),
+    OnRefreshListener,
     RefreshableFragment,
     MenuProvider {
 
@@ -99,7 +101,7 @@ class AccountMediaFragment :
         binding.recyclerView.adapter = adapter
 
         binding.swipeRefreshLayout.isEnabled = false
-        binding.swipeRefreshLayout.setOnRefreshListener { refreshContent() }
+        binding.swipeRefreshLayout.setOnRefreshListener(this)
         binding.swipeRefreshLayout.setColorSchemeColors(MaterialColors.getColor(binding.root, androidx.appcompat.R.attr.colorPrimary))
 
         binding.statusView.visibility = View.GONE
@@ -112,7 +114,6 @@ class AccountMediaFragment :
 
         adapter.addLoadStateListener { loadState ->
             binding.statusView.hide()
-            binding.progressBar.hide()
 
             if (loadState.refresh != LoadState.Loading && loadState.source.refresh != LoadState.Loading) {
                 binding.swipeRefreshLayout.isRefreshing = false
@@ -128,11 +129,11 @@ class AccountMediaFragment :
                     }
                     is LoadState.Error -> {
                         binding.statusView.show()
-                        binding.statusView.setup((loadState.refresh as LoadState.Error).error)
-                    }
-                    is LoadState.Loading -> {
-                        binding.progressBar.show()
+                        binding.statusView.setup((loadState.refresh as LoadState.Error).error) {
+                            refreshContent()
+                        }
                     }
+                    is LoadState.Loading -> { /* nothing to do */ }
                 }
             }
         }
@@ -151,7 +152,6 @@ class AccountMediaFragment :
     override fun onMenuItemSelected(menuItem: MenuItem): Boolean {
         return when (menuItem.itemId) {
             R.id.action_refresh -> {
-                binding.swipeRefreshLayout.isRefreshing = true
                 refreshContent()
                 true
             }
@@ -191,7 +191,18 @@ class AccountMediaFragment :
         }
     }
 
+    /** Refresh the displayed content, as if the user had swiped on the SwipeRefreshLayout */
     override fun refreshContent() {
+        binding.swipeRefreshLayout.isRefreshing = true
+        onRefresh()
+    }
+
+    /**
+     * Listener for the user swiping on the SwipeRefreshLayout. The SwipeRefreshLayout has
+     * handled displaying the animated spinner.
+     */
+    override fun onRefresh() {
+        binding.statusView.hide()
         adapter.refresh()
     }
 

diff --git a/app/src/main/java/app/pachli/components/conversation/ConversationsFragment.kt b/app/src/main/java/app/pachli/components/conversation/ConversationsFragment.kt
@@ -34,6 +34,7 @@ import androidx.paging.LoadState
 import androidx.recyclerview.widget.LinearLayoutManager
 import androidx.recyclerview.widget.RecyclerView
 import androidx.recyclerview.widget.SimpleItemAnimator
+import androidx.swiperefreshlayout.widget.SwipeRefreshLayout.OnRefreshListener
 import app.pachli.R
 import app.pachli.StatusListActivity
 import app.pachli.adapter.StatusBaseViewHolder
@@ -71,6 +72,7 @@ import kotlin.time.toDuration
 @AndroidEntryPoint
 class ConversationsFragment :
     SFragment(),
+    OnRefreshListener,
     StatusActionListener,
     ReselectableFragment,
     MenuProvider {
@@ -114,11 +116,11 @@ class ConversationsFragment :
                 }
 
                 binding.statusView.hide()
-                binding.progressBar.hide()
 
                 if (adapter.itemCount == 0) {
                     when (loadState.refresh) {
                         is LoadState.NotLoading -> {
+                            binding.swipeRefreshLayout.isRefreshing = false
                             if (loadState.append is LoadState.NotLoading && loadState.source.refresh is LoadState.NotLoading) {
                                 binding.statusView.show()
                                 binding.statusView.setup(
@@ -130,13 +132,14 @@ class ConversationsFragment :
                         }
 
                         is LoadState.Error -> {
+                            binding.swipeRefreshLayout.isRefreshing = false
                             binding.statusView.show()
-                            binding.statusView.setup((loadState.refresh as LoadState.Error).error) { refreshContent() }
+                            binding.statusView.setup((loadState.refresh as LoadState.Error).error) {
+                                refreshContent()
+                            }
                         }
 
-                        is LoadState.Loading -> {
-                            binding.progressBar.show()
-                        }
+                        is LoadState.Loading -> { /* nothing to do */ }
                     }
                 }
             }
@@ -208,7 +211,6 @@ class ConversationsFragment :
     override fun onMenuItemSelected(menuItem: MenuItem): Boolean {
         return when (menuItem.itemId) {
             R.id.action_refresh -> {
-                binding.swipeRefreshLayout.isRefreshing = true
                 refreshContent()
                 true
             }
@@ -229,13 +231,24 @@ class ConversationsFragment :
         binding.recyclerView.adapter = adapter.withLoadStateFooter(ConversationLoadStateAdapter(adapter::retry))
     }
 
+    private fun initSwipeToRefresh() {
+        binding.swipeRefreshLayout.setOnRefreshListener(this)
+        binding.swipeRefreshLayout.setColorSchemeColors(MaterialColors.getColor(binding.root, androidx.appcompat.R.attr.colorPrimary))
+    }
+
+    /** Refresh the displayed content, as if the user had swiped on the SwipeRefreshLayout */
     private fun refreshContent() {
-        adapter.refresh()
+        binding.swipeRefreshLayout.isRefreshing = true
+        onRefresh()
     }
 
-    private fun initSwipeToRefresh() {
-        binding.swipeRefreshLayout.setOnRefreshListener { refreshContent() }
-        binding.swipeRefreshLayout.setColorSchemeColors(MaterialColors.getColor(binding.root, androidx.appcompat.R.attr.colorPrimary))
+    /**
+     * Listener for the user swiping on the SwipeRefreshLayout. The SwipeRefreshLayout has
+     * handled displaying the animated spinner.
+     */
+    override fun onRefresh() {
+        binding.statusView.hide()
+        adapter.refresh()
     }
 
     override fun onReblog(reblog: Boolean, position: Int) {
@@ -375,6 +388,8 @@ class ConversationsFragment :
     }
 
     companion object {
+        @Suppress("unused")
+        private const val TAG = "ConversationsFragment"
         fun newInstance() = ConversationsFragment()
     }
 }
diff --git a/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt b/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt
@@ -391,20 +391,13 @@ class TimelineFragment :
                     }
                 }
 
-                // Manage the display of progress bars. Rather than hide them as soon as the
-                // Refresh portion completes, hide them when then first Prepend completes. This
-                // is a better signal to the user that it is now possible to scroll up and see
-                // new content.
+                // Manage the progress display. Rather than hide as soon as the Refresh portion
+                // completes, hide when then first Prepend completes. This is a better signal to
+                // the user that it is now possible to scroll up and see new content.
                 launch {
                     refreshState.collect {
                         when (it) {
-                            UserRefreshState.ACTIVE -> {
-                                if (adapter.itemCount == 0 && !binding.swipeRefreshLayout.isRefreshing) {
-                                    binding.progressBar.show()
-                                }
-                            }
                             UserRefreshState.COMPLETE, UserRefreshState.ERROR -> {
-                                binding.progressBar.hide()
                                 binding.swipeRefreshLayout.isRefreshing = false
                             }
                             else -> { /* nothing to do */ }
@@ -555,10 +548,19 @@ class TimelineFragment :
         )
     }
 
+    /** Refresh the displayed content, as if the user had swiped on the SwipeRefreshLayout */
+    override fun refreshContent() {
+        binding.swipeRefreshLayout.isRefreshing = true
+        onRefresh()
+    }
+
+    /**
+     * Listener for the user swiping on the SwipeRefreshLayout. The SwipeRefreshLayout has
+     * handled displaying the animated spinner.
+     */
     override fun onRefresh() {
         binding.statusView.hide()
         snackbar?.dismiss()
-
         adapter.refresh()
     }
 
@@ -754,11 +756,6 @@ class TimelineFragment :
         }
     }
 
-    override fun refreshContent() {
-        binding.swipeRefreshLayout.isRefreshing = true
-        onRefresh()
-    }
-
     companion object {
         private const val TAG = "TimelineFragment" // logging tag
         private const val KIND_ARG = "kind"

diff --git a/app/src/main/res/layout-sw640dp/fragment_timeline.xml b/app/src/main/res/layout-sw640dp/fragment_timeline.xml
@@ -1,6 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
 <FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
-    xmlns:app="http://schemas.android.com/apk/res-auto"
     android:layout_width="match_parent"
     android:layout_height="match_parent">
 
@@ -33,26 +32,5 @@
                     android:visibility="gone" />
             </FrameLayout>
         </androidx.swiperefreshlayout.widget.SwipeRefreshLayout>
-
-        <ProgressBar
-            android:id="@+id/progressBar"
-            android:layout_width="wrap_content"
-            android:layout_height="wrap_content"
-            app:layout_constraintBottom_toBottomOf="parent"
-            app:layout_constraintLeft_toLeftOf="parent"
-            app:layout_constraintRight_toRightOf="parent"
-            app:layout_constraintTop_toTopOf="parent" />
-
-        <androidx.core.widget.ContentLoadingProgressBar
-            android:id="@+id/topProgressBar"
-            style="@style/Widget.AppCompat.ProgressBar.Horizontal"
-            android:layout_width="match_parent"
-            android:layout_height="wrap_content"
-            android:indeterminate="true"
-            android:visibility="gone"
-            app:layout_constraintBottom_toTopOf="parent"
-            app:layout_constraintEnd_toEndOf="parent"
-            app:layout_constraintStart_toStartOf="parent"
-            app:layout_constraintTop_toTopOf="parent" />
     </androidx.constraintlayout.widget.ConstraintLayout>
 </FrameLayout>
diff --git a/app/src/main/res/layout/fragment_timeline.xml b/app/src/main/res/layout/fragment_timeline.xml
@@ -1,6 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
 <androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
-    xmlns:app="http://schemas.android.com/apk/res-auto"
     android:layout_width="match_parent"
     android:layout_height="match_parent">
 
@@ -27,26 +26,4 @@
                 android:visibility="gone" />
         </FrameLayout>
     </androidx.swiperefreshlayout.widget.SwipeRefreshLayout>
-
-    <ProgressBar
-        android:id="@+id/progressBar"
-        android:layout_width="wrap_content"
-        android:layout_height="wrap_content"
-        app:layout_constraintBottom_toBottomOf="parent"
-        app:layout_constraintLeft_toLeftOf="parent"
-        app:layout_constraintRight_toRightOf="parent"
-        app:layout_constraintTop_toTopOf="parent" />
-
-    <androidx.core.widget.ContentLoadingProgressBar
-        android:id="@+id/topProgressBar"
-        style="@style/Widget.AppCompat.ProgressBar.Horizontal"
-        android:layout_width="match_parent"
-        android:layout_height="wrap_content"
-        android:indeterminate="true"
-        android:visibility="gone"
-        app:layout_constraintBottom_toTopOf="parent"
-        app:layout_constraintEnd_toEndOf="parent"
-        app:layout_constraintStart_toStartOf="parent"
-        app:layout_constraintTop_toTopOf="parent" />
-
 </androidx.constraintlayout.widget.ConstraintLayout>