Split EDDI project into EddiCore and EddiApp

[richardbuckle]

I propose to split the "EDDI" project into:

1. "EddiCore", containing the stuff upon which the monitors and services depend, and
2. "EddiApp", being purely the UI that runs up standalone.

Rationale:

1. Banishes the "F5 doesn't update everything problem".
   This re-jig allows "EddiApp" to depend upon all code-generating projects, so that having "EddiApp" as the startup project means that good old F5 once again works to build everything necessary before debugging.
2. It allows for adding shared resources to "EddiCore" that all UI-displaying projects can access.
3. It allows for XAML resource dictionaries in "EDDICore" that will let me push down shared stuff like the alternating table AliceBlue into the proper place.

Code impact:

• Minimal upon dependent code, if done with zero change. Just change using EDDI to using EDDICore.
• Project file dependencies will definitely change.
• We may need to decouple EddiVoiceAttackResponder's dependencies on EddiApp via declaring an interface and doing dependency injection.

[richardbuckle]

[GioVAX]

I started splitting the DLLs.
Initial findings to collect the team thoughts on possible approaches:

• CheckUpgrade() and StartUpgrade() are inside the EDDI class. These should be moved back into the main application, possibly in a separate class, as suggested by Richard.

• when some specific events are handled (e.g. squadron status and updates) Eddi updates some configuration tab in the app UI, going against the dependency direction.
  One possible approach would be to implement in the app a responder interface to handle these events and update the UI.

Probably not the right moment, but this whole work would be a no brainer if we had a Dependency Injection framework in place.

[Tkael]

Fair points.

• I have no problem with putting CheckUpgrade() and StartUpgrade() into a separate class callable from app.xaml.cs.
• Hmm. I agree that we should probably be handling these through the responder or monitor interfaces but we'll need to think about which is most appropriate here. Typically, we would use a monitor to manage global state rather than a responder because monitors handle events and set global states before responders react to them. You should also be aware that if the EDDI version is too old that it will enter a "safe mode" where it loads neither monitors nor responders.

[GioVAX]

About the monitor vs responder for updating the UI, are you implying that other parts of the application do read the state from the (e.g.) squadron tab of the app UI?

My assumption was that some monitor (journal?) would get the original event, hand it to the EDDI object, and then all the responders would get notified and do whatever they need to do.
For the new app UI responder, that would be doing the update.

I also assumed that any other part of the app would get any necessary state from the EDDI object, not from the app UI.

[Tkael]

No. Global state is as you see it in EDDI.cs. It's not tangled up in the front end UI thank goodness.

I'm referring to the OnEvent() method in EDDI.cs:

        private async void OnEvent(Event @event)
        {
            // We send the event to all monitors to ensure that their info is up-to-date
            // All changes to state must be handled here, so this must be synchronous
            passToMonitorPreHandlers(@event);

            // Now we pass the data to the responders to process asynchronously, waiting for all to complete
            // Responders must not change global states.
            await passToRespondersAsync(@event);

            // We also pass the event to all active monitors in case they have asynchronous follow-on work, waiting for all to complete
            await passToMonitorPostHandlersAsync(@event);
        }

In the case of squadrons, there is a global state variable in EDDI.cs called "SquadronStarSystem". I think it would be appropriate to update that variable with a PreHandler from a Monitor, before the value is passed to responders like the Speech Responder or the VoiceAttack Responder.

[GioVAX]

Apologies I wasn't clear :)
I was not suggesting nor stating that the state is managed in the UI.

I was referring to code like the following (line 2241-> 2249 from EDDI.cs)

// Update the squadron UI data
Application.Current?.Dispatcher?.Invoke(() =>
{
       if (Application.Current?.MainWindow != null)
      {
             ((MainWindow)Application.Current.MainWindow).eddiSquadronNameText.Text = theEvent.name;
             ((MainWindow)Application.Current.MainWindow).squadronRankDropDown.SelectedItem = rank.localizedName;
      }
});

The code above is sandwiched between other instructions that do update the EDDI state, while this appears to me to be just updating the UI and could be handled by having a responder in the MainWindow code.

I am aware I have very little knowledge of the codebase, so I'd appreciate if you could point out what I am missing 😃

[Tkael]

Agreed, the highlighted code ought to be moved.

The section immediately below the section you highlighted

            // Update the commander object, if it exists
            if (Cmdr != null)
            {
                Cmdr.squadronname = theEvent.name;
                Cmdr.squadronrank = rank;
            }

updates a global state.

Since updating global states needs to happen before responders act on those global states, I think that it would be appropriate to consider a "Commander Monitor", with the UI for the commander tab being updated from that monitor, rather than a "Commander Responder".

[Tkael]

It occurs to me that there may be a simpler approach. We could set up two way bindings between the UI elements you identified and the Cmdr object, making provisions to update the configuration file when the properties of the Cmdr object have changed. Thoughts?

[GioVAX]

WRT the Monitor/Responder dichotomy, my understanding was that monitors wait on external events (e.g. new journal entries) and create internal events, while responders act on internal events and perform application actions.
But based on your comments I am starting to think that I got this wrong 😄
Can you shed some more light on this, please?

About the 2-way bindings, is it implemented by the following functions (from CargoMonitor)?

public void EnableConfigBinding(MainWindow configWindow)
{
    configWindow.Dispatcher.Invoke(() => { BindingOperations.EnableCollectionSynchronization(inventory, inventoryLock); });
}

public void DisableConfigBinding(MainWindow configWindow)
{
     configWindow.Dispatcher.Invoke(() => { BindingOperations.DisableCollectionSynchronization(inventory); });
}

[Tkael]

Yes, BindingOperations.EnableCollectionSynchronization is part of how we enable two way binding for the observable collection inventory in the Cargo Monitor.
https://docs.microsoft.com/en-us/dotnet/framework/wpf/data/how-to-create-and-bind-to-an-observablecollection
Please also note that some bound items in EddiCargoMonitor/ConfigurationWindow.xaml have the mode set to "TwoWay" to allow editing either in the front end or in the back end.
https://docs.microsoft.com/en-us/dotnet/framework/wpf/data/how-to-specify-the-direction-of-the-binding

As for Monitors and Responders... your understanding is generally correct... new data typically comes in through either a monitor rather than through a responder. Many monitors, however, also play a critical role in managing state data (e.g. the Cargo monitor, the Shipyard monitor, the Material monitor).

Both monitors and responders act on events and the timing of those actions is important. Monitors act first to pre-handle events and set states. Then responders then act to handle events referencing those global states. Finally, monitors post-handle the events as required.

Example:

• We collect some new material. The journal monitor picks this up, generates an event, and passes that event to EDDI.cs.
• EDDI.cs invokes each monitor and responder via the appropriate interfaces, giving each an opportunity to act on the event.
• The Material monitor acts first. It pre-handles the event by updating our material inventory (which is two way bound to the UI via an observable collection).
• Now that the material inventory is updated, the Speech Responder and VoiceAttack responder are free handle the event.
  • This triggers our scripts and VoiceAttack commands via the Speech responder and VoiceAttack responder.
  • The material inventory, if queried during any of these triggered responder actions, will return values which were already updated by the event by the Material monitor's pre-handler.
• The Material monitor then has the opportunity to act again and post-handle the event (though a post-handle action would not be needed here).

[GioVAX]

So, if I get this correctly, Monitors and Responders are quite a different concept from Producers and Consumers of events.

They actually represent stages of event processing: Monitor:PreHandle --> Responder:Handle --> Monitor:PostHandle.
In addition to this, Responders:Handle should not update the state so that they can be executed in parallel.

[Tkael]

Correct. A monitor can be a producer but isn't necessarily so.

[richardbuckle]

Erroneously closed.

[barrywimlett]

Certainly my first impression with this codebase is that two large libraries 'Utilities' and 'DataDefinitions' have a whole host of sub dependencies/requirements such as RollBar UI and Mathlib. Breaking these large libraries into smaller chunks would then mean that consuming code projects are not encumbered by the restrictions of those 3rd part externals. ie. anything that wants DataDefintions needs MathLib when only one class 'StarClass' uses that library.

My experience is that libraries with vauge names involving words like core/common/util often contain a hodge-podge of functionaility often unrelated to each other.

Also separating the XAML code from functional code is a good idea.

[richardbuckle]

'Utilities' and 'DataDefinitions' are stable, well-defined and have low churn. We are happy with the structure there and that they represent a sensible separation of concerns for our use case.

Everything is going to depend on RollBar because it is our crash and logging telemetry.

Everything is going to depend on MathLib because, well, we need to do math.

Thank you for the input but we are not going to restructure a well-working project based on purely doctrinal principles.