#744 Fix load order issues

main/src/application/session.rs

@@ -1,6 +1,6 @@
 use crate::application::{
-    get_track_label, get_virtual_track_label, share_group, share_mapping, Affected, Change,
-    ChangeResult, CompartmentCommand, CompartmentModel, CompartmentProp, ControllerPreset, FxId,
+    get_track_label, share_group, share_mapping, Affected, Change, ChangeResult,
+    CompartmentCommand, CompartmentModel, CompartmentProp, ControllerPreset, FxId,
     FxPresetLinkConfig, GroupCommand, GroupModel, MainPreset, MainPresetAutoLoadMode,
     MappingCommand, MappingModel, MappingProp, Preset, PresetLinkManager, PresetManager,
     ProcessingRelevance, SharedGroup, SharedMapping, SourceModel, TargetCategory, TargetModel,
@@ -1516,6 +1516,19 @@ impl Session {
         })
     }
 
+    pub fn find_mapping_by_key(
+        &self,
+        compartment: Compartment,
+        key: &MappingKey,
+    ) -> Option<SharedMapping> {
+        self.mappings(compartment)
+            .find(|m| {
+                let m = m.borrow();
+                m.key() == key
+            })
+            .cloned()
+    }
+
     pub fn mappings(&self, compartment: Compartment) -> impl Iterator<Item = &SharedMapping> {
         self.mappings[compartment].iter()
     }

main/src/application/target_model.rs

@@ -20,10 +20,10 @@ use crate::domain::{
     find_bookmark, get_fx_name, get_fx_params, get_non_present_virtual_route_label,
     get_non_present_virtual_track_label, get_track_routes, ActionInvocationType, AnyOnParameter,
     Compartment, CompoundMappingTarget, Exclusivity, ExpressionEvaluator, ExtendedProcessorContext,
-    FeedbackResolution, FxDescriptor, FxDisplayType, FxParameterDescriptor, GroupId, InstanceId,
-    MappingId, MappingSnapshotId, MouseActionType, OscDeviceId, PotFilterItemsTargetSettings,
-    ProcessorContext, RealearnTarget, ReaperTarget, ReaperTargetType, SeekOptions,
-    SendMidiDestination, SoloBehavior, Tag, TagScope, TouchedRouteParameterType,
+    FeedbackResolution, FxDescriptor, FxDisplayType, FxParameterDescriptor, GroupId, MappingId,
+    MappingKey, MappingRef, MappingSnapshotId, MouseActionType, OscDeviceId,
+    PotFilterItemsTargetSettings, ProcessorContext, RealearnTarget, ReaperTarget, ReaperTargetType,
+    SeekOptions, SendMidiDestination, SoloBehavior, Tag, TagScope, TouchedRouteParameterType,
     TouchedTrackParameterType, TrackDescriptor, TrackExclusivity, TrackGangBehavior,
     TrackRouteDescriptor, TrackRouteSelector, TrackRouteType, TransportAction,
     UnresolvedActionTarget, UnresolvedAllTrackFxEnableTarget, UnresolvedAnyOnTarget,
@@ -169,8 +169,7 @@ pub enum TargetCommand {
     SetMappingSnapshotDefaultValue(Option<AbsoluteValue>),
     SetPotFilterItemKind(PotFilterItemKind),
     SetLearnableFeature(LearnableMappingFeature),
-    SetInstanceId(Option<InstanceId>),
-    SetMappingId(Option<MappingId>),
+    SetMappingRef(MappingRefModel),
 }
 
 #[derive(Eq, PartialEq)]
@@ -269,8 +268,7 @@ pub enum TargetProp {
     MappingSnapshotDefaultValue,
     PotFilterItemKind,
     LearnableFeature,
-    InstanceId,
-    MappingId,
+    MappingRef,
 }
 
 impl GetProcessingRelevance for TargetProp {
@@ -649,13 +647,9 @@ impl<'a> Change<'a> for TargetModel {
                 self.learnable_feature = f;
                 One(P::LearnableFeature)
             }
-            C::SetInstanceId(id) => {
-                self.instance_id = id;
-                One(P::InstanceId)
-            }
-            C::SetMappingId(id) => {
-                self.mapping_id = id;
-                One(P::MappingId)
+            C::SetMappingRef(mapping_ref) => {
+                self.mapping_ref = mapping_ref;
+                One(P::MappingRef)
             }
         };
         Some(affected)
@@ -790,12 +784,56 @@ pub struct TargetModel {
     group_id: GroupId,
     active_mappings_only: bool,
     learnable_feature: LearnableMappingFeature,
-    instance_id: Option<InstanceId>,
-    mapping_id: Option<MappingId>,
+    mapping_ref: MappingRefModel,
     // # For Pot targets
     pot_filter_item_kind: PotFilterItemKind,
 }
 
+#[derive(Clone, Debug)]
+pub enum MappingRefModel {
+    OwnMapping {
+        mapping_id: Option<MappingId>,
+    },
+    ForeignMapping {
+        session_id: String,
+        mapping_key: Option<MappingKey>,
+    },
+}
+
+impl MappingRefModel {
+    pub fn session_id(&self) -> Option<&str> {
+        if let Self::ForeignMapping { session_id, .. } = self {
+            Some(session_id)
+        } else {
+            None
+        }
+    }
+}
+
+impl Default for MappingRefModel {
+    fn default() -> Self {
+        Self::OwnMapping { mapping_id: None }
+    }
+}
+
+impl MappingRefModel {
+    fn create_mapping_ref(&self) -> Result<MappingRef, &'static str> {
+        let mapping_ref = match self {
+            MappingRefModel::OwnMapping { mapping_id } => MappingRef::OwnMapping {
+                mapping_id: (*mapping_id).ok_or("mapping not specified")?,
+            },
+            MappingRefModel::ForeignMapping {
+                session_id,
+                mapping_key,
+            } => MappingRef::ForeignMapping {
+                session_id: session_id.clone(),
+                mapping_key: mapping_key.clone().ok_or("mapping_not_specified")?,
+            },
+        };
+        Ok(mapping_ref)
+    }
+}
+
 impl Default for TargetModel {
     fn default() -> Self {
         Self {
@@ -894,8 +932,7 @@ impl Default for TargetModel {
             browse_tracks_mode: Default::default(),
             pot_filter_item_kind: Default::default(),
             learnable_feature: Default::default(),
-            instance_id: None,
-            mapping_id: None,
+            mapping_ref: Default::default(),
         }
     }
 }
@@ -2466,10 +2503,7 @@ impl TargetModel {
                         UnresolvedReaperTarget::LearnMapping(UnresolvedLearnMappingTarget {
                             compartment,
                             feature: self.learnable_feature,
-                            instance_id: self.instance_id,
-                            mapping_id: self
-                                .mapping_id
-                                .ok_or("mapping to be learned not specified")?,
+                            mapping_ref: self.mapping_ref.create_mapping_ref()?,
                         })
                     }
                     EnableInstances => {
@@ -2614,12 +2648,8 @@ impl TargetModel {
         self.learnable_feature
     }
 
-    pub fn instance_id(&self) -> Option<InstanceId> {
-        self.instance_id
-    }
-
-    pub fn mapping_id(&self) -> Option<MappingId> {
-        self.mapping_id
+    pub fn mapping_ref(&self) -> &MappingRefModel {
+        &self.mapping_ref
     }
 
     pub fn set_mouse_action_without_notification(&mut self, mouse_action: MouseAction) {

main/src/domain/realearn_target.rs

@@ -1,4 +1,4 @@
-use crate::application::{SharedSession, WeakSession};
+use crate::application::SharedSession;
 use crate::base::{SenderToNormalThread, SenderToRealTimeThread};
 use crate::domain::ui_util::{
     format_as_percentage_without_unit, format_raw_midi, log_output,
@@ -355,7 +355,8 @@ pub fn get_track_color(t: &Track) -> Option<RgbColor> {
 }
 
 pub trait InstanceContainer: Debug {
-    fn find_session_by_instance_id(&self, instance_id: InstanceId) -> Option<WeakSession>;
+    fn find_session_by_id(&self, session_id: &str) -> Option<SharedSession>;
+    fn find_session_by_instance_id(&self, instance_id: InstanceId) -> Option<SharedSession>;
     /// Returns activated tags if they don't correspond to the tags in the args.
     fn enable_instances(&self, args: EnableInstancesArgs) -> Option<HashSet<Tag>>;
     fn change_instance_fx(&self, args: ChangeInstanceFxArgs) -> Result<(), &'static str>;

main/src/domain/targets/learn_mapping_target.rs

@@ -1,34 +1,52 @@
 use crate::domain::{
     format_value_as_on_off, Compartment, CompoundChangeEvent, ControlContext, DomainEvent,
     DomainEventHandler, ExtendedProcessorContext, HitInstruction, HitInstructionContext,
-    HitInstructionResponse, HitResponse, InstanceId, MappingControlContext,
-    MappingEnabledChangeRequestedEvent, MappingId, MappingLearnRequestedEvent, RealearnTarget,
-    ReaperTarget, ReaperTargetType, TagScope, TargetCharacter, TargetTypeDef,
-    UnresolvedReaperTargetDef, DEFAULT_TARGET,
+    HitInstructionResponse, HitResponse, InstanceId, MappingControlContext, MappingId, MappingKey,
+    MappingLearnRequestedEvent, RealearnTarget, ReaperTarget, ReaperTargetType, TargetCharacter,
+    TargetTypeDef, UnresolvedReaperTargetDef, DEFAULT_TARGET,
 };
 use helgoboss_learn::{AbsoluteValue, ControlType, ControlValue, Target, UnitValue};
 use realearn_api::persistence::LearnableMappingFeature;
 use std::borrow::Cow;
+use std::rc::Rc;
 
 #[derive(Debug)]
 pub struct UnresolvedLearnMappingTarget {
     pub compartment: Compartment,
     pub feature: LearnableMappingFeature,
-    pub instance_id: Option<InstanceId>,
-    pub mapping_id: MappingId,
+    pub mapping_ref: MappingRef,
+}
+
+#[derive(Clone, Eq, PartialEq, Debug)]
+pub enum MappingRef {
+    OwnMapping {
+        mapping_id: MappingId,
+    },
+    ForeignMapping {
+        // We can't use instance ID here because at the time when an unresolved target is created,
+        // the other instance might not exist yet. We need to resolve the other instance at a later
+        // point in time. Either at target resolve time (which would be in line with our existing
+        // stuff) or at control time (which would be simpler and not a performance issue because
+        // learning a mapping is not something done super frequently). Let's do the latter: Lazy
+        // resolve at control time. Otherwise we would need to refresh all ReaLearn instances
+        // whenever an instance was loaded. No important performance gain in this case,
+        // not worth the added complexity.
+        session_id: String,
+        // For the same reason, we can't use mapping ID here.
+        mapping_key: MappingKey,
+    },
 }
 
 impl UnresolvedReaperTargetDef for UnresolvedLearnMappingTarget {
     fn resolve(
         &self,
-        _: ExtendedProcessorContext,
+        context: ExtendedProcessorContext,
         _: Compartment,
     ) -> Result<Vec<ReaperTarget>, &'static str> {
         Ok(vec![ReaperTarget::LearnMapping(LearnMappingTarget {
             compartment: self.compartment,
             feature: self.feature,
-            instance_id: self.instance_id,
-            mapping_id: self.mapping_id,
+            mapping_ref: self.mapping_ref.clone(),
         })])
     }
 }
@@ -39,8 +57,7 @@ pub struct LearnMappingTarget {
     /// lead to strange behavior.
     pub compartment: Compartment,
     pub feature: LearnableMappingFeature,
-    pub instance_id: Option<InstanceId>,
-    pub mapping_id: MappingId,
+    pub mapping_ref: MappingRef,
 }
 
 impl RealearnTarget for LearnMappingTarget {
@@ -75,6 +92,7 @@ impl RealearnTarget for LearnMappingTarget {
                         .instance_container
                         .find_session_by_instance_id(instance_id)
                     {
+                        let session = Rc::downgrade(&session);
                         session.handle_event_ignoring_error(event)
                     }
                 } else {
@@ -85,12 +103,30 @@ impl RealearnTarget for LearnMappingTarget {
                 HitInstructionResponse::CausedEffect(vec![])
             }
         }
+        let (instance_id, mapping_id) = match &self.mapping_ref {
+            MappingRef::OwnMapping { mapping_id } => (None, *mapping_id),
+            MappingRef::ForeignMapping {
+                session_id,
+                mapping_key,
+            } => {
+                let session = context
+                    .control_context
+                    .instance_container
+                    .find_session_by_id(session_id)
+                    .ok_or("other ReaLearn instance not found")?;
+                let session = session.borrow();
+                let mapping_id = session
+                    .find_mapping_id_by_key(self.compartment, mapping_key)
+                    .ok_or("mapping in other ReaLearn instance not found")?;
+                (Some(*session.instance_id()), mapping_id)
+            }
+        };
         let instruction = LearnMappingInstruction {
             compartment: self.compartment,
             feature: self.feature,
-            instance_id: self.instance_id,
+            instance_id,
             on,
-            mapping_id: self.mapping_id,
+            mapping_id,
         };
         Ok(HitResponse::hit_instruction(Box::new(instruction)))
     }

main/src/infrastructure/data/target_model_data.rs

@@ -4,10 +4,10 @@ use reaper_high::{BookmarkType, Fx, Guid, Reaper};
 
 use crate::application::{
     AutomationModeOverrideType, BookmarkAnchorType, Change, FxParameterPropValues, FxPropValues,
-    FxSnapshot, MappingSnapshotTypeForLoad, MappingSnapshotTypeForTake, RealearnAutomationMode,
-    RealearnTrackArea, TargetCategory, TargetCommand, TargetModel, TargetUnit, TrackPropValues,
-    TrackRoutePropValues, TrackRouteSelectorType, VirtualControlElementType,
-    VirtualFxParameterType, VirtualFxType, VirtualTrackType,
+    FxSnapshot, MappingRefModel, MappingSnapshotTypeForLoad, MappingSnapshotTypeForTake,
+    RealearnAutomationMode, RealearnTrackArea, TargetCategory, TargetCommand, TargetModel,
+    TargetUnit, TrackPropValues, TrackRoutePropValues, TrackRouteSelectorType,
+    VirtualControlElementType, VirtualFxParameterType, VirtualFxType, VirtualTrackType,
 };
 use crate::base::default_util::{
     bool_true, deserialize_null_default, is_bool_true, is_default, is_none_or_some_default,
@@ -512,6 +512,17 @@ impl TargetModelData {
         conversion_context: &impl ModelToDataConversionContext,
     ) -> Self {
         let (track_data, track_selector_clip_column) = serialize_track(model.track());
+        let (session_id, mapping_key) = match model.mapping_ref() {
+            MappingRefModel::OwnMapping { mapping_id } => {
+                let mapping_key =
+                    mapping_id.and_then(|id| conversion_context.mapping_key_by_id(id));
+                (None, mapping_key)
+            }
+            MappingRefModel::ForeignMapping {
+                session_id,
+                mapping_key,
+            } => (Some(session_id.clone()), mapping_key.clone()),
+        };
         Self {
             category: model.category(),
             unit: model.unit(),
@@ -610,12 +621,8 @@ impl TargetModelData {
             mouse_action: model.mouse_action(),
             pot_filter_item_kind: model.pot_filter_item_kind(),
             learnable_feature: model.learnable_feature(),
-            session_id: model
-                .instance_id()
-                .and_then(|id| conversion_context.session_id_by_instance_id(id)),
-            mapping_key: model
-                .mapping_id()
-                .and_then(|id| conversion_context.mapping_key_by_id(id)),
+            session_id,
+            mapping_key,
         }
     }
 
@@ -925,16 +932,20 @@ impl TargetModelData {
         model.set_mouse_action_without_notification(self.mouse_action);
         model.change(C::SetPotFilterItemKind(self.pot_filter_item_kind));
         model.change(C::SetLearnableFeature(self.learnable_feature));
-        let instance_id = self
-            .session_id
-            .as_ref()
-            .and_then(|session_id| conversion_context.instance_id_by_session_id(session_id));
-        model.change(C::SetInstanceId(instance_id));
-        let mapping_id = self
-            .mapping_key
-            .as_ref()
-            .and_then(|key| conversion_context.mapping_id_by_key(key));
-        model.change(C::SetMappingId(mapping_id));
+        let mapping_ref = if let Some(session_id) = self.session_id.as_ref() {
+            MappingRefModel::ForeignMapping {
+                session_id: session_id.clone(),
+                mapping_key: self.mapping_key.clone(),
+            }
+        } else {
+            MappingRefModel::OwnMapping {
+                mapping_id: self
+                    .mapping_key
+                    .as_ref()
+                    .and_then(|key| conversion_context.mapping_id_by_key(key)),
+            }
+        };
+        model.change(C::SetMappingRef(mapping_ref));
         Ok(())
     }
 }