[Rick-van-Dam]

Changes:

• Added a new way to save editoruserdata using duality serializers. Just serialize and deserialize models instead of XElements
• Old system still works, it gets stored in a string field in PluginSettings. Default settings have moved to their respective projects though with the use of EditorPlugin.GetDefaultUserData.
• Dockpanel xml is stored in a separate string field on the DualityEditorUserData class

Editorplugins will be updated in separate PR's

[ilexp]

Editorplugins will be updated in separate PR's

Related PRs for reference: #866, #867, #868, #869, #870, #871, #872

[ilexp]

Looks good overall 👍 Some change requests, see inline comments.

[ilexp]

Done with most of my own change requests and also restored the old settings behavior / wired up editor internals to the new settings container class.

There's one more refactoring I'd like to explore: Due to the non-trivial nature of saving editor data which is composed of the main container, all the plugin data objects and a serialized dock panel blob, I'd like to re-introduce the SaveUserData / LoadUserData method pair, and add a similar one for AppData, while at the same time having the public properties return the instance directly. This would remove the need to always write .Instance as well. The advantage of having a shared interface for settings containers to allow unified saving still remains, it's just used internally.

I'll experiment with this locally to see if the result is a net improvement or not, will report back.

[ilexp]

I'll experiment with this locally to see if the result is a net improvement or not, will report back.

Okay, so I played around with this a bit and don't think this is a great way to go about this. Going forward, the SettingsContainer should theoretically be able to handle all the load / save code, without the need for additional calls:

• The EditorPluginManager.Load/Save call can probably be removed once all plugins use the new serializer API, since there wouldn't be anything left to do in there after that. All the plugins would access their settings object directly, no more field assignment needed. Any post-load code can still be executed by subscribing to the Changed event.
• For Pre-save code, we could add a Saving event, but so far it's only the main form dock panel that would make use of this. And that specific case can still be done manually where needed, without much harm done. So we might not even need this event and could keep the SettingsContainer API a bit simpler that way.

Now that I've worked on this a bit, I think it might make sense to include the content of the plugin PRs (i.e. the whole port of the plugin settings serialization) in this one and merge it all in one big step. Could be less noise and intermediate states on the master branch in the end.

[ilexp]

Looked into some of the cases of per-plugin user settings we'd need to port and found a tricky one in the CamView plugin:

• The plugin itself doesn't have settings (but could in the future)
• It stores settings for any number of CamViews
• Each CamView stores its own settings, and the settings of any number of CamViewStates and CamViewLayers
• Each CamViewState or CamViewLayer can have any number of subclasses, which in turn can be defined in additional plugins, each with their own settings

This would mean a really big number of various settings classes. Could mean a bit of boilerplate code - but then again, we get well-documented properties and a proper definition in return. Could be worth starting with the CamView port as a test case so we can be sure it won't get much worse after that.

[ilexp]

Going forward, the SettingsContainer should theoretically be able to handle all the load / save code, without the need for additional calls:

• The EditorPluginManager.Load/Save call can probably be removed once all plugins use the new serializer API, since there wouldn't be anything left to do in there after that. All the plugins would access their settings object directly, no more field assignment needed. Any post-load code can still be executed by subscribing to the Changed event.
• For Pre-save code, we could add a Saving event, but so far it's only the main form dock panel that would make use of this. And that specific case can still be done manually where needed, without much harm done. So we might not even need this event and could keep the SettingsContainer API a bit simpler that way.

More examples have come up where a Saving event or an actual Save call would be necessary, because their settings are stored in UI state.

It could make sense to refactor those parts as part of the port, which would mean that the call would no longer be necessary - but in that case, every UI change would directly edit the settings object, theoretically requiring to call its Changed event all the time. This would create a load of unnecessary work throughout the editor though and not be what this event was intended to do.

To fix this, I'd propose to rename Changed to Loaded and also add a Saving event for those cases that need it, as a mirror image. This would allow us to get rid of any additional call requirements beyond the container Load / Save to always serialize and apply the full thing, including the dock panel. It would also bring the API closer to what's actually happening, because Changed was never fired for every change anyway, or intended to do that. Loaded would match its behavior exactly.

We could get rid of the whole EditorPluginManager load / save calls entirely, and any plugin that really wants to go a different route for settings storage can just subscribe to Saving / Loaded and transform their data on-demand.

[ilexp]

One more thing to address: DualityApp user and app data make use of their Changed event to apply global settings internally, but there is no longer any way to invoke the Changed event at runtime without loading new settings from file, and I just proposed earlier to get rid of this event entirely / rename it to Loaded.

I'd honestly take a shortcut for this fix for now to keep workload low and wrap up this PR soon: Let's just add an Apply method to the SettingsContainer that calls Save and then Load immediately after internally. For runtime settings changes in a Duality game, like in an options menu, this should do exactly what people would expect.

Edit: Maybe a better naming for those events would be Saving and Applying, since the latter will be invoked both after load and when calling Apply?

[ilexp]

Progress

One more thing to address: DualityApp user and app data make use of their Changed event to apply global settings internally, but there is no longer any way to invoke the Changed event at runtime without loading new settings from file, and I just proposed earlier to get rid of this event entirely / rename it to Loaded.

I'd honestly take a shortcut for this fix for now to keep workload low and wrap up this PR soon: Let's just add an Apply method to the SettingsContainer that calls Save and then Load immediately after internally. For runtime settings changes in a Duality game, like in an options menu, this should do exactly what people would expect.

Edit: Maybe a better naming for those events would be Saving and Applying, since the latter will be invoked both after load and when calling Apply?

Done.

ToDo

• Port all editor plugin settings to the new serializer based system.
  • Adjust settings object defaults to match the default settings that were previously applied via embedded EditorUserData.xml, the legacy one.
• Consider getting rid of all explicit EditorPluginManager load / save calls and rely instead on the new Applied / Saving events.
• Do some post-port testing, see if the settings properly end up in the file and are loaded properly.

[ilexp]

I'm fine with a (squash-)merge based on the current state as long as the remaining ToDos are categorized in a new issue or PR when merging, I think the current state is solid enough to stand on its own, though individual merging would bump the priority of the followup PRs to not remain in this intermediate state for too long. Alternatively, we can do it all in this PR directly. Thoughts?

[Rick-van-Dam]

I'm fine with a (squash-)merge based on the current state as long as the remaining ToDos are categorized in a new issue or PR when merging, I think the current state is solid enough to stand on its own, though individual merging would bump the priority of the followup PRs to not remain in this intermediate state for too long. Alternatively, we can do it all in this PR directly. Thoughts?

We could cherrypick/merge the commits for the other PR's into to this branch before merging this PR to master.

The main reason I splitted them up is for reviewing purposes.

[ilexp]

Added a few more commits where I removed the need for explicit "Update XY" calls by subscribing to the settings event handlers instead. It should now be a simple Load / Save call in all cases.

[ilexp]

Okay, so after letting this sink in a bit, I think it would be better to merge this as-is and continue with individual PRs for porting the existing user data:

• This PR is self-contained enough to not introduce functional regressions when merging, it's only added infrastructure.
• This PR introduces codepaths to handle not-yet-ported implementations, so we remain backwards compatible on the code side.
• It continues work that is already on master and improves on existing implementations, including restoring a feature that was previously lost (runtime apply for game settings via code)
• It's somewhat big already and growing it further would make it harder to review and reason about.
• It's already reviewed.
• Porting the individual plugin user data has very different difficulty levels and requirements due to very different contents, so doing them one by one is a big advantage.