Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -85,19 +85,6 @@ class LandingTests {
Intents.intended(hasAction(Intent.ACTION_OPEN_DOCUMENT))
}

@Test
fun theFabOpensTheSystemPicker() {
// the fab is hidden behind the empty state, which offers the same thing with a label
seedRecentDocument()
stubOpenDocumentCancelled()

launch()

onView(withId(R.id.landing_open_fab)).perform(click())

Intents.intended(hasAction(Intent.ACTION_OPEN_DOCUMENT))
}

@Test
fun aRecentDocumentIsListed() {
seedRecentDocument()
Expand All @@ -121,6 +108,19 @@ class LandingTests {
)
}

@Test
fun theFabOpensTheSystemPicker() {
// the fab is hidden behind the empty state, which offers the same thing with a label
seedRecentDocument()
stubOpenDocumentCancelled()

launch()

onView(withId(R.id.landing_open_fab)).perform(click())

Intents.intended(hasAction(Intent.ACTION_OPEN_DOCUMENT))
}

@Test
fun addingAFolderAsksTheSystemForATree() {
Intents.intending(hasAction(Intent.ACTION_OPEN_DOCUMENT_TREE))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,19 @@ object RecentDocumentList {

/** Drops every entry for [uri]. Returns [current] unchanged if there is none. */
fun remove(current: List<Entry>, uri: String): List<Entry> = current.filter { it.uri != uri }

/**
* Puts [entry] back at [index], for undoing a removal.
*
* Unlike [add] this does not move it to the front - the point of an undo is that the list ends
* up looking like it did before. An index past the end lands at the end.
*/
fun insert(current: List<Entry>, entry: Entry, index: Int): List<Entry> {
val entries = ArrayList<Entry>(current.size + 1)
current.filterTo(entries) { it.uri != entry.uri }

entries.add(index.coerceIn(0, entries.size), entry)

return entries
Comment on lines +66 to +68

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Cap restored recents after concurrent additions

If the recent list is already at MAX_ENTRIES, the user can swipe one item away, open another document while the undo snackbar is still available, and then tap Undo. At that point current is full again, but insert() adds the restored entry and returns 33 items because it does not apply the same cap/eviction rule as add(), breaking the class invariant that the recent list cannot grow without bound and leaving extra persisted URI grants referenced.

Useful? React with πŸ‘Β / πŸ‘Ž.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ object RecentDocumentsUtil {
return update.evicted
}

/** Puts a removed document back where it was, for undoing a swipe. */
@Synchronized
fun restoreRecentDocument(context: Context, entry: RecentDocumentList.Entry, index: Int) {
write(context, RecentDocumentList.insert(read(context), entry, index))
}

/** Drops [uri] from the list. Does nothing if it is not in it. */
@Synchronized
fun removeRecentDocument(context: Context, uri: Uri) {
Expand Down
19 changes: 19 additions & 0 deletions app/src/main/java/app/opendocument/droid/ui/SnackbarHelper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,25 @@ object SnackbarHelper {
)
}

/** Same, with a button that says something other than "OK" - "undo", typically. */
fun show(
activity: Activity,
resId: Int,
buttonResId: Int,
callback: Runnable?,
isIndefinite: Boolean,
isError: Boolean,
) {
show(
activity,
activity.getString(buttonResId),
activity.getString(resId),
callback,
isIndefinite,
isError,
)
}

private fun show(
activity: Activity,
buttonText: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,21 @@ import android.app.Activity
import android.content.Intent
import android.net.Uri
import android.os.Bundle
import android.text.format.DateUtils
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.activity.OnBackPressedCallback
import androidx.activity.result.contract.ActivityResultContracts
import androidx.fragment.app.Fragment
import androidx.lifecycle.ViewModelProvider
import androidx.recyclerview.widget.ItemTouchHelper
import androidx.recyclerview.widget.LinearLayoutManager
import androidx.recyclerview.widget.RecyclerView
import app.opendocument.droid.R
import app.opendocument.droid.background.DocumentTreeBrowser
import app.opendocument.droid.background.PersistedUriPermissions
import app.opendocument.droid.background.RecentDocumentList
import app.opendocument.droid.nonfree.AnalyticsConstants
import app.opendocument.droid.ui.SnackbarHelper
import app.opendocument.droid.ui.widget.LandingAdapter
Expand All @@ -42,6 +45,10 @@ class LandingFragment : Fragment(), LandingAdapter.Listener {

private var landingVisible = true

// the entries behind the recent rows, kept so a swipe can restore the exact entry - the row
// itself only carries what it needs to draw
private var lastDocuments: List<RecentDocumentList.Entry> = emptyList()

/**
* Back goes up a folder before it goes anywhere else.
*
Expand Down Expand Up @@ -117,6 +124,8 @@ class LandingFragment : Fragment(), LandingAdapter.Listener {
mainActivity.findDocument()
}

attachSwipeToRemove()

requireActivity()
.onBackPressedDispatcher
.addCallback(viewLifecycleOwner, folderBackCallback)
Expand Down Expand Up @@ -158,6 +167,8 @@ class LandingFragment : Fragment(), LandingAdapter.Listener {
private fun render(state: LandingViewModel.State) {
folderBackCallback.isEnabled = landingVisible && state.location != null

lastDocuments = state.documents

val items = ArrayList<LandingItem>()

val location = state.location
Expand All @@ -182,6 +193,75 @@ class LandingFragment : Fragment(), LandingAdapter.Listener {
adapter.submitList(items)
}

/**
* Swiping a recent document away removes it, with an undo that puts it back at the same place.
*
* Only the recently opened documents can go: a document inside a folder is simply there, and
* the app has no business deleting anything.
*/
private fun attachSwipeToRemove() {
val callback =
object :
ItemTouchHelper.SimpleCallback(
0,
ItemTouchHelper.START or ItemTouchHelper.END,
) {

override fun onMove(
recyclerView: RecyclerView,
viewHolder: RecyclerView.ViewHolder,
target: RecyclerView.ViewHolder,
): Boolean = false

override fun getSwipeDirs(
recyclerView: RecyclerView,
viewHolder: RecyclerView.ViewHolder,
): Int =
if (adapter.isRemovableDocument(viewHolder.bindingAdapterPosition))
super.getSwipeDirs(recyclerView, viewHolder)
else 0

override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {
val position = viewHolder.bindingAdapterPosition
val document = adapter.documentAt(position) ?: return

removeWithUndo(document, position)
}
}

ItemTouchHelper(callback).attachToRecyclerView(list)
}

private fun removeWithUndo(document: LandingItem.Document, position: Int) {
val entry = entryFor(document) ?: return

// where it sits among the documents, which is what restoring it needs - the adapter
// position counts the section header too
val index = lastDocuments.indexOfFirst { it.uri == entry.uri }

mainActivity.analyticsManager.report("recent_swiped_away")

viewModel.removeRecentDocumentUndoably(document.uri)

// the grant it was holding is deliberately not released here: an undo would need it back,
// and prune() reclaims it on the next launch anyway, which is the whole point of
// reconciling against the stored lists rather than bookkeeping every removal
SnackbarHelper.show(
requireActivity(),
R.string.landing_recent_removed,
R.string.landing_undo,
{ viewModel.restoreRecentDocument(entry, index) },
false,
false,
)
}

private fun entryFor(document: LandingItem.Document): RecentDocumentList.Entry? {
val uri = document.uri.toString()

return lastDocuments.firstOrNull { it.uri == uri }
}

private fun renderRoot(
items: ArrayList<LandingItem>,
state: LandingViewModel.State,
Expand All @@ -196,7 +276,13 @@ class LandingFragment : Fragment(), LandingAdapter.Listener {
if (state.documents.isNotEmpty()) {
items.add(LandingItem.Header(R.string.landing_section_recent))
for (document in state.documents) {
items.add(LandingItem.Document(document.filename, Uri.parse(document.uri)))
items.add(
LandingItem.Document(
document.filename,
Uri.parse(document.uri),
lastOpenedLabel(document.lastOpenedAt),
)
)
}
}

Expand All @@ -220,7 +306,28 @@ class LandingFragment : Fragment(), LandingAdapter.Listener {
}

items.add(LandingItem.Header(R.string.landing_section_settings))
items.add(LandingItem.CatchAll(state.catchAllEnabled))
items.add(
LandingItem.Setting(
LandingItem.SETTING_CATCH_ALL,
R.string.landing_catch_all_title,
R.string.landing_intro_open_all,
state.catchAllEnabled,
)
)
}

/** "2 hours ago", or nothing at all for entries written before this was recorded. */
private fun lastOpenedLabel(lastOpenedAt: Long): String? {
if (lastOpenedAt <= 0) {
return null
}

return DateUtils.getRelativeTimeSpanString(
lastOpenedAt,
System.currentTimeMillis(),
DateUtils.MINUTE_IN_MILLIS,
)
.toString()
}

private fun renderFolder(
Expand Down Expand Up @@ -341,12 +448,16 @@ class LandingFragment : Fragment(), LandingAdapter.Listener {
mainActivity.findDocument()
}

override fun onCatchAllChanged(enabled: Boolean) {
viewModel.setCatchAllEnabled(enabled)
override fun onSettingChanged(setting: Int, enabled: Boolean) {
when (setting) {
LandingItem.SETTING_CATCH_ALL -> {
viewModel.setCatchAllEnabled(enabled)

mainActivity.analyticsManager.report(
if (enabled) "catch_all_enabled" else "catch_all_disabled"
)
mainActivity.analyticsManager.report(
if (enabled) "catch_all_enabled" else "catch_all_disabled"
)
}
}
}

private val mainActivity: MainActivity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,34 @@ class LandingViewModel(application: Application) : AndroidViewModel(application)
}
}

/**
* Puts a swiped away document back where it was.
*
* The grant it needs is still held: [prune] only runs after a removal that the user has had the
* chance to undo, so nothing has been handed back yet.
*/
fun restoreRecentDocument(entry: RecentDocumentList.Entry, index: Int) {
executor.execute {
RecentDocumentsUtil.restoreRecentDocument(getApplication(), entry, index)

refresh()
}
}

/** Forgets a document without releasing its grant yet, so an undo can still put it back. */
fun removeRecentDocumentUndoably(uri: Uri) {
executor.execute {
RecentDocumentsUtil.removeRecentDocument(getApplication(), uri)

refresh()
}
}

/** Hands back the grants of everything that is no longer referenced. */
fun releaseUnusedGrants() {
executor.execute { PersistedUriPermissions.prune(getApplication()) }
}

override fun onCleared() {
super.onCleared()

Expand Down
Loading
Loading