-
Notifications
You must be signed in to change notification settings - Fork 323
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
Focus management #3863
Focus management #3863
Conversation
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.
Please add unit tests for event propagation and focus. The debug scene is nice, but automatic tests are nicer, and I see some untested cases, starting with hiding focused element, checking if is_focused
returns valid value, etc.
} | ||
if event.bubbles.get() { | ||
for object in rev_parent_chain.iter().rev() { | ||
object.bubbling_event_fan.emit(&event.data); |
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.
An event will be emitted here even if the event was cancelled in the capturing_event_fan
loop above
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.
Amazing catch, thanks for it! I added tests to check for it too.
for object in &rev_parent_chain { | ||
object.capturing_event_fan.emit(&event.data); | ||
if event.state() == event::State::Cancelled { | ||
break; |
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.
Maybe return
in this case, to simplify the rest of the function?
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.
I changed the implementation, now we can't return there. Also, even if we could, I really dislike early returns from functions. This is probably because of my Haskell background though ;)
|
||
fn focused_instance(&self) -> Option<Instance<Host>> { | ||
if let Some(child) = &*self.focused_descendant.borrow() { | ||
child.upgrade() |
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.
If this returns None
, would it be better to handle this the same way as if focused_descendant
were None
?
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.
I'm sorry, would you be so nice to rephrase your comment? I did not get its meaning.
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.
If there's no child because focused_descendant
is None
, the logic in the following else
branch runs--but if there's no child because focused_descendant
is an expired weak handle, we're returning None
(the result of the upgrade
call) without entering the else
branch. It's not clear to me that these cases should be treated differently.
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.
Oh, gotcha. Right, I already fixed it before you've written this comment but I did not realisee it. Thanks for catching it. Thanks Kaz for always catching such issues - they are hard to catch, and I appreciate how well you are checking the code, thanks!
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.
Are the cases where we detach/attach focused display Object from/to tree covered? If no, it would be nice to have those as well (including events).
It's covered :) |
Introduction
This PR introduces a few important changes:
Display Object Event System
Display Objects can now emit and capture events in a very similar way JavaScript DOM elements can. Events are propagated in three phases, capturing, target, and the bubbling one. Events are configurable, can be cancelled, and are nicely integrated with FRP. There is a new example scene
ensogl-example-focus-management
showing it in action. Consider the following code to see how easy it is to use it!Events propagate to all parent display objects, contain information of the
target
dimply object (the one the event was originally fired on), and provide a lot of cool utilities.Focus Management
Display objects can be now focused. The focus information propagates trough display object chain and old display objects are automatically de-focused. There is a new example scene
ensogl-example-focus-management
showing it in action. Consider the following code to see how easy it is to use it!And a short movie:
Screen.Recording.2022-11-10.at.07.05.11.mov
FRP
Fan
component.This is really cool!
Fan
is a utility with a single input allowing emitting values of different types and multiple outputs, one per type. Check it out:Fixed regression with upper case letters not being captured.
Shame on me for introducing it in the past. Fixed CC @sylwiabr.
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide build
and./run ide watch
.