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

Native callback handler simplification (how to access data without too many Rc/Weak jungling) #1714

Open
ogoffart opened this issue Oct 7, 2022 · 6 comments
Assignees
Labels
a:language-rust Rust API and codegen (mO,mS) breaking change Anything that probably require a semver bump rfc Request for comments: proposals for changes

Comments

@ogoffart
Copy link
Member

ogoffart commented Oct 7, 2022

Currently, one need to move a weak pointer of the component handle to the callback, then upgrade it in the callback.

Given a callback foo(int, int) -> int

Currently we have to do

let state2 = state.clone(); // where state is a Rc with the application state
let weak = my_ui.as_weak();    
my_ui.on_foo(move |bar, boo| {
        let my_ui = weak.unwrap();
        // do something with my_ui, state2, foo and bar 
    });

This gymnastic is a bit cumbersome. We need to improve that.

Some ideas include passing the handle directly in the callback:

let state2 = state.clone(); // where state is a Rc with the application state
my_ui.on_foo(|my_ui, bar, boo| {
            // do something with my_ui, state2, foo and bar 
   });

This already saves the creation of the weak for the component handle, This adds an extra argument to the callback.
(Should it be |my_ui, bar, boo| {...} or |my_ui, (bar, boo)| {...} ?)

For the extra applicaiton state we could add another Rc in the Component handle

// We add a Data template parameter create a MyUi<MyState>
let my_ui = MyUi::new_with_data(state.clone()); 
// ...
my_ui.on_foo(|my_ui, bar, boo| {
            // do something with my_ui, my_ui.data(), foo and bar 
 });

The problem is that it is a breaking change. Should we enable that only for the new component syntax? or shoud we have pragma or something? Or should we just go for it in a new semver major version?

@ogoffart ogoffart added a:language-rust Rust API and codegen (mO,mS) rfc Request for comments: proposals for changes breaking change Anything that probably require a semver bump labels Oct 7, 2022
@ogoffart
Copy link
Member Author

ogoffart commented Oct 7, 2022

We can also think a bit outside of the box and provide completely different syntax with macro or so.
eg, given a callback start-job(string)

    let engine = MyPrinterEngine::new();
   // ...
    let app = slint!{
        import PrinterWindow from "printer.slint";
        export App := PrinterWindow {
            // implement the callback with slint code in the slint! macro
            start_job(title) => rust!{
                engine.push_job(title);
            }
        }
    };

or using some other macro

    #[slint::callback(start_job(title))]
    {
        egine.push_job(title)
    }

@ogoffart
Copy link
Member Author

ogoffart commented Oct 7, 2022

Another usecase is to process the callback in a thread. Then we need to use some kind of message passing.
We could just add a message passing API on top of our current one. Eg:

    let (message_sender, message_processor) = slint::MessageSender::<PrinterEngine>::new(;
    std::thread::spawn(move || {
        // Construct the engine in the thread and process events in the thread
        let engine = PrinterEngine::new(main_window_weak);
        while true {
            message_processor.process_event(&engine)
        }
    });

    ////////////  Option A
    // the message_sender.exec will run the lambda on the thread, and give the engine as a parametter
    main_window
        .global::<PrinterQueue>()
        .on_start_job(move |title| massage_sender.exec(move |engine| engine.push_job(title)));

    /////////////// Option B
    // in order to avoid callback in callback, use an API that connects to a given callback type
    message_sender.register_callback(
        main_window.global::<PrinterQueue>().callback_start_job(),
        |engine, (title,)| engine.push_job(title),
    );

    ////////// Option C
    // in addtion to the on_*, we add a queued_* 
    main_window.global::<PrinterQueue>().queued_start_job(
        &message_sender,
        |/*main_window_weak,*/  engine, title| {
            engine.push_job(title);
            main_window_weak.upgrade_in_event_loop(|main_window| main_window.set_job_done(true));
        },
    );

@jturcotte
Copy link
Contributor

For the extra applicaiton state we could add another Rc in the Component handle

If the typing would allow it, for simpler cases it would be nice if I could also move my whole application state into the window and not have to wrap it into a RefCell and borrow it explicitly in every callback handler.

In those cases where having the main window or the main stack frame owning the application state wouldn’t make too much difference.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 7, 2022

Maybe worth opening a new issue for this, but something I've been wondering if narrowing the current 'static requirement for GUI callbacks to the lifetime of the Slint window could help replace some Rcs by ordinary references.

@jturcotte
Copy link
Contributor

One way to avoid breaking the API and keep allowing a different set of arguments could be to do it like it's interfaced for upgrade_in_event_loop.

// It's not exactly an upgrade from that point of view since this doesn't start from a Weak, the prefix could be a different one.
let state2 = state.clone(); // where state is a Rc with the application state
my_ui.upgrade_on_foo(move |my_ui, bar, boo| {
    // do something with my_ui, state2, foo and bar 
    });

@jturcotte
Copy link
Contributor

It would be nice if it would be possible to do this directly on globals too, as now I need to both upgrade the handle and re-fetch the global inside the callback before I can start writing code similar to what I can write directly in slint files.

For example I have a function like this in my slint file that I want to convert to Rust:

public function cycle_selection_mode() {
    selected_step_range_first = selected_step;
}

Now I need to do this:

let global_ui = &window.global::<GlobalUI>();

let weak = window.as_weak();
global_ui.on_cycle_selection_mode(move || {
    // Meh... and it needs to be on a separate line since handle sets the lifetime.
    let handle = weak.upgrade().unwrap();
    let this = handle.global::<GlobalUI>();

    this.set_selected_step_range_first(this.get_selected_step());
})

But it would be nice if all I had to do is this:

let global_ui = &window.global::<GlobalUI>();

global_ui.upgrade_on_cycle_selection_mode(move |this| {
    this.set_selected_step_range_first(this.get_selected_step());
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:language-rust Rust API and codegen (mO,mS) breaking change Anything that probably require a semver bump rfc Request for comments: proposals for changes
Projects
None yet
Development

No branches or pull requests

4 participants