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.

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

operation::focusable::is_focused & text_input::is_focused #2812

Merged
merged 2 commits into from
Feb 23, 2025

Conversation

andymandias
Copy link
Contributor

This PR consists of two additions:

  1. An is_focused function that produces an Operation that looks for a focusable widget by ID, and produces a bool that stores whether the widget is focused or not. This function was modeled after the existing find_focused, and for my purposes was a means to implement the second addition.
  2. An is_focused function that produces a Task that looks for a text_input widget by ID, and produces a bool with whether the widget is focused or not.

Originally I tried to use the find_focused Operation for text_input::is_focused, but I couldn't figure out how to make it work as desired when there is no currently focused widget (and find_focused produces Outcome::None).

…used state of a `focusable` by ID.

Add `is_focused` function that produces a `Task` to get the focused state of a `text_input` by ID.
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Thanks!

Looking good! Just a couple nits.

_bounds: Rectangle,
operate_on_children: &mut dyn FnMut(&mut dyn Operation<bool>),
) {
operate_on_children(self);
Copy link
Member

Choose a reason for hiding this comment

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

We could probably stop traversing the widget tree if self.is_focused is already Some:

Suggested change
operate_on_children(self);
if self.is_focused.is_some() {
return;
}
operate_on_children(self);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! Makes sense to me. Outside the scope of this PR probably, but would it make sense to similarly stop traversing once a result is found for find_focused as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, both nits should apply there as well.

Comment on lines 292 to 293
if let Some(is_focused) = &self.is_focused {
Outcome::Some(*is_focused)
Copy link
Member

Choose a reason for hiding this comment

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

Since bool is copy, we can avoid the dereference:

Suggested change
if let Some(is_focused) = &self.is_focused {
Outcome::Some(*is_focused)
if let Some(is_focused) = self.is_focused {
Outcome::Some(is_focused)

Copy link
Member

@hecrj hecrj Feb 22, 2025

Choose a reason for hiding this comment

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

Maybe even something functional like:

self.is_focused.map_or(Outcome::None, Outcome::Some)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much cleaner, thank you!

@hecrj hecrj added feature New feature or request widget addition labels Feb 22, 2025
@hecrj hecrj added this to the 0.14 milestone Feb 22, 2025
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks!

@hecrj hecrj enabled auto-merge February 23, 2025 06:28
@hecrj hecrj merged commit 34314b3 into iced-rs:master Feb 23, 2025
15 checks passed
@andymandias andymandias deleted the text_input-is_focused branch February 23, 2025 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition feature New feature or request widget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants