Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix onBackPressed() deprecation In Card Browser #17594

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+52 −18
Open

Fix onBackPressed() deprecation In Card Browser #17594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 50 additions & 16 deletions AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ import android.widget.CheckBox
import android.widget.ListView
import android.widget.Spinner
import android.widget.TextView
import androidx.activity.OnBackPressedCallback
import androidx.activity.result.ActivityResult
import androidx.activity.result.contract.ActivityResultContracts.StartActivityForResult
import androidx.annotation.CheckResult
import androidx.annotation.ColorInt
import androidx.annotation.MainThread
import androidx.annotation.VisibleForTesting
import androidx.appcompat.app.ActionBarDrawerToggle
import androidx.appcompat.widget.SearchView
import androidx.lifecycle.ViewModelProvider
import androidx.lifecycle.lifecycleScope
Expand Down Expand Up @@ -284,6 +286,31 @@ open class CardBrowser :
private var shouldRestoreScroll = false
private var postAutoScroll = false

private val drawerBackCallback =
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in CardBrowser? This should be a responsibility of the drawer

object : OnBackPressedCallback(false) {
override fun handleOnBackPressed() {
closeDrawer()
}
}

private val multiSelectBackCallback =
object : OnBackPressedCallback(false) {
override fun handleOnBackPressed() {
viewModel.endMultiSelectMode()
}
}

private val closeCardBrowserBackCallback =
object : OnBackPressedCallback(true) {
override fun handleOnBackPressed() {
Timber.i("Back key pressed")
val data = Intent()
// Add reload flag to result intent so that schedule reset when returning to note editor
data.putExtra(NoteEditor.RELOAD_REQUIRED_EXTRA_KEY, reloadRequired)
closeCardBrowser(RESULT_OK, data)
Copy link
Member

Choose a reason for hiding this comment

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

This seems... unusual. It feels like it shouldn't be in the callback and should be the default of pressing 'back', unless there's documentation to disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this back callback method responsible for close of CardBrowser.

Copy link
Member

Choose a reason for hiding this comment

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

As stated twice, this isn't correct

}
}

init {
ChangeManager.subscribe(this)
}
Expand Down Expand Up @@ -443,6 +470,27 @@ open class CardBrowser :

setupFlows()
registerOnForgetHandler { viewModel.queryAllSelectedCardIds() }
val drawerListener =
object : ActionBarDrawerToggle(
this,
drawerLayout,
R.string.drawer_open,
R.string.drawer_close,
) {
override fun onDrawerClosed(drawerView: View) {
super.onDrawerClosed(drawerView)
drawerBackCallback.isEnabled = false
}

override fun onDrawerOpened(drawerView: View) {
super.onDrawerOpened(drawerView)
drawerBackCallback.isEnabled = true
}
}
drawerLayout.addDrawerListener(drawerListener)
Comment on lines +473 to +490
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be the responsibility of the Card Browser

onBackPressedDispatcher.addCallback(this, closeCardBrowserBackCallback)
onBackPressedDispatcher.addCallback(this, multiSelectBackCallback)
onBackPressedDispatcher.addCallback(this, drawerBackCallback)
}

@Suppress("UNUSED_PARAMETER")
Expand Down Expand Up @@ -498,6 +546,7 @@ open class CardBrowser :
// show title and hide spinner
actionBarTitle.visibility = View.VISIBLE
deckSpinnerSelection.setSpinnerVisibility(View.GONE)
multiSelectBackCallback.isEnabled = true
} else {
Timber.d("end multiselect mode")
// If view which was originally selected when entering multi-select is visible then maintain its position
Expand All @@ -507,6 +556,7 @@ open class CardBrowser :
cardsAdapter.notifyDataSetChanged()
deckSpinnerSelection.setSpinnerVisibility(View.VISIBLE)
actionBarTitle.visibility = View.GONE
multiSelectBackCallback.isEnabled = false
}
// reload the actionbar using the multi-select mode actionbar
invalidateOptionsMenu()
Expand Down Expand Up @@ -926,22 +976,6 @@ open class CardBrowser :
super.onDestroy()
}

@Deprecated("Deprecated in Java")
@Suppress("DEPRECATION")
override fun onBackPressed() {
when {
isDrawerOpen -> super.onBackPressed()
viewModel.isInMultiSelectMode -> viewModel.endMultiSelectMode()
else -> {
Timber.i("Back key pressed")
val data = Intent()
// Add reload flag to result intent so that schedule reset when returning to note editor
data.putExtra(NoteEditor.RELOAD_REQUIRED_EXTRA_KEY, reloadRequired)
closeCardBrowser(RESULT_OK, data)
}
}
}

override fun onPause() {
super.onPause()
// If the user entered something into the search, but didn't press "search", clear this.
Expand Down
4 changes: 2 additions & 2 deletions AnkiDroid/src/main/java/com/ichi2/anki/NavigationDrawerActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ abstract class NavigationDrawerActivity :
private var navButtonGoesBack = false

// Navigation drawer list item entries
private lateinit var drawerLayout: DrawerLayout
lateinit var drawerLayout: DrawerLayout
private var navigationView: NavigationView? = null
lateinit var drawerToggle: ActionBarDrawerToggle
private set
Expand Down Expand Up @@ -413,7 +413,7 @@ abstract class NavigationDrawerActivity :
drawerLayout.openDrawer(GravityCompat.START, animationEnabled())
}

private fun closeDrawer() {
protected fun closeDrawer() {
drawerLayout.closeDrawer(GravityCompat.START, animationEnabled())
}

Expand Down
Loading