Safe wrapper for ViewDispatcher and multiple mutable borrows

[mogenson]

Hi. I'd like to make a safe wrapper around sys::ViewDispatcher. Like furi thread, I'd like to use closures for callbacks instead of function pointers. As per the discussion from #80, it sounds like the right thing to do would be to make the ViewDispatcher methods take &mut self, since the C code will take a raw *mut sys::ViewDispatcher pointer and mutate the view dispatcher. Although it's worth noting that the NonNull type will happily produce a *mut T from a &T without any compiler complaints.

However, most of the time in a view dispatcher callback / closure, you want to do something with the view dispatcher. This leads to a multiple mutable borrows error. For example:

pub struct ViewDispatcher {
    data: NonNull<sys::ViewDispatcher>,
}

impl ViewDispatcher {
    pub fn set_navigation_event_callback<F>(&mut self, f: F)
    where
        F: FnMut() -> bool + 'static,
    {
        let callback = Box::new(f);
        unsafe extern "C" fn trampoline(context: *mut c_void) -> bool {
            let mut callback = context as *mut FnMut() -> bool;
            callback()
        }

        unsafe {
            sys::view_dispatcher_set_event_callback_context(
                self.data.as_ptr(),
                &*callback as *mut c_void,
            );
            sys::view_dispatcher_set_navigation_event_callback(self.data.as_ptr(), Some(trampoline))
        };
    }
    
    pub fn run(&mut self) {
        unsafe { sys::view_dispatcher_run(self.data.as_ptr()) };
    }
}

And an example program:

fn main(_args: *mut u8) -> i32 {
    let mut view_dispatcher = ViewDispatcher::new(ViewDispatcherType::Fullscreen);

    view_dispatcher.set_navigation_event_callback(|| {
        view_dispatcher.stop();
        true
    });

    view_dispatcher.run();
    
    0
 }

Here view_dispatcher is borrowed as mut by the closure, set_navigation_event_callback, and run. Using the usual Rust tools of wrapping view_dispatcher in an Rc<RefCell<T>> doesn't work because view_dispatcher is in fact borrowed mutably in multiple places at the same time so borrow_mut() will panic. Quick note: all the view dispatcher callbacks are invoked from the thread that called run() so no Arc<Mutex<T>> should be required.

One idea is to make all of the ViewDispatcher methods take &self, even though they may mutate the wrapped sys::ViewDispatcher. Then we can pass/capture cloned / immutable Rc<T>s without complaint.

Another idea is to pass &mut ViewDispatcher as an argument to the closures.

type NavigationEventCallback = Box<dyn FnMut(&mut ViewDispatcher) -> bool>;

pub struct ViewDispatcher {
    data: NonNull<sys::ViewDispatcher>,
    navigation_event_callback: Option<NavigationEventCallback>,
}

    pub fn set_navigation_event_callback<F>(&mut self, f: F)
    where
        F: FnMut(&mut ViewDispatcher) -> bool + 'static,
    {
        let callback: NavigationEventCallback = Box::new(f);
        unsafe extern "C" fn trampoline(context: *mut c_void) -> bool {
            let view_dispatcher = &mut *(context as *mut ViewDispatcher);
            // take the callback and replace it after invoking so we don't have to borrow it
            let mut navigation_event_callback = view_dispatcher.navigation_event_callback.take();
            let mut ret = false;
            if let Some(ref mut navigation_event_callback) = navigation_event_callback {
                ret = navigation_event_callback(view_dispatcher);
            }
            view_dispatcher.navigation_event_callback = navigation_event_callback;
            return ret;
        }

        self.navigation_event_callback = Some(callback);
        unsafe {
              sys::view_dispatcher_set_event_callback_context(
                self.data.as_ptr(),
                &mut self as *mut _ as *mut c_void,
            );
            sys::view_dispatcher_set_navigation_event_callback(self.data.as_ptr(), Some(trampoline))
        };
    }
}

This works until we want to use a mutable method (like send_custom_event()) for our view dispatcher from another place (like an input event callback). We've exposed our view dispatcher as mutable in the view dispatcher callbacks, but it's still borrowed as mutable and unavailable to use else ware.

@dcoles @str4d @JarvisCraft Do you have any thoughts on this potential design?

[JarvisCraft]

Hi! My current experience with this APIs shows that there is a number of pitfalls with ViewDispatcher APIs, i.e.:

• it has quite complicated initialization logic involving queue setup (some methods are only valid after it has been set up correctly);
• the same is true for destruction since it must have quite specific order;
• it follows the same problems as other GUI APIs.

As for the callbacks, I suggest that you look at the approach currently used in #29 which.

My personal suggestion would be to join our efforts working on #29.

[mogenson]

@JarvisCraft Thanks for the suggestion. I took a look at PR #29 and tried implementing a view dispatcher wrapper using the callback approach you defined:

//! ViewDispatcher APIs

use crate::gui::Gui;
use alloc::boxed::Box;
use core::{
    ffi::c_void,
    ptr::{self, NonNull},
};

use flipperzero_sys::{
    self as sys, ViewDispatcher as SysViewDispatcher, ViewDispatcherType as SysViewDispatcherType,
};

/// ViewDispatcherType
#[derive(Debug, Clone, Copy)]
pub enum ViewDispatcherType {
    /// Desktop view dispatcher type.
    Desktop,
    /// Window view dispatcher type.
    Window,
    /// Fullscreen view dispatcher type.
    Fullscreen,
}

impl From<ViewDispatcherType> for SysViewDispatcherType {
    fn from(value: ViewDispatcherType) -> Self {
        match value {
            ViewDispatcherType::Desktop => sys::ViewDispatcherType_ViewDispatcherTypeDesktop,
            ViewDispatcherType::Window => sys::ViewDispatcherType_ViewDispatcherTypeWindow,
            ViewDispatcherType::Fullscreen => sys::ViewDispatcherType_ViewDispatcherTypeFullscreen,
        }
    }
}

/// System ViewDispatcher
pub struct ViewDispatcher<C: ViewDispatcherCallbacks> {
    raw: NonNull<SysViewDispatcher>,
    callbacks: NonNull<C>,
}

impl<C: ViewDispatcherCallbacks> ViewDispatcher<C> {
    /// Creates and initializes a new `ViewDispatcher` with callbacks
    pub fn new(view_dispatcher_type: ViewDispatcherType, callbacks: C) -> Self {
        let raw = unsafe { NonNull::new_unchecked(sys::view_dispatcher_alloc()) };
        let callbacks = Box::into_raw(Box::new(callbacks));

        let view_dispatcher = {
            let callbacks = unsafe { NonNull::new_unchecked(callbacks) };
            Self { raw, callbacks }
        };

        let raw = raw.as_ptr();
        unsafe { sys::view_dispatcher_enable_queue(raw) };

        let context = callbacks.cast();
        unsafe { sys::view_dispatcher_set_event_callback_context(raw, context) };

        let gui = Gui::new();
        unsafe {
            sys::view_dispatcher_attach_to_gui(raw, gui.as_raw(), view_dispatcher_type.into())
        };

        pub unsafe extern "C" fn dispatch_custom_event<C: ViewDispatcherCallbacks>(
            context: *mut c_void,
            event: u32,
        ) -> bool {
            let context: *mut C = context.cast();
            unsafe { &mut *context }.on_custom_event(event)
        }

        if !ptr::eq(
            C::on_custom_event as *const c_void,
            <() as ViewDispatcherCallbacks>::on_custom_event as *const c_void,
        ) {
            let callback = Some(dispatch_custom_event::<C> as _);
            unsafe { sys::view_dispatcher_set_custom_event_callback(raw, callback) };
        }

        pub unsafe extern "C" fn dispatch_navigation_event<C: ViewDispatcherCallbacks>(
            context: *mut c_void,
        ) -> bool {
            let context: *mut C = context.cast();
            unsafe { &mut *context }.on_navigation_event()
        }

        if !ptr::eq(
            C::on_navigation_event as *const c_void,
            <() as ViewDispatcherCallbacks>::on_navigation_event as *const c_void,
        ) {
            let callback = Some(dispatch_navigation_event::<C> as _);
            unsafe { sys::view_dispatcher_set_navigation_event_callback(raw, callback) };
        }

        pub unsafe extern "C" fn dispatch_tick_event<C: ViewDispatcherCallbacks>(
            context: *mut c_void,
        ) {
            let context: *mut C = context.cast();
            unsafe { &mut *context }.on_tick_event();
        }

        if !ptr::eq(
            C::on_tick_event as *const c_void,
            <() as ViewDispatcherCallbacks>::on_tick_event as *const c_void,
        ) {
            let callback = Some(dispatch_tick_event::<C> as _);
            unsafe {
                sys::view_dispatcher_set_tick_event_callback(raw, callback, C::get_tick_period())
            };
        }

        view_dispatcher
    }

    /// Start view dispatcher event loop. This method blocks until `stop()` is called.
    pub fn run(&self) {
        unsafe { sys::view_dispatcher_run(self.raw.as_ptr()) };
    }

    /// Send message to exit view dispatcher event loop.
    pub fn stop(&self) {
        unsafe { sys::view_dispatcher_stop(self.raw.as_ptr()) };
    }

    /// Register a view
    pub fn add_view(&self, view_id: u32, view: *mut sys::View) {
        unsafe { sys::view_dispatcher_add_view(self.raw.as_ptr(), view_id, view) };
    }

    /// Remove a view
    pub fn remove_view(&self, view_id: u32) {
        unsafe { sys::view_dispatcher_remove_view(self.raw.as_ptr(), view_id) };
    }

    /// Switch to a view
    pub fn switch_to_view(&self, view_id: u32) {
        unsafe { sys::view_dispatcher_switch_to_view(self.raw.as_ptr(), view_id) };
    }

    /// Send view dispatcher's view port to front
    pub fn send_to_front(&self) {
        unsafe { sys::view_dispatcher_send_to_front(self.raw.as_ptr()) };
    }

    /// Send view dispatcher's view port to back
    pub fn send_to_back(&self) {
        unsafe { sys::view_dispatcher_send_to_back(self.raw.as_ptr()) };
    }

    /// Send a custom event to the the custom event callback handler
    pub fn send_custom_event(&self, event: u32) {
        unsafe { sys::view_dispatcher_send_custom_event(self.raw.as_ptr(), event) };
    }
}

pub trait ViewDispatcherCallbacks {
    fn get_tick_period() -> u32 {
        0
    }
    fn on_custom_event(&mut self, _event: u32) -> bool {
        false
    }
    fn on_navigation_event(&mut self) -> bool {
        false
    }
    fn on_tick_event(&mut self) {}
}

impl ViewDispatcherCallbacks for () {}

I'm still struggling with how to get access to the view dispatcher instance within a view dispatcher callback. Here's a small example program:

#![no_main]
#![no_std]

extern crate flipperzero_alloc as alloc;
extern crate flipperzero_rt;

use core::ptr::NonNull;
use flipperzero::gui::canvas::{Align, Font};
use flipperzero::gui::view_dispatcher::{
    ViewDispatcher, ViewDispatcherCallbacks, ViewDispatcherType,
};
use flipperzero::println;
use flipperzero_rt::{entry, manifest};
use flipperzero_sys::{self as sys, c_string};

manifest!(name = "Rust ViewDispatcher example");
entry!(main);

