Proposal for the refactoring of IPlugin::isActive

[Holt59]

This is an issue to propose a global refactoring of how plugin are enabled / disabled. This is open for discussion and I'll do the tedious works of refactoring existing plugins if we agree on something.

1. The problem

Currently, a plugin is considered enabled if isActive() returns true. There a few issues with this:

• There are inconsistencies between plugins: Some plugins provide a enabled setting, others don't.
• Some plugin types seem to have non-relevant isActive(), e.g. IPluginGame::isActive returns true if the plugin corresponds to the currently managed game (for all plugins inheriting GameBryo). This is not documented, hence some issues arose on Discord.

Edit —

The global idea is to have more control and restrictions over the whole plugins "system" to prevent plugin authors from writing incorrect plugins and to allow us to more easily manage plugins, and dependencies between plugins (or between MO2 and plugins). This includes

• Avoid having meaningless isActive implementation, i.e. implementation that have to perform specific action for MO2 to work correctly.
  • This includes the game plugin which basically have to check this == organizer->managedGame(), and fixing this is not easy since the top-level interface (IPluginGame, IPluginInstaller, etc.), do not have direct access to the organizer and thus cannot check the currently managed game. Having a default init() implementation that stores the organizer would be a solution but you'd have to call it from child class and it's pretty easy to miss (and it's as much as work as this proposal).
• Have a consistent way of enabling / disabling plugins in order to control when a plugin is enabled / disabled (to warn user, to perform specific actions, etc.).
  • One example (from the comments) is the proxy plugin. Currently, a user can disable the Python proxy, and then wonder why all the Python plugins disappeared. A solution would be to warn user when they disable the proxy plugin, but currently this is not easy to do because the "enabled" setting is nothing special, so there is no guarantee the setting exist, and even if we ensure this in the proxies, we'd have to perform special check in the settings dialog.
    • Basically, there is no easy way to know if a plugin is going to be disabled unless checking if a setting has the name "enabled" (assuming the plugin author used the right name). With the proposal, it's trivial since all plugins that can be disabled would have this settings, and plugins can only prevent user from enabling them, not disabling them.
• Simplify the implementation for plugin authors. They would only need to implement canActivate if a plugin had special requirements.

2. The proposal

I propose to move part of the "active" check to the organizer instead of the plugins, in particular I propose that (almost) all plugins have a enabled settings that can be combined with plugin-provided information to enable plugins.

I propose to remove IPlugin::isActive() and replace it by IPlugin::canActivate() IPlugin::requirements, and to add IOrganizer::isPluginActive():

• IPlugin::requirements returns a list of "requirements":
  • Has a default implementation that returns nothing.
  • IPluginGame::requirements() is final, so all games can be activated.
  • For all other plugins, this basically replace isActive but provides more information:
    • We could have plugin-dependency requirements. This would allow us to warn users when they try to disable a plugin that is a requirement for another.
    • For instance, some Skyrim-specific plugin could require the Skyrim plugin.
    • All Python plugins would require the Python proxy - This can easily be done automatically in the runner implementation.
  • These requirements could replace / be combined with diagnose in some cases (e.g. the NCC installer could have a requirements for .NET).

# This:
def isActive(self) -> bool:
    return self._organizer.pluginSetting(self.name(), "enabled", True) and self._checkNccInstalled()

# Become this:
def requirements(self) -> ...:
    return Requirement(self._checkNccInstalled, "NCC is not installed.")

• IOrganizer::isPluginActive():
  • This check that the plugin is enabled and has all its IPlugin::requirements().
  • For some plugin types, it performs custom check. E.g. for IPluginGame, it checks that the managed game corresponds to the plugin.

3. Amount of work to implement

This requires changing all existing plugins, which is tedious but not that much work:

• C++ Plugins:

  • Since those are all managed by MO2 developers, we simply need to either drop the isActive implementation (if it was only returning true or checking for the enabled setting) or replace it by requirements.
  • Call to isActive should be replaced by call to IOrganizer::isPluginActive().

• Python Plugins:

  • We replace the isActive() bindings to call IOrganizer::isPluginActive() with a deprecation warning.
  • We add the requirements() bindings.
  • Basically, existing plugins that are not updated will still have isActive(), but the isActive() will do nothing. This avoid making breaking existing plugins although they will not behave exactly as before (regarding isActive()).

4. Difference with the current implementation

1. Almost all plugins can be enabled / disabled through the MO2 settings dialog.
2. Game plugins can always be activated, there is no way to disable a game plugin (none of the existing game plugins had special isActive(), and I don't see why you would disable a game plugin).
3. There is no way to prevent disabling of a plugin. This is probably the biggest "breaking" change, although I don't see the use of this. You can still prevent enabling via requirements.

[AnyOldName3]

To me, this seems like changing things for the sake of changing things rather than for any particular reason. The game plugins using isActive for two separate concerns isn't good, but this seems like throwing out the baby with the bathwater.

[Holt59]

To me, this seems like changing things for the sake of changing things rather than for any particular reason. The game plugins using isActive for two separate concerns isn't good, but this seems like throwing out the baby with the bathwater.

I wrote the two main reasons in the issue, maybe you should clarify why those are not valid or sufficient enough from your point-of-view. "It's inconsistent and it's not what you think it is" is from my point-of-view a pretty good argument.

[AnyOldName3]

I don't really see the point in plugins having an enabled setting in the first place except invasive ones that might annoy you, and that's something I think the plugin author's in a much better position to determine than we are. If a user absolutely needs to disable a plugin with no setting, that's what the blacklist is for. Making MO2 add an enabled setting for every plugin is more likely to make users accidentally deactivate parts of MO2 they're relying on than actually add any utility for anyone.

As for isActive being used for isManagedGame, there are other, less invasive options. For example, we could change the call site for everywhere that's using it for that to just check if IOrganizer::managedGame returns the same pointer as the game plugin instance, or add an isManagedGame function to the game plugin interface that does that (in which case it can be final and part of the interface).

[isanae]

So, tl;dr: 1) MO manages the enabled flag for all plugins 2) MO manages the check that a game plugin is used by the current instance 3) Other plugins can tell MO they're inactive so nothing gets called on them

