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.

Sign up for GitHub

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

Jump to bottom

Add CallbackRef that takes ref in argument instead of value #3419

Merged
merged 8 commits into from
Sep 29, 2023
Merged

Add CallbackRef that takes ref in argument instead of value #3419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c1f6f4b
Add CallbackRef that takes ref in argument instead of value
cecton Sep 27, 2023
47c449a
Fix up commented reform/filter_reform
kirillsemyonkin Sep 27, 2023
912061d
Add reform_ref functions to create CallbackRef instead
cecton Sep 27, 2023
46f46e0
Use macro_rules to avoid code duplication
cecton Sep 27, 2023
ed5fdb5
Add tests and fix wrong types
cecton Sep 27, 2023
929f8c9
Update doc comments
cecton Sep 27, 2023
a153c0a
clippy
cecton Sep 27, 2023
844093c
more clippy
cecton Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
298 changes: 227 additions & 71 deletions packages/yew/src/callback.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,92 +9,194 @@ use std::rc::Rc;

use crate::html::ImplicitClone;

/// Universal callback wrapper.
///
/// An `Rc` wrapper is used to make it cloneable.
pub struct Callback<IN, OUT = ()> {
/// A callback which can be called multiple times
pub(crate) cb: Rc<dyn Fn(IN) -> OUT>,
}
macro_rules! generate_callback_impls {
($callback:ident, $in_ty:ty, $out_var:ident => $out_val:expr) => {
impl<IN, OUT, F: Fn($in_ty) -> OUT + 'static> From<F> for $callback<IN, OUT> {
fn from(func: F) -> Self {
$callback { cb: Rc::new(func) }
}
}

impl<IN, OUT, F: Fn(IN) -> OUT + 'static> From<F> for Callback<IN, OUT> {
fn from(func: F) -> Self {
Callback { cb: Rc::new(func) }
}
}
impl<IN, OUT> Clone for $callback<IN, OUT> {
fn clone(&self) -> Self {
Self {
cb: self.cb.clone(),
}
}
}

impl<IN, OUT> Clone for Callback<IN, OUT> {
fn clone(&self) -> Self {
Self {
cb: self.cb.clone(),
#[allow(clippy::vtable_address_comparisons)]
impl<IN, OUT> PartialEq for $callback<IN, OUT> {
fn eq(&self, other: &$callback<IN, OUT>) -> bool {
let ($callback { cb }, $callback { cb: rhs_cb }) = (self, other);
Rc::ptr_eq(cb, rhs_cb)
}
}
}
}

#[allow(clippy::vtable_address_comparisons)]
impl<IN, OUT> PartialEq for Callback<IN, OUT> {
fn eq(&self, other: &Callback<IN, OUT>) -> bool {
let (Callback { cb }, Callback { cb: rhs_cb }) = (self, other);
Rc::ptr_eq(cb, rhs_cb)
}
}
impl<IN, OUT> fmt::Debug for $callback<IN, OUT> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "$callback<_>")
}
}

impl<IN, OUT> fmt::Debug for Callback<IN, OUT> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "Callback<_>")
}
}
impl<IN, OUT> $callback<IN, OUT> {
/// This method calls the callback's function.
pub fn emit(&self, value: $in_ty) -> OUT {
(*self.cb)(value)
}
}

impl<IN, OUT> Callback<IN, OUT> {
/// This method calls the callback's function.
pub fn emit(&self, value: IN) -> OUT {
(*self.cb)(value)
}
impl<IN> $callback<IN> {
/// Creates a "no-op" callback which can be used when it is not suitable to use an
/// `Option<$callback>`.
pub fn noop() -> Self {
Self::from(|_: $in_ty| ())
}
}
Comment on lines +49 to +55
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 believe this is actually wrong because creating 2 "noop" callbacks and comparing them for equality will yield false

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine as it is trying to compare the address of 2 functions are not the content of the functions.

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 should have written a test -_-

The PartialEq implementation uses Rc::ptr_eq() so it's actually comparing that it is the same RC but we create one each time we call noop (or Default).

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3e6510283a5655f1166410d77fd62d51

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 made this PR to address this: #3430


impl<IN> Default for $callback<IN> {
fn default() -> Self {
Self::noop()
}
}

impl<IN: 'static, OUT: 'static> $callback<IN, OUT> {
/// Creates a new [`Callback`] from another callback and a function.
///
/// That when emitted will call that function and will emit the original callback
pub fn reform<F, T>(&self, func: F) -> Callback<T, OUT>
where
F: Fn(T) -> IN + 'static,
{
let this = self.clone();
let func = move |input: T| {
#[allow(unused_mut)]
let mut $out_var = func(input);
this.emit($out_val)
};
func.into()
}

/// Creates a new [`CallbackRef`] from another callback and a function.
///
/// That when emitted will call that function and will emit the original callback
pub fn reform_ref<F, T>(&self, func: F) -> CallbackRef<T, OUT>
where
F: Fn(&T) -> $in_ty + 'static,
{
let this = self.clone();
let func = move |input: &T| {
#[allow(unused_mut)]
let mut $out_var = func(input);
this.emit($out_val)
};
func.into()
}

/// Creates a new [`CallbackRefMut`] from another callback and a function.
///
/// That when emitted will call that function and will emit the original callback
pub fn reform_ref_mut<F, T>(&self, func: F) -> CallbackRefMut<T, OUT>
where
F: Fn(&mut T) -> $in_ty + 'static,
{
let this = self.clone();
let func = move |input: &mut T| {
#[allow(unused_mut)]
let mut $out_var = func(input);
this.emit($out_val)
};
func.into()
}

/// Creates a new [`Callback`] from another callback and a function.
///
/// When emitted will call the function and, only if it returns `Some(value)`, will emit
/// `value` to the original callback.
pub fn filter_reform<F, T>(&self, func: F) -> Callback<T, Option<OUT>>
where
F: Fn(T) -> Option<IN> + 'static,
{
let this = self.clone();
let func = move |input: T| {
func(input).map(
#[allow(unused_mut)]
|mut $out_var| this.emit($out_val),
)
};
func.into()
}

/// Creates a new [`CallbackRef`] from another callback and a function.
///
/// When emitted will call the function and, only if it returns `Some(value)`, will emit
/// `value` to the original callback.
pub fn filter_reform_ref<F, T>(&self, func: F) -> CallbackRef<T, Option<OUT>>
where
F: Fn(&T) -> Option<$in_ty> + 'static,
{
let this = self.clone();
let func = move |input: &T| {
func(input).map(
#[allow(unused_mut)]
|mut $out_var| this.emit($out_val),
)
};
func.into()
}

/// Creates a new [`CallbackRefMut`] from another callback and a function.
///
/// When emitted will call the function and, only if it returns `Some(value)`, will emit
/// `value` to the original callback.
pub fn filter_reform_ref_mut<F, T>(&self, func: F) -> CallbackRefMut<T, Option<OUT>>
where
F: Fn(&mut T) -> Option<$in_ty> + 'static,
{
let this = self.clone();
let func = move |input: &mut T| {
func(input).map(
#[allow(unused_mut)]
|mut $out_var| this.emit($out_val),
)
};
func.into()
}
}

impl<IN, OUT> ImplicitClone for $callback<IN, OUT> {}
};
}

impl<IN> Callback<IN> {
/// Creates a "no-op" callback which can be used when it is not suitable to use an
/// `Option<Callback>`.
pub fn noop() -> Self {
Self::from(|_| ())
}
/// Universal callback wrapper.
///
/// An `Rc` wrapper is used to make it cloneable.
pub struct Callback<IN, OUT = ()> {
/// A callback which can be called multiple times
pub(crate) cb: Rc<dyn Fn(IN) -> OUT>,
}

impl<IN> Default for Callback<IN> {
fn default() -> Self {
Self::noop()
}
generate_callback_impls!(Callback, IN, output => output);

/// Universal callback wrapper with reference in argument.
///
/// An `Rc` wrapper is used to make it cloneable.
pub struct CallbackRef<IN, OUT = ()> {
/// A callback which can be called multiple times
pub(crate) cb: Rc<dyn Fn(&IN) -> OUT>,
}

impl<IN: 'static, OUT: 'static> Callback<IN, OUT> {
/// Creates a new callback from another callback and a function
/// That when emitted will call that function and will emit the original callback
pub fn reform<F, T>(&self, func: F) -> Callback<T, OUT>
where
F: Fn(T) -> IN + 'static,
{
let this = self.clone();
let func = move |input| {
let output = func(input);
this.emit(output)
};
Callback::from(func)
}
generate_callback_impls!(CallbackRef, &IN, output => #[allow(clippy::needless_borrow)] &output);

/// Creates a new callback from another callback and a function.
/// When emitted will call the function and, only if it returns `Some(value)`, will emit
/// `value` to the original callback.
pub fn filter_reform<F, T>(&self, func: F) -> Callback<T, Option<OUT>>
where
F: Fn(T) -> Option<IN> + 'static,
{
let this = self.clone();
let func = move |input| func(input).map(|output| this.emit(output));
Callback::from(func)
}
/// Universal callback wrapper with mutable reference in argument.
///
/// An `Rc` wrapper is used to make it cloneable.
pub struct CallbackRefMut<IN, OUT = ()> {
/// A callback which can be called multiple times
pub(crate) cb: Rc<dyn Fn(&mut IN) -> OUT>,
}

impl<IN, OUT> ImplicitClone for Callback<IN, OUT> {}
generate_callback_impls!(CallbackRefMut, &mut IN, output => &mut output);

#[cfg(test)]
mod test {
Expand Down Expand Up @@ -144,4 +246,58 @@ mod test {
vec![true, false]
);
}

#[test]
fn test_ref() {
let callback: CallbackRef<usize, usize> = CallbackRef::from(|x: &usize| *x);
assert_eq!(callback.emit(&42), 42);
}

#[test]
fn test_ref_mut() {
let callback: CallbackRefMut<usize, ()> = CallbackRefMut::from(|x: &mut usize| *x = 42);
let mut value: usize = 0;
callback.emit(&mut value);
assert_eq!(value, 42);
}

#[test]
fn test_reform_ref() {
let callback: Callback<usize, usize> = Callback::from(|x: usize| x + 1);
let reformed: CallbackRef<usize, usize> = callback.reform_ref(|x: &usize| *x + 2);
assert_eq!(reformed.emit(&42), 45);
}

#[test]
fn test_reform_ref_mut() {
let callback: CallbackRefMut<usize, ()> = CallbackRefMut::from(|x: &mut usize| *x += 1);
let reformed: CallbackRefMut<usize, ()> = callback.reform_ref_mut(|x: &mut usize| {
*x += 2;
x
});
let mut value: usize = 42;
reformed.emit(&mut value);
assert_eq!(value, 45);
}

#[test]
fn test_filter_reform_ref() {
let callback: Callback<usize, usize> = Callback::from(|x: usize| x + 1);
let reformed: CallbackRef<usize, Option<usize>> =
callback.filter_reform_ref(|x: &usize| Some(*x + 2));
assert_eq!(reformed.emit(&42), Some(45));
}

#[test]
fn test_filter_reform_ref_mut() {
let callback: CallbackRefMut<usize, ()> = CallbackRefMut::from(|x: &mut usize| *x += 1);
let reformed: CallbackRefMut<usize, Option<()>> =
callback.filter_reform_ref_mut(|x: &mut usize| {
*x += 2;
Some(x)
});
let mut value: usize = 42;
reformed.emit(&mut value).expect("is some");
assert_eq!(value, 45);
}
}
2 changes: 1 addition & 1 deletion packages/yew/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ pub mod prelude {

#[cfg(feature = "csr")]
pub use crate::app_handle::AppHandle;
pub use crate::callback::Callback;
pub use crate::callback::{Callback, CallbackRef, CallbackRefMut};
pub use crate::context::{ContextHandle, ContextProvider};
pub use crate::events::*;
pub use crate::functional::*;
Expand Down
Loading