fn main(_args: *mut u8) -> i32 {
    struct State {
        // view_dispatcher: Option<ViewDispatcher>,
    }

    impl ViewDispatcherCallbacks for State {
        fn on_navigation_event(&mut self) -> bool {
            println!("back button pressed");
            // self.view_dispatcher.unwrap().stop();
            true
        }
    }

    let state = State {
        // view_dispatcher: None,
    };

    let view_dispatcher = ViewDispatcher::new(ViewDispatcherType::Fullscreen, state);

    let widget = unsafe { NonNull::new_unchecked(sys::widget_alloc()) };
    unsafe {
        sys::widget_add_string_element(
            widget.as_ptr(),
            64,
            32,
            Align::Center.into(),
            Align::Center.into(),
            Font::Primary.into(),
            c_string!("Hello"),
        )
    };

    view_dispatcher.add_view(0, unsafe { sys::widget_get_view(widget.as_ptr()) });
    view_dispatcher.switch_to_view(0);

    view_dispatcher.run();

    unsafe { sys::widget_free(widget.as_ptr()) };

    0
}

As you can see, the view dispatcher callbacks are in their own scope so they can't access variables from main(). Also state is consumed by new() so we can't partially initialize it and finish later. Is there a clever way to get a reference to view_dispatcher inside of state, or do I need to do something like pass a mutable reference to the view dispatcher instance as an argument to every callback?

[JarvisCraft]

You could store copy of NonNull<SysViewDispatcher> in the context like:

// unsafe to create since may have dangling pointer
struct Context<C: ViewDispatcherCallbacks, 'a> {
    // what we also need for the context
    view_port NonNull<SysViewPort>,
    // what is currently stored in the context
    callbacks: C,
    // this struct should only live for the lifetime of callback handler
    _lifetime: PhantomReference<&'a ()>
}

Although you should be extra careful here withj pointer aliasing rules (I still am unsure whether my implementations are sound which may require MIRI to check).

[mogenson]

One more question. I'm very new to traits. How can I put an argument of type ViewDispatcher<ViewDispatcherCallbacks> in a function that's part of the ViewDispatcherCallbacks trait? eg:

pub trait ViewDispatcherCallbacks {
    fn get_tick_period() -> u32 {
        0
    }
    fn on_custom_event(
        &mut self,
        _view_dispatcher: &mut ViewDispatcher<ViewDispatcherCallbacks>,
        _event: u32,
    ) -> bool {
        false
    }
    fn on_navigation_event(
        &mut self,
        _view_dispatcher: &mut ViewDispatcher<ViewDispatcherCallbacks>,
    ) -> bool {
        false
    }
    fn on_tick_event(&mut self, _view_dispatcher: &mut ViewDispatcher<ViewDispatcherCallbacks>) {}
}

The compiler is complaining about unknown size for the trait.

[mogenson]

As a work-around I can pull out all of the callback generics into a separate ViewDispatcherBuilder type and keep ViewDispatcher as a simple wrapper.

pub struct ViewDispatcher {
    raw: NonNull<SysViewDispatcher>,
}

pub struct ViewDispatcherBuilder<C: ViewDispatcherCallbacks> {
    raw: NonNull<SysViewDispatcher>,
    callbacks: NonNull<C>,
}

impl<C: ViewDispatcherCallbacks> ViewDispatcherBuilder<C> {
    /// Creates and initializes a new `ViewDispatcher` with callbacks
    pub fn new(view_dispatcher_type: ViewDispatcherType, callbacks: C) -> ViewDispatcher { /* setup callbacks */ }
}

impl ViewDispatcher {
/* methods like run, stop, add_view */
}

[dcoles]

One more question. I'm very new to traits. How can I put an argument of type ViewDispatcher<ViewDispatcherCallbacks> in a function that's part of the ViewDispatcherCallbacks trait?

I'm not really familiar with ViewDispatcher and friends (that would be @JarvisCraft), but to store callbacks: C in Context then C: ViewDispatcherCallbacks must be Sized; otherwise the compiler doesn't know how much space to reserve in the struct for C. This would not be a problem if it was callbacks: &dyn C or Callbacks: Box<C> since both Box and &dyn C have sizes known at compile time.