#757 Refactor script feedback implementation draft

main/src/domain/lua_feedback_script.rs

@@ -2,7 +2,7 @@ use crate::base::SendOrSyncWhatever;
 use crate::domain::{SafeLua, ScriptColor, ScriptFeedbackEvent, ScriptFeedbackValue};
 use helgoboss_learn::{
     FeedbackScript, FeedbackScriptInput, FeedbackScriptOutput, FeedbackValue, NumericValue,
-    PropValue,
+    PropProvider, PropValue,
 };
 use mlua::{Function, Lua, LuaSerdeExt, Table, ToLua, Value};
 use std::borrow::Cow;
@@ -42,21 +42,20 @@ impl<'lua> LuaFeedbackScript<'lua> {
         input: FeedbackScriptInput,
     ) -> Result<FeedbackScriptOutput, Box<dyn Error>> {
         let lua = self.lua.as_ref();
-        // We use the mlua "send" feature to make Lua instances implement Send. However, here
-        // for once we have a Rust function that is not Send, so create_function() complains.
-        // We ignore this because in our usage scenario (= async immediate execution), we don't
+        let thin_ref = &input.prop_provider;
+        // We need to use the Trafficker here because mlua requires the input to create_function()
+        // to be 'static and Send. However, here we have a Rust function that doesn't fulfill any
+        // of these requirements, so create_function() would complain. However, in this case, the
+        // requirements are unnecessarily strict. Because in our usage scenario (= synchronous
+        // immediate execution, just once), the function can't go out of scope and we also don't
         // send anything to another thread.
-        let thin_ref = &input.get_prop_value;
-        let thin_ptr = thin_ref as *const _ as *const c_void;
-        let thin_ptr_wrapper = unsafe { SendOrSyncWhatever::new(thin_ptr) };
+        let trafficker = Trafficker::new(thin_ref);
         // Build input data
         let context_table = {
             let table = lua.create_table()?;
             let prop = lua.create_function(move |_, key: String| {
-                let thin_ptr = *thin_ptr_wrapper.get();
-                let thin_ref = unsafe { &*(thin_ptr as *const &dyn Fn(&str) -> Option<PropValue>) };
-                let get_prop_value = *thin_ref;
-                let prop_value = (get_prop_value)(&key);
+                let prop_provider: &dyn PropProvider = unsafe { trafficker.get() };
+                let prop_value = prop_provider.get_prop_value(&key);
                 Ok(prop_value.map(LuaPropValue))
             })?;
             table.set("prop", prop)?;
@@ -113,24 +112,35 @@ struct LuaScriptOutput {
     feedback_event: Option<ScriptFeedbackEvent>,
 }
 
-struct Trafficker<T> {
+/// This utility provides a way to pass a trait object reference that is neither `Send` nor
+/// `'static` into functions that require these traits.
+///
+/// Dangerous stuff and rarely necessary! You go down to C level with this.
+struct Trafficker {
     thin_ptr: *const c_void,
-    _p: PhantomData<T>,
 }
 
-unsafe impl<T> Send for Trafficker<T> {}
+unsafe impl Send for Trafficker {}
 
-impl<T: Copy> Trafficker<T> {
-    pub fn new(val: T) -> Self {
-        let thin_ref = &val;
+impl Trafficker {
+    /// Put a reference to a trait object reference in here (`&&dyn ...`).
+    ///
+    /// We need a reference to a reference here because
+    pub fn new<T: Copy>(thin_ref: &T) -> Self {
         let thin_ptr = thin_ref as *const _ as *const c_void;
-        Self {
-            thin_ptr,
-            _p: Default::default(),
-        }
+        Self { thin_ptr }
     }
 
-    pub unsafe fn get_ref(&self) -> T {
+    /// Get it out again.
+    ///
+    /// Make sure you use the same type as in `new`! We can't make `T` a type parameter of the
+    /// struct because otherwise the borrow checker would complain that things go out of scope.
+    ///
+    /// # Safety
+    ///
+    /// If you don't provide the proper type or the reference passed to `new` went out of scope,
+    /// things crash horribly.
+    pub unsafe fn get<T: Copy>(&self) -> T {
         *(self.thin_ptr as *const T)
     }
 }
@@ -155,7 +165,7 @@ mod tests {
         let script = LuaFeedbackScript::compile(&lua, text).unwrap();
         // When
         let input = FeedbackScriptInput {
-            get_prop_value: &|_| None,
+            prop_provider: &|_: &str| None,
         };
         let output = script.feedback(input).unwrap();
         // Then
@@ -177,7 +187,7 @@ mod tests {
         let script = LuaFeedbackScript::compile(&lua, text).unwrap();
         // When
         let input = FeedbackScriptInput {
-            get_prop_value: &|_| None,
+            prop_provider: &|_: &str| None,
         };
         let output = script.feedback(input).unwrap();
         // Then
@@ -207,7 +217,7 @@ mod tests {
         let script = LuaFeedbackScript::compile(&lua, text).unwrap();
         // When
         let input = FeedbackScriptInput {
-            get_prop_value: &|_| None,
+            prop_provider: &|_: &str| None,
         };
         let output = script.feedback(input).unwrap();
         // Then
@@ -234,7 +244,7 @@ mod tests {
         let script = LuaFeedbackScript::compile(&lua, text).unwrap();
         // When
         let input = FeedbackScriptInput {
-            get_prop_value: &|key| match key {
+            prop_provider: &|key: &str| match key {
                 "name" => Some(PropValue::Text("hello".into())),
                 _ => None,
             },

main/src/domain/mapping.rs

@@ -1,15 +1,15 @@
 use crate::domain::{
-    get_prop_value, prop_feedback_resolution, prop_is_affected_by, ActivationChange,
-    ActivationCondition, BoxedHitInstruction, CompartmentParamIndex, CompoundChangeEvent,
-    ControlContext, ControlEvent, ControlEventTimestamp, ControlOptions, ExtendedProcessorContext,
-    FeedbackResolution, GroupId, HitResponse, KeyMessage, KeySource, MappingActivationEffect,
-    MappingControlContext, MappingData, MappingInfo, MessageCaptureEvent, MidiScanResult,
-    MidiSource, Mode, OscDeviceId, OscScanResult, PersistentMappingProcessingState,
-    PluginParamIndex, PluginParams, RealTimeMappingUpdate, RealTimeReaperTarget,
-    RealTimeTargetUpdate, RealearnParameterChangePayload, RealearnParameterSource, RealearnTarget,
-    ReaperMessage, ReaperSource, ReaperSourceFeedbackValue, ReaperTarget, ReaperTargetType, Tag,
-    TargetCharacter, TrackExclusivity, UnresolvedReaperTarget, VirtualControlElement,
-    VirtualFeedbackValue, VirtualSource, VirtualSourceAddress, VirtualSourceValue, VirtualTarget,
+    prop_feedback_resolution, prop_is_affected_by, ActivationChange, ActivationCondition,
+    BoxedHitInstruction, CompartmentParamIndex, CompoundChangeEvent, ControlContext, ControlEvent,
+    ControlEventTimestamp, ControlOptions, ExtendedProcessorContext, FeedbackResolution, GroupId,
+    HitResponse, KeyMessage, KeySource, MappingActivationEffect, MappingControlContext,
+    MappingData, MappingInfo, MappingPropProvider, MessageCaptureEvent, MidiScanResult, MidiSource,
+    Mode, OscDeviceId, OscScanResult, PersistentMappingProcessingState, PluginParamIndex,
+    PluginParams, RealTimeMappingUpdate, RealTimeReaperTarget, RealTimeTargetUpdate,
+    RealearnParameterChangePayload, RealearnParameterSource, RealearnTarget, ReaperMessage,
+    ReaperSource, ReaperSourceFeedbackValue, ReaperTarget, ReaperTargetType, Tag, TargetCharacter,
+    TrackExclusivity, UnresolvedReaperTarget, VirtualControlElement, VirtualFeedbackValue,
+    VirtualSource, VirtualSourceAddress, VirtualSourceValue, VirtualTarget,
     COMPARTMENT_PARAMETER_COUNT,
 };
 use derive_more::Display;
@@ -19,8 +19,8 @@ use helgoboss_learn::{
     format_percentage_without_unit, parse_percentage_without_unit, AbsoluteValue, ControlResult,
     ControlType, ControlValue, FeedbackValue, GroupInteraction, MidiSourceAddress, MidiSourceValue,
     ModeControlOptions, ModeControlResult, ModeFeedbackOptions, NumericFeedbackValue, NumericValue,
-    OscSource, OscSourceAddress, PreliminaryMidiSourceFeedbackValue, PropValue, RawMidiEvent,
-    SourceCharacter, SourceContext, Target, UnitValue, ValueFormatter, ValueParser,
+    OscSource, OscSourceAddress, PreliminaryMidiSourceFeedbackValue, PropProvider, PropValue,
+    RawMidiEvent, SourceCharacter, SourceContext, Target, UnitValue, ValueFormatter, ValueParser,
 };
 use helgoboss_midi::{Channel, RawShortMessage, ShortMessage};
 use num_enum::{IntoPrimitive, TryFromPrimitive};
@@ -1127,15 +1127,11 @@ impl MainMapping {
         //   form of feedback it sends, it just provides us with options and we can choose.
         // - This leaves us with asking the mode. That means the user needs to explicitly choose
         //   whether it wants numerical or textual feedback.
+        let prop_provider = MappingPropProvider::new(self, control_context);
         let feedback_value = if self.core.mode.wants_advanced_feedback() {
-            self.core
-                .mode
-                .build_feedback(&|key| get_prop_value(key, self, control_context))
+            self.core.mode.build_feedback(&prop_provider)
         } else {
-            let style = self
-                .core
-                .mode
-                .feedback_style(&|key| get_prop_value(key, self, control_context));
+            let style = self.core.mode.feedback_style(&prop_provider);
             FeedbackValue::Numeric(NumericFeedbackValue::new(style, combined_target_value))
         };
         let source_feedback_is_okay = if self.core.options.feedback_send_behavior

main/src/domain/props.rs

@@ -4,7 +4,7 @@ use crate::domain::{
     UnresolvedCompoundMappingTarget,
 };
 use enum_dispatch::enum_dispatch;
-use helgoboss_learn::{PropValue, Target};
+use helgoboss_learn::{PropProvider, PropValue, Target};
 use realearn_api::persistence::TrackScope;
 use reaper_high::ChangeEvent;
 use std::str::FromStr;
@@ -54,20 +54,31 @@ pub fn prop_is_affected_by(
     }
 }
 
-pub fn get_prop_value(
-    key: &str,
-    mapping: &MainMapping,
-    control_context: ControlContext,
-) -> Option<PropValue> {
-    match key.parse::<Props>().ok() {
-        Some(props) => props.get_value(mapping, mapping.targets().first(), control_context),
-        None => {
-            if let (Some(key), Some(target)) =
-                (key.strip_prefix("target."), mapping.targets().first())
-            {
-                target.prop_value(key, control_context)
-            } else {
-                None
+pub struct MappingPropProvider<'a> {
+    mapping: &'a MainMapping,
+    context: ControlContext<'a>,
+}
+
+impl<'a> MappingPropProvider<'a> {
+    pub fn new(mapping: &'a MainMapping, context: ControlContext<'a>) -> Self {
+        Self { mapping, context }
+    }
+}
+
+impl<'a> PropProvider for MappingPropProvider<'a> {
+    fn get_prop_value(&self, key: &str) -> Option<PropValue> {
+        match key.parse::<Props>().ok() {
+            Some(props) => {
+                props.get_value(self.mapping, self.mapping.targets().first(), self.context)
+            }
+            None => {
+                if let (Some(key), Some(target)) =
+                    (key.strip_prefix("target."), self.mapping.targets().first())
+                {
+                    target.prop_value(key, self.context)
+                } else {
+                    None
+                }
             }
         }
     }

main/src/infrastructure/ui/simple_script_editor_panel.rs

@@ -62,7 +62,7 @@ impl ScriptEngine for LuaFeedbackScriptEngine {
     fn compile(&self, code: &str) -> Result<Box<dyn Script>, Box<dyn Error>> {
         let script = LuaFeedbackScript::compile(&self.lua, code)?;
         let test_input = FeedbackScriptInput {
-            get_prop_value: &|_| None,
+            prop_provider: &|_: &str| None,
         };
         script.feedback(test_input)?;
         Ok(Box::new(()))