-
Notifications
You must be signed in to change notification settings - Fork 839
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.
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
PM-10094: Disable double-navigation by default #3660
Conversation
@@ -2364,8 +2365,7 @@ sealed class VaultAddEditEvent { | |||
/** | |||
* Navigate the user to the tooltip URI. | |||
*/ | |||
data object NavigateToTooltipUri : | |||
VaultAddEditEvent() | |||
data object NavigateToTooltipUri : VaultAddEditEvent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/EventsEffect.kt
Outdated
Show resolved
Hide resolved
No New Or Fixed Issues Found |
7103bd1
to
dcf6bac
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3660 +/- ##
=======================================
Coverage 87.81% 87.81%
=======================================
Files 368 368
Lines 30454 30458 +4
Branches 4547 4548 +1
=======================================
+ Hits 26744 26748 +4
Misses 2128 2128
Partials 1582 1582 ☔ View full report in Codecov by Sentry. |
app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/EventsEffect.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt
Outdated
Show resolved
Hide resolved
dcf6bac
to
a00c9ca
Compare
app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/BackgroundEvent.kt
Show resolved
Hide resolved
app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/EventsEffect.kt
Show resolved
Hide resolved
app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/BackgroundEvent.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
🎟️ Tracking
PM-10094
📔 Objective
Nearly everywhere that navigation is possible in the app, rapidly double-clicking on the navigation button would cause the navigation action to happen twice, which would either stack two new views on top of each other or, in the case of rapidly navigating back too many times, break the view entirely. This PR fixes that issue by ignoring events that occur when the view is not resumed by default.
IMPORTANT NOTE: There is a new interface called
BackgroundEvent
that non-navigation events should conform to if there is a chance they might occur (and should be processed) before the view is resumed. So far only the passkey management related events fall into this category that we know of.📸 Screenshots
An example of rapidly navigating to a new view (also occurs for push navigation when tapping on any of the folders in the list, and on the vault and settings tabs as well):
10094.before.mov
10094.after.mov
An example of rapidly navigating back from a view (also occurs when dismissing a slide-up view, though in that case the base view is still visible just unresponsive):
10094.before.2.mov
10094.after.2.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes