Space Switching Back Navigation #6877
Conversation
ericdecanini
requested review from
a team,
ganfra,
fedrunov and
amshakal
and removed request for
a team
ericdecanini
changed the title
ericdecanini
requested review from
ouchadam
and removed request for
ganfra and
amshakal
…ew-layout-debug # Conflicts: # vector/src/test/java/im/vector/app/features/navigation/DefaultNavigatorTest.kt
|
to double check the behaviour, if I go into a inner space, leave the app and return, do I return to the inner space selected and if so, does pressing back exit the app or move up a space level? |
| vectorPreferences.getSpaceBackstack().toMutableList().apply { | ||
| val poppedSpaceId = removeLast() | ||
| vectorPreferences.setSpaceBackstack(this) | ||
| return poppedSpaceId |
There was a problem hiding this comment.
nit - having the return within the apply feels a little odd to me, I would expect apply usages to augment the receiver but here we have a side effect and exit point
this might be a case where avoiding the scope could be simpler?
val backstack = vectorPreferences.getSpaceBackstack().toMutableList()
val poppedSpaceId = backstack.removeLast()
vectorPreferences.setSpaceBackstack(backstack)
return poppedSpaceIdor with the side effect declared with an also
val backstack = vectorPreferences.getSpaceBackstack().toMutableList()
return backstack.removeLast().also { vectorPreferences.setSpaceBackstack(backstack) }super minor opinion for me, will leave up to you!
| true | ||
| } else { | ||
| } catch (e: NoSuchElementException) { |
There was a problem hiding this comment.
which part of the api throws NoSuchElementException? if we're using exceptions for control flow we'll probably want to document it in the SpaceStateHandler api and add a unit test case
ideally we wouldn't rely on exceptions by catching and modelling the state within our own domain, unless this is an exceptional case out of our control 🤔
There was a problem hiding this comment.
It happens in SpaceStateHandlerImpl:144 (val poppedSpaceId = (spaceBackstack).removeLast)
The reason I wanted to use exception here instead of null is because the space backstack can include a valid null which signifies the "All Chats" space, thus we can't use null as a fail option
There was a problem hiding this comment.
my concern with using the exception is that it reads as if an error has occurred when in reality the user is at the top of the backstack (an empty stack), there's the risk that a NoSuchElementException could be thrown somewhere else and we unexpectedly catch it, causing swallowed errors 🤔
a named guard function provides extra context in relation to the navigation and allows the use of valid nulls
fun onBackPressed() = when {
spaceStateHandler.isRoot() -> false
else -> {
val lastSpace = spaceStateHandler.popSpaceBackstack()
spaceStateHandler.setCurrentSpace(lastSpace, isForwardNavigation = false)
true
}
}
private fun SpaceStateHandler.isRoot() = getBackstack().isEmpty()
There was a problem hiding this comment.
Good suggestion. Changes made
Debug Icon UX Improvements & Addition in New Layout
…ayout-navigation # Conflicts: # vector/src/main/java/im/vector/app/features/home/HomeActivity.kt # vector/src/main/java/im/vector/app/features/home/NewHomeDetailFragment.kt
…ayout-navigation # Conflicts: # vector/src/main/java/im/vector/app/features/home/NewHomeDetailFragment.kt
…ayout-navigation # Conflicts: # vector/src/main/java/im/vector/app/features/home/NewHomeDetailFragment.kt
…ayout-navigation # Conflicts: # vector/src/main/java/im/vector/app/features/spaces/NewSpaceSummaryController.kt
|
SonarCloud Quality Gate failed. |
Type of change
Content
Adds a space backstack that gets used when navigating back with the android back button.
Motivation and context
Closes #6751
Screenshots / GIFs
goingback.mp4
Tests
Tested devices
Checklist