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

Focus management #3863

Merged
merged 18 commits into from
Nov 14, 2022
Merged

Focus management #3863

merged 18 commits into from
Nov 14, 2022

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented Nov 10, 2022

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!

/// A new event definition.
#[derive(Clone, Copy, Debug, Default)]
pub struct Foo;

/// A new event definition.
#[derive(Clone, Copy, Debug, Default)]
pub struct Bar;

...

// Event handler.
let on_foo = circle.on_event::<Foo>();

// Event handler for the capturing phase.
let on_bar = circle.on_event_capturing::<Bar>();

frp::extend! { network
    trace on_foo;
    trace on_bar;
}

...

circle.emit_event(Foo); // Traces the event.
circle.emit_event(Bar); // Traces the event.

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!

let rect_on_focus_in = rect.on_event::<FocusIn>();
let rect_on_focus_out = rect.on_event::<FocusOut>();

frp::extend! { network
    eval_ rect.events.mouse_down (rect.focus());
    trace rect_on_focus_in;
    trace rect_on_focus_out;
}

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:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test1() {
        let network = crate::Network::new("network");
        let fan = Fan::new(&network);
        let out_usize = fan.output::<usize>(&network);
        let out_string = fan.output::<String>(&network);
        let last_usize: Rc<RefCell<usize>> = default();
        let last_string: Rc<RefCell<String>> = default();

        crate::extend! { network
            source_usize <- source::<usize>();
            source_string <- source::<String>();
            fan.source <+ source_usize.any_data();
            fan.source <+ source_string.any_data();
            eval out_usize ([last_usize](t) *last_usize.borrow_mut() = *t);
            eval out_string ([last_string](t) *last_string.borrow_mut() = t.clone());
        }

        assert_eq!(*last_usize.borrow(), 0);
        assert_eq!(*last_string.borrow(), "");

        source_usize.emit(1);
        assert_eq!(*last_usize.borrow(), 1);
        assert_eq!(*last_string.borrow(), "");

        source_string.emit("foo".to_string());
        assert_eq!(*last_usize.borrow(), 1);
        assert_eq!(*last_string.borrow(), "foo");

        source_usize.emit(2);
        assert_eq!(*last_usize.borrow(), 2);
        assert_eq!(*last_string.borrow(), "foo");

        source_string.emit("bar".to_string());
        assert_eq!(*last_usize.borrow(), 2);
        assert_eq!(*last_string.borrow(), "bar");
    }
}

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:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@wdanilo wdanilo changed the title Wip/wdanilo/focus management 183747460 Focus management Nov 10, 2022
@wdanilo wdanilo marked this pull request as ready for review November 10, 2022 07:53
lib/rust/frp/src/fan.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@farmaazon farmaazon left a 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.

lib/rust/ensogl/core/src/application/frp.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/class.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/class.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/class.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/event.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/event.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/event.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/event.rs Outdated Show resolved Hide resolved
lib/rust/frp/src/fan.rs Outdated Show resolved Hide resolved
}
if event.bubbles.get() {
for object in rev_parent_chain.iter().rev() {
object.bubbling_event_fan.emit(&event.data);
Copy link
Contributor

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

Copy link
Member Author

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;
Copy link
Contributor

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?

Copy link
Member Author

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()
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@kazcw kazcw Nov 11, 2022

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.

Copy link
Member Author

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!

Copy link
Contributor

@farmaazon farmaazon left a 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).

@wdanilo
Copy link
Member Author

wdanilo commented Nov 14, 2022

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 :)

@wdanilo wdanilo merged commit 77934e0 into develop Nov 14, 2022
@wdanilo wdanilo deleted the wip/wdanilo/focus-management-183747460 branch November 14, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants