blish-hud / Blish-HUD

A Guild Wars 2 overlay with extreme extensibility through compiled modules.
https://blishhud.com
MIT License
311 stars 60 forks source link

Load modules sorted by dependencies #923

Open Tharylia opened 9 months ago

Tharylia commented 9 months ago

This PR implements module loading sorted by dependencies. This is needed for contexts to work properly if the context class is contained inside the module.

Discussion Reference

All new features must be discussed prior to code review. This is to ensure that the implementation aligns with other design considerations. Please link to the Discord discussion:

https://discord.com/channels/531175899588984842/599270434642460753/1160999293361061979

Is this a breaking change?

Breaking changes require additional review prior to merging. If you answer yes, please explain what breaking changes have been made.

No

dlamkins commented 9 months ago

I did a first-pass lookover.

Other things that need answering:

  1. What is the expected behavior when a module is allowed to start prior to GW2? How does this affect the load order?
  2. What happens when a user manually disables a dependency?
  3. What UI feedback is provided to the user to explain a module not enabled because of a dependency issue? Is it just the dependency section?

Regarding your use case, it's still unclear to me why the dependent module has a different implementation of an assembly being loaded, making the load order important. Can you provide a minimal code example that exhibits this issue so that I can see it?

  1. Load order doesn't explicitly ensure that a module's assemblies are loaded prior to another in the event that the module doesn't attempt to use an assembly first.
Tharylia commented 9 months ago

I did a first-pass lookover.

Other things that need answering:

  1. What is the expected behavior when a module is allowed to start prior to GW2? How does this affect the load order?
  2. What happens when a user manually disables a dependency?
  3. What UI feedback is provided to the user to explain a module not enabled because of a dependency issue? Is it just the dependency section?
  1. Nothing, the same as prior to this PR. The dependent module needs to check for this case. In case of contexts: The state of Expired.
  2. No change. It is still the same as prior to the PR. Its only visible in the dependency section. I considered ScreenNotifications but I'm not sure if we want to potentially spam users.
Tharylia commented 9 months ago

BlishHUD Module Load Order Test.zip

ModuleB registers a context and ModuleA is dependent on B as it wants to use it.

Steps:

sonarcloud[bot] commented 9 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication