forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Return Result from tab navigation API (#17071)

Browse Source 
# Objective

Tab navigation can fail in all manner of ways. The current API
recognizes this, but merely logs a warning and returns `None`.

We should supply the actual reason for failure to the caller, so they
can handle it in whatever fashion they please (including logging a
warning!).

Swapping to a Result-oriented pattern is also a bit more idiomatic and
makes the code's control flow easier to follow.

## Solution

- Refactor the `tab_navigation` module to return a `Result` rather than
an `Option` from its key APIs.
- Move the logging to the provided prebuilt observer. This leaves the
default behavior largely unchanged, but allows for better user control.
- Make the case where no tab group was found for the currently focused
entity an error branch, but provide enough information that we can still
recover from it.

## Testing

The `tab_navigation` example continues to function as intended.
This commit is contained in:
Alice Cecile 2024-12-31 23:05:48 -05:00 committed by GitHub
parent c817864e4f
commit 5f1e762f19
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 90 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
crates/bevy_input_focus/Cargo.toml
View File

@ -17,6 +17,8 @@ bevy_hierarchy = { path = "../bevy_hierarchy", version = "0.15.0-dev", default-f
bevy_utils = { path = "../bevy_utils", version = "0.15.0-dev", default-features = false }
bevy_window = { path = "../bevy_window", version = "0.15.0-dev", default-features = false }
thiserror = { version = "2", default-features = false }
[dev-dependencies]
smol_str = "0.2"

114
crates/bevy_input_focus/src/tab_navigation.rs
View File

@ -38,6 +38,7 @@ use bevy_input::{
};
use bevy_utils::tracing::warn;
use bevy_window::PrimaryWindow;
use thiserror::Error;
use crate::{FocusedInput, InputFocus, InputFocusVisible};
@ -79,20 +80,55 @@ impl TabGroup {
}
}
/// A navigation action for tabbing.
/// A navigation action that users might take to navigate your user interface in a cyclic fashion.
///
/// These values are consumed by the [`TabNavigation`] system param.
pub enum NavAction {
/// Navigate to the next focusable entity, wrapping around to the beginning if at the end.
///
/// This is commonly triggered by pressing the Tab key.
Next,
/// Navigate to the previous focusable entity, wrapping around to the end if at the beginning.
///
/// This is commonly triggered by pressing Shift+Tab.
Previous,
/// Navigate to the first focusable entity.
///
/// This is commonly triggered by pressing Home.
First,
/// Navigate to the last focusable entity.
///
/// This is commonly triggered by pressing End.
Last,
}
/// An error that can occur during [tab navigation](crate::tab_navigation).
#[derive(Debug, Error, PartialEq, Eq, Clone)]
pub enum TabNavigationError {
/// No tab groups were found.
#[error("No tab groups found")]
NoTabGroups,
/// No focusable entities were found.
#[error("No focusable entities found")]
NoFocusableEntities,
/// Could not navigate to the next focusable entity.
///
/// This can occur if your tab groups are malformed.
#[error("Failed to navigate to next focusable entity")]
FailedToNavigateToNextFocusableEntity,
/// No tab group for the current focus entity was found.
#[error("No tab group found for currently focused entity {previous_focus}. Users will not be able to navigate back to this entity.")]
NoTabGroupForCurrentFocus {
/// The entity that was previously focused,
/// and is missing its tab group.
previous_focus: Entity,
/// The new entity that will be focused.
///
/// If you want to recover from this error, set [`InputFocus`] to this entity.
new_focus: Entity,
},
}
/// An injectable helper object that provides tab navigation functionality.
#[doc(hidden)]
#[derive(SystemParam)]
@ -112,23 +148,23 @@ pub struct TabNavigation<'w, 's> {
}
impl TabNavigation<'_, '_> {
/// Navigate to the next focusable entity.
/// Navigate to the desired focusable entity.
///
/// Change the [`NavAction`] to navigate in a different direction.
/// Focusable entities are determined by the presence of the [`TabIndex`] component.
///
/// Arguments:
/// * `focus`: The current focus entity, or `None` if no entity has focus.
/// * `action`: Whether to select the next, previous, first, or last focusable entity.
///
/// If no focusable entities are found, then this function will return either the first
/// or last focusable entity, depending on the direction of navigation. For example, if
/// `action` is `Next` and no focusable entities are found, then this function will return
/// the first focusable entity.
pub fn navigate(&self, focus: &InputFocus, action: NavAction) -> Option<Entity> {
pub fn navigate(
&self,
focus: &InputFocus,
action: NavAction,
) -> Result<Entity, TabNavigationError> {
// If there are no tab groups, then there are no focusable entities.
if self.tabgroup_query.is_empty() {
warn!("No tab groups found");
return None;
return Err(TabNavigationError::NoTabGroups);
}
// Start by identifying which tab group we are in. Mainly what we want to know is if
@ -144,11 +180,21 @@ impl TabNavigation<'_, '_> {
})
});
if focus.0.is_some() && tabgroup.is_none() {
warn!("No tab group found for focus entity. Users will not be able to navigate back to this entity.");
}
let navigation_result = self.navigate_in_group(tabgroup, focus, action);
self.navigate_in_group(tabgroup, focus, action)
match navigation_result {
Ok(entity) => {
if focus.0.is_some() && tabgroup.is_none() {
Err(TabNavigationError::NoTabGroupForCurrentFocus {
previous_focus: focus.0.unwrap(),
new_focus: entity,
})
} else {
Ok(entity)
}
}
Err(e) => Err(e),
}
}
fn navigate_in_group(
@ -156,7 +202,7 @@ impl TabNavigation<'_, '_> {
tabgroup: Option<(Entity, &TabGroup)>,
focus: &InputFocus,
action: NavAction,
) -> Option<Entity> {
) -> Result<Entity, TabNavigationError> {
// List of all focusable entities found.
let mut focusable: Vec<(Entity, TabIndex)> =
Vec::with_capacity(self.tabindex_query.iter().len());
@ -189,8 +235,7 @@ impl TabNavigation<'_, '_> {
}
if focusable.is_empty() {
warn!("No focusable entities found");
return None;
return Err(TabNavigationError::NoFocusableEntities);
}
// Stable sort by tabindex
@ -204,7 +249,10 @@ impl TabNavigation<'_, '_> {
(None, NavAction::Next) | (_, NavAction::First) => 0,
(None, NavAction::Previous) | (_, NavAction::Last) => count - 1,
};
focusable.get(next).map(|(e, _)| e).copied()
match focusable.get(next) {
Some((entity, _)) => Ok(*entity),
None => Err(TabNavigationError::FailedToNavigateToNextFocusableEntity),
}
}
/// Gather all focusable entities in tree order.
@ -252,6 +300,8 @@ fn setup_tab_navigation(mut commands: Commands, window: Query<Entity, With<Prima
///
/// This observer responds to [`KeyCode::Tab`] events and Shift+Tab events,
/// cycling through focusable entities in the order determined by their tab index.
///
/// Any [`TabNavigationError`]s that occur during tab navigation are logged as warnings.
pub fn handle_tab_navigation(
mut trigger: Trigger<FocusedInput<KeyboardInput>>,
nav: TabNavigation,
@ -265,7 +315,7 @@ pub fn handle_tab_navigation(
&& key_event.state == ButtonState::Pressed
&& !key_event.repeat
{
let next = nav.navigate(
let maybe_next = nav.navigate(
&focus,
if keys.pressed(KeyCode::ShiftLeft) || keys.pressed(KeyCode::ShiftRight) {
NavAction::Previous
@ -273,10 +323,22 @@ pub fn handle_tab_navigation(
NavAction::Next
},
);
if next.is_some() {
trigger.propagate(false);
focus.0 = next;
visible.0 = true;
match maybe_next {
Ok(next) => {
trigger.propagate(false);
focus.set(next);
visible.0 = true;
}
Err(e) => {
warn!("Tab navigation error: {}", e);
// This failure mode is recoverable, but still indicates a problem.
if let TabNavigationError::NoTabGroupForCurrentFocus { new_focus, .. } = e {
trigger.propagate(false);
focus.set(new_focus);
visible.0 = true;
}
}
}
}
}
@ -305,16 +367,16 @@ mod tests {
let next_entity =
tab_navigation.navigate(&InputFocus::from_entity(tab_entity_1), NavAction::Next);
assert_eq!(next_entity, Some(tab_entity_2));
assert_eq!(next_entity, Ok(tab_entity_2));
let prev_entity =
tab_navigation.navigate(&InputFocus::from_entity(tab_entity_2), NavAction::Previous);
assert_eq!(prev_entity, Some(tab_entity_1));
assert_eq!(prev_entity, Ok(tab_entity_1));
let first_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::First);
assert_eq!(first_entity, Some(tab_entity_1));
assert_eq!(first_entity, Ok(tab_entity_1));
let last_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::Last);
assert_eq!(last_entity, Some(tab_entity_2));
assert_eq!(last_entity, Ok(tab_entity_2));
}
}