Is that correct?

If so, I don't think it fully fixes Stalker Anomaly, which would need to separate its plugin class into two: one of the BasicGame, which always returns true in canActivate(), and another for IPluginFileMapper, which returns true in canActivate() only if the active game is Stalker Anomaly. Or have some way of having plugin dependencies.

[Holt59]

I don't really see the point in plugins having an enabled setting in the first place except invasive ones that might annoy you, and that's something I think the plugin author's in a much better position to determine than we are. If a user absolutely needs to disable a plugin with no setting, that's what the blacklist is for.

I don't agree on this. I consider black-listed plugins as plugins that do not work properly, and black-listing a plugin should be a last resort (breaking change in MO2 that broke the plugin, a bug in the plugin, etc.). I think users should have the choice to enable/disable plugins. That may seem insignificant now, but if we get more and more plugins, that something I think users will want.

Making MO2 add an enabled setting for every plugin is more likely to make users accidentally deactivate parts of MO2 they're relying on than actually add any utility for anyone.

Unless this can be done by mistake, I don't really see that as an issue. People can already disable the Python proxy plugin, so if this was doable "by mistake", we'd had a lot more complaints.

As for isActive being used for isManagedGame, there are other, less invasive options. For example, we could change the call site for everywhere that's using it for that to just check if IOrganizer::managedGame returns the same pointer as the game plugin instance, or add an isManagedGame function to the game plugin interface that does that (in which case it can be final and part of the interface).

You cannot have isManagedGame() in IPluginGame or you would have to implement it in all games because you don't have access to the organizer in the base class. That's a change almost as big as what I'm proposing, and that would be complicated because then you would have isActive and this check, so where do you check what? Like do you check isActive() in the organizer proxy to disable callbacks? But also if it's the current game, but only if it's a game plugin? It basically forces every game plugin to implement a function that always does the same thing.

[Holt59]

So, tl;dr:

1. MO manages the enabled flag for all plugins
2. MO manages the check that a game plugin is used by the current instance
3. Other plugins can tell MO they're inactive so nothing gets called on them

Is that correct?

Yes, basically. Plugins can tell MO they should not be activated (canActivate) rather than they're inactive (e.g. the NCC installer could not be activated if the requried .NET dependencies or whatever is not present).

If so, I don't think it fully fixes Stalker Anomaly, which would need to separate its plugin class into two: one of the BasicGame, which always returns true in canActivate(), and another for IPluginFileMapper, which returns true in canActivate() only if the active game is Stalker Anomaly. Or have some way of having plugin dependencies.

You can have some kind of plugin dependency using the master function but I don't think that's required here. File mapper cannot be standalone plugins (well, except in Python... ), so when you would check for isPluginActive() in the organizer, even for the file mapper, it would check for the underlying game plugin, which would work as expected.

Basically, if a plugin is a game plugin and something else, it's only active for the currently managed game (all the implemented interfaces).

[isanae]

File mapper cannot be standalone plugins (well, except in Python... ), so when you would check for isPluginActive() in the organizer, even for the file mapper, it would check for the underlying game plugin, which would work as expected.

File mappers are just an IPlugin and IPluginFileMapper, they're not necessarily associated with a game.

But basic_games seems to put some interfaces togethers, like StalkerAnomalyGame, which is an IPlugin, BasicGame and IPluginFileMapper, but there's also a separate StalkerAnomalyModDataChecker, so I'm not exactly sure how that works. However, since the game plugin and mapper are in the same class, you're relying on the fact that some check somewhere is going figure out that the mapper is also a game.

For the general case, I think we still need to give plugins an easy way of figuring out what game plugin is currently in use, so plugins can use that as a basic dependency check.

[Holt59]

File mappers are just an IPlugin and IPluginFileMapper, they're not necessarily associated with a game.

Yes I know I was referring to this particular case.

But basic_games seems to put some interfaces togethers, like StalkerAnomalyGame, which is an IPlugin, BasicGame and IPluginFileMapper [...]

BasicGame is an intermediate class between IPluginGame and the actual game. There is no way for basic games to separate plugins, you have to implement all the interfaces in a single class, as it's done for Stalker Anomaly. That's specific to basic games.

[...] but there's also a separate StalkerAnomalyModDataChecker, so I'm not exactly sure how that works. [...]

This is a game feature so not related to the isActive stuff.

[...] you're relying on the fact that some check somewhere is going figure out that the mapper is also a game.

Yes, basically if a plugin implements the IPluginGame interface, we perform a custom check by "comparing it" to the currently managed game.

For the general case, I think we still need to give plugins an easy way of figuring out what game plugin is currently in use, so plugins can use that as a basic dependency check.

Agree, IOrganizer.managedGame() would still be here but you would also be able to check by doing organizer.isPluginActive("game") (where "game" is the plugin name).

[AnyOldName3]

Making MO2 add an enabled setting for every plugin is more likely to make users accidentally deactivate parts of MO2 they're relying on than actually add any utility for anyone.

Unless this can be done by mistake, I don't really see that as an issue. People can already disable the Python proxy plugin, so if this was doable "by mistake", we'd had a lot more complaints.

Most of the complaints I deal with for the OpenMW Export plugin are people who've inadvertently disabled the Python proxy plugin. It's already happening.

[Holt59]

Most of the complaints I deal with for the OpenMW Export plugin are people who've inadvertently disabled the Python proxy plugin. It's already happening.

Well, I would say that

1. People inadvertently disable the Python proxy because "enabled" is just another plugin setting. If we move this to MO2 core, we can handle it more carefully, e.g. warn users if they try to disable the proxy while having other plugins active, etc. Yes this can be done in the current state of MO2 but would much more annoying.
2. If people manage to inadvertently disable the Python proxy, I'd say they can inadvertently do anything, so...

[AnyOldName3]

If people manage to inadvertently disable the Python proxy, I'd say they can inadvertently do anything, so...

So we shouldn't enlarge the attack surface.

[Holt59]

If people manage to inadvertently disable the Python proxy, I'd say they can inadvertently do anything, so...

So we shouldn't enlarge the attack surface.

Funny thing, if you'd read my full comment instead of complaining by reading the last line of it you would have noticed that we could reduce the attack surface.

[AnyOldName3]

It's not really a 2., then, it's another sentence in 1..

I'm not trying to argue for the sake of making things hard, just to double-check that everyone's thought this through (including me).

As it's described in the initial post, the first part of the problem still doesn't really seem like an actual problem to me, and the second part seemed to have simpler solutions. The discussion afterwards has made it clearer why my simpler proposals wouldn't work as well as I first thought, so this has been a productive debate, and I agree that having MO2 yell at people for disabling plugins they probably don't want to disable is better than what we have now. That's not part of the problem or solution as described in the initial post, though, and only showed up in the ensuing debate.

[isanae]

Alright, I searched for isActive() everywhere in plugins to see what they use:

• Always true:

  • BasicGame
  • diagnose_basic
  • installer_bain
  • installer_bundle
  • installer_manual
  • installer_ncc
  • preview_dds
  • tool_inibakery
  • tool_inieditor

• Enabled flag:

  • bsapacker:
  • bsa_extractor
  • installer_fomod
  • installer_fomod_csharp
  • installer_ncc
  • tool_configurator

• Game plugin is this:

  • game_enderal
  • game_fallout4
  • game_fallout4vr
  • game_falloutnv
  • game_morrowind
  • game_oblivion
  • game_skyrim
  • game_skyrimse
  • game_skyrimvr
  • game_falloutttw

• Other:

  • check_fnis: managed game name is Skyrim
  • FNISPatches, FNISTool and FNISToolReset: game is one of skyrim/skyrimse/skyrimvr + enabled flag
  • form43_checker: game is skyrimse + enabled flag
  • plugin_python: load didn't fail
  • script_extended_plugin_checker: game is supported + enabled flag

Then I had a look at where isActive() is currently called in MO:

• IPluginInstaller
  • InstallationManager: install(), notifyInstallationStart(), notifyInstallationEnd()
• IPluginDiagnose
  • MainWindow: checkForProblemsImpl()
  • ProblemsDialog, runDiagnosis()
• IPluginTool
  • MainWindow: registerPluginTools()
• IPluginFileMapper
  • OrganizerCore: fileMapping
• IPluginPreview:
  • PreviewGenerator: previewSupported(), genPreview()

There's also callIfPluginActive(), which is used when registering various callbacks from plugins so these callbacks are only executed if the plugin is active.

Finally, here's where they're used within the plugins themselves:

• Bypasses init() when false:
  • preview_base
  • preview_bsa

Note that MO itself doesn't use isActive() for either loading a plugin or initializing it, although some plugins refuse to initialize when it's false (but still return true). It's only used in some places before using a plugin, and it's not consistent (it's never called for IPluginModPage, for example).

[Holt59]

Thanks, really nice!

* Always `true`:
* Enabled flag:
* Game plugin is `this`:

These would simply have to be removed.

For other plugins, this could be moved to canActivate. Also, when thinking about it, canActivate should be renamed requirements and we should have some type of requirements (e.g. PluginDependency) so that we could warn user if they try to disable a plugin that is a requirement for another, etc. For instance, Skyrim would be a requirement of check_fnis and all python plugins would have the proxy for requirements. We would have a generic Requirement that plugin could return and that we could use to tell users why the plugin cannot be activated. This could replace (or be combined) with IPluginDiagnose in some cases (e.g. for the NCC installer).

Then I had a look at where isActive() is currently called in MO: There's also callIfPluginActive(), which is used when registering various callbacks from plugins so these callbacks are only executed if the plugin is active.

All of these can be changed to use the IOrganizer::isPluginActivate I think.

Finally, here's where they're used within the plugins themselves:

* Bypasses `init()` when `false`:

I'd have to look at the actual init(), but this could probably be removed? Unless some complex stuff is done in the init().

Note that MO itself doesn't use isActive() for either loading a plugin or initializing it, although some plugins refuse to initialize when it's false (but still return true). It's only used in some places before using a plugin, and it's not consistent (it's never called for IPluginModPage, for example).

I try to fix some of these in a PR, but I probably missed some.

[isanae]

To me, this sounds like a complicated project that I'm not interested in doing. Feel free to work on it. Things that bug me:

• All plugins should have a built in enabled flag, managed by MO. Should be a checkbox in the plugins settings instead of a non localized enabled flag.
• isActive() is a crappy name
• MO needs consistency in how it uses isActive() or whatever it's going to be renamed to

For now, all I need to release a devbuild is a managed game check in BasicGame so Stalker stops injecting mappings. @Holt59, do you have time to do this today?