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.

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

EDDI split #1996

Closed
wants to merge 15 commits into from
Closed

EDDI split #1996

wants to merge 15 commits into from

Conversation

GioVAX
Copy link
Contributor

@GioVAX GioVAX commented Nov 10, 2020

Final change to move EDDI to a separate dll
Resolves #1781.

@Tkael
Copy link
Member

Tkael commented Nov 11, 2020

For reference, here's the original proposal and the architecture at that time:

I propose to split the "EDDI" project into:

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

Rationale:

 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.
 It allows for adding shared resources to "EddiCore" that all UI-displaying projects can access.
 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.

Original Architecture

Here's the architecture under this PR:

Architecture view for EDDI - PR 1996

And here's the architecture under this PR, after cleaning up unused compile-time dependencies:

Architecture view for EDDI - PR 1996 - cleaned dependencies

EddiCore/EDDI.cs Outdated Show resolved Hide resolved
@Tkael
Copy link
Member

Tkael commented Nov 11, 2020

First pass thoughts:

  1. We don't seem to have achieved our goal yet of placing Eddi at the base of the dependency stack?
  2. By reacting to squadron events in a responder rather than a monitor pre-handler, there is no guarantee that squadron state information will be up to date when queried from the Speech responder or from VoiceAttack (both of which will be acting concurrent with the MainWindowResponder). This isn't necessarily a problem as long as the responder has no responsibilities other than passing data to the UI but I'm not sure that a responder is the best interface here. Thoughts welcome.
  3. We seem to be handling some squadron events twice with this PR, once in EDDI.cs to write the configuration and a second time in MainWindowResponder.cs to update the UI. We ought to consider instead binding directly to objects holding the appropriate information (such as the Cmdr object currently in EddiCore.cs) so that when the object changes in the view model the model is automatically updated. Perhaps rather than a Main window responder, we should have a Commander monitor responsible for updating the state of the Cmdr object and (via binding) also the Commander tab in the UI?
  4. The classes MarketInfoReader, OutfittingInfoReader, and ShipyardInfoReader in EddiCore are obsolete and no longer used. They have been replaced by the MarketInfo, OutfittingInfo, and ShipyardInfo classes.
  5. This last one is a very minor nitpick, but using statements need sorting.

@richardbuckle
Copy link
Member

Looks like we are lot of the way there, but haven't yet renamed Eddi to EddiApp and hoisted it to the top of the tree. Would that be an accurate summary?

@GioVAX
Copy link
Contributor Author

GioVAX commented Nov 14, 2020

Answering Tkael points:

  1. Not yet. To be honest, I didn't realise the goal was to move the main app to the bottom of the architecture diagram. :)
    In any case, moving EddiCore out of the main app is the first step in that direction.
    Now we can remove references to EddiApp from the remaining dlls until we achieve the desired outcome.
    It's just a long and time-consuming process and I just want to have EddiCore merged into develop.

  2. I'm not sure I follow this. The main handling of the squadron events happens in EddiCore even before going to the monitors pre-handlers. Changes to the configuration file are already written to disk, and, unless I missed something, the MainWindowResponder is only updating the UI. Unless Speech and VoiceAttack do retrieve the data from the UI, this should not be a problem.

  3. EddiCore is pre-handling a lot of events before sending them to the monitors and responders. I agree that the binding with the UI might definitely be improved, but to prevent tight coupling between monitors/responders and the UI I think we would need to introduce quite a bit of infrastructure. This could be another long term project.
    I really think that updating UI can be achieved using a responder: IMHO UI is a terminal leaf in the graph of updates propagation.

  4. I'll fix those. I believe I involuntarily re-introduced them during a merge. All more reason to speed up this merge ;)

  5. Will fix these too. Do we use full alphabetical order, or do we keep (e.g.) System-based namespaces at the top?

@richardbuckle
Copy link
Member

I recommend using the Code Cleanup command to prune and sort using statements alphabetically. These are my two setups:

Code cleanup 1

Code cleanup 2

@Tkael Tkael marked this pull request as draft February 4, 2021 08:37
@Tkael Tkael force-pushed the develop branch 2 times, most recently from 73e181b to 01d5afd Compare April 11, 2021 00:28
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split EDDI project into EddiCore and EddiApp
3 participants