Feature Introduce temporary preferences for scripting

[Irrat]

Scripts often modify preferences to control how KoLmafia behaves. Sometimes this is because the user's existing preferences are unsuitable, and sometimes because the script needs different behavior temporarily (eg, autoscend or garbo)

Normally this works, but many of these changes are not meant to be permanent. They only need to apply while the script is running.

If a script exits unexpectedly or the process is killed before script cleanup, the user can be left with modified preferences they did not intend to keep.

An idea to address this, was adding support for temporary preferences overrides.

set_temporary_property(string preference, string value)

This would set a memory-only preference stored separately from persistent preferences, and not written to disk.

temporary_property_exists(string preference)
remove_temporary_property(string preference)
get_actual_property(string preference)

The function get_property would return the temporary preference if set, otherwise the actual preference.

Calling set_property sets the actual preference and removes any temporary overrides.

For example, if a parent script temporarily clears afterAdventureScript and a child script later calls set_property, the child's change is saved to disk and the parent script now sees the updated value. If neither script restores the original value, it is permanently changed. It's up to the scripts to handle that.

As for the scope and lifetime of temporary preferences, two possible ideas.

1. Script scope

Temporary preferences exist only for the current script runtime.

Each runtime can set their own temporary values.

The newest value is the active value, what's returned by get_property

When the runtime is no longer running, it falls through to the next oldest value.

Picture it as

get_property(string preference) {
    List temporarySettings;
    temporarySettings.removeIf( ! setting.runtime.isRunning())

    if (temporarySettings.isEmpty()) return actualSetting;

    temporarySettings.sort(setting.timestamp)
    return temporarySettings.newest()
}

set_temporary_property(ScriptRuntime runtime, string preference, string value) {
    List temporarySettings = map.get(preference);
    setting = new Setting(scriptRuntime, timestamp, value);
    temporarySettings.add(setting)
}

set_property(string preference, string value) {
   // This is being saved to file, remove all temporary settings.
    temporarySettings.get(preference).clear();
    // Normal code
}

2. Session scope

Temporary preferences persist in memory until preferences are loaded or unloaded, at which point they are cleared.

They will persist even when no scripts are active. There's only a single global Map<string, string> temporaryPreferences = new Map();

The downside of a session scope, is that the user may be surprised when a script fails to clean up after itself, and may have headaches trying to figure out why things are working weirdly. It would also mean more care would need to be taken to ensure that the user is aware when a temporary setting is active, whereas with a script runtime, normally it would finish before the user's input is considered.

Eg, the commands choice, prefref and get

As a final note, could also be worth introducing a new preference logTemporaryPreferenceChange that acts as logPreferenceChange does. And "prefref" and "get" commands would likely need to be updated to show both the actual, and the temporary preferences for assistance in debugging.

Thoughts?

 

[Ryo_Sangnoir]

Session scope already exists: statics for ASH, sessionStorage for JavaScript. They're just not preferences.

I think if someone wants to have preference-like behaviour, they should just use preferences. I think autoscend uses preferences for backup, it just doesn't have an automated process to restore from backup.

Garbo similarly tries to reset in try-finally, which fails in case of abort or if Mafia unexpectedly closes or whatever. But in the latter case, an in-memory store isn't saving you either.

 

[Ryo_Sangnoir]

I suppose my perspective is this:

> If a script exits unexpectedly or the process is killed before script cleanup, the user can be left with modified preferences they did not intend to keep.

How does the script exit unexpectedly? If a power cut or Mafia close, we're not going to be able to use anything in-memory to fix it anyway. If it's the user slamming escape repeatedly, yeah that's a problem that abort is caught by exception handlers but I don't really know how to fix it. The script can handle unexpected escapes: either with a try-finally for JavaScript scripts or by storing the backup preferences and checking if they exist and if so restoring them for ASH or JS scripts that want to work even if Mafia closes.

I think scripts storing backup preferences is a better and more general solution than anything in-memory we could do.

 

[VeeArr]

I think scripts storing backup preferences is a better and more general solution than anything in-memory we could do.

I disagree. The status quo of having every script that wants to temporarily change a preference need to implement their own mechanism for saving off temporarily-changed preferences (and a mechanism to manually restore them in case of unexpected exit) is very clunky.

How does the script exit unexpectedly? If a power cut or Mafia close, we're not going to be able to use anything in-memory to fix it anyway.

This feels like it misses one of the primary motivations for a change like this--if the temporary preference is never persisted, there's nothing to fix! The "permanent" value is what the user ends up with when they restart mafia, which is what is desired anyway.

 

[Ryo_Sangnoir]

This feels like it misses one of the primary motivations for a change like this--if the temporary preference is never persisted, there's nothing to fix! The "permanent" value is what the user ends up with when they restart mafia, which is what is desired anyway.

Yep, I did miss that.

Let me think about this a bit more.

Design-wise we have to consider choice adventure / pre-adventure / post-adventure / fight / spading scripts etc. All these are called implicitly whenever the character spends an adventure / enters a choice / fight / any number of things for spading hooks. They're going to want to access the properties set by the script (e.g., temporary choice adventure settings).

Garbo currently uses "withProperty" to temporarily set a property. We want to support this behaviour. This would be satisfied with set_temporary / remove_temporary, assuming that temporary properties go into a stack.

Autoscend sets many settings at script start. This is purely set_temporary.

What's the intended use-case of get_actual_property / temporary_property_exists? Why does a script need to know if there's a temporary property in place? Is this to support "Calling set_property sets the actual preference and removes any temporary overrides."? I feel like in most cases you want set_property to just change the value written to disk and keep the temporary preference in place. You wouldn't want a user's counter script to be able to clear your script-set temporary preferences.

...okay, that would make get_property work in a confusing way. Let me think some more.

 

[Ryo_Sangnoir]

Okay, so to give a concrete example:

Looking on GitHub, scripts that would be affected are autoscend, garbage-collector, ttpack, combo, oreo, and anything that uses grimoire.

Garbo calls out to both combo and oreo. Suppose garbo is updated with set_temporary and oreo is left as-is. Then logPreferenceChangeFilter:

* starts as user,stuff
* is temporarily set to garbo,user,stuff
* is permanently set to oreo,garbo,user,stuff by oreo, clearing the temporary variables
* is reset to garbo,user,stuff by oreo after oreo does its thing, because that was the value when it started
* is still the same after garbo finishes and unsets the temporary variables, because they were already cleared

After garbo finishes, it's still set to garbo,user,stuff. What's the intent here? All scripts are responsible for ensuring all the downstream scripts use set_temporary if they do?

We could instead say that set_property adds a temporary property if there's already a temporary property. Haven't really thought this design through thoroughly though.

I have vague concerns over adding a check for temporary properties to every get_property call, as that's something we do a lot, but it's O(1) so I expect it's fine. We need to do this for the Mafia-internal property lookups too, due to e.g. choiceAdventure choices.

Also thinking about remove_temporary, if a script sets up a temporary variable, then calls another script that sets up another, a remove_temporary by the first script could remove the wrong temporary variable. After the script runs, do we want Mafia to remove all temporary variables? This would stop being able to use it for e.g. temporarily setting a choice in a pre-adventure, but you probably do want to write that to disk normally in case of crash. If it's a script called by a different script, we presumably want Mafia to clear the temporary variables set by that script.

Consider again garbo calling oreo, with the "script-scope" style:

* garbo update preference to tempGarbo
* oreo updates preference to tempOreo
* oreo finishes

At this point, we (Mafia) want to set the preference back to tempGarbo, not to clear all the temporary variables.

With the session-scope style, oreo would be responsible for clearing its own temporary variable, in a try-finally like what currently happens with JS script preferences.

 

[xKiv]

There are two very different things you can be trying to do with this.

1) "dynamic scoping" type behavior
Here you are saying "I want the value to change to X during execution of this function (or a specified code block), and then automatically revert back when the function ends". The changed value is still current in anything you call from inside that function - unless you call something that also uses this dynamic scoping, in which case it naturally nests.
I think scoping this "for the entire script" is nebulous, ambiguous, mad, bad, and dangerous. Unless you are 100% fine with saying that the entire-script scope is only "engaged" when running the entire script and not just using it as a library.
Such changes cannot persist between calls to the same script from the outside. I am confident that trying to do that is just asking for trouble as soon as you start combining it ouside the situations you explicitly thought of when designing.

2) Unscoped stack of values
This is for cases when you want the temporary changes persist after the first call to the script, until some later call to the script reverts them - and they have to persist even when the script execution is currently "done".
Here the scope is controlled by function calls - every script/library is responsible for ending its own scope. That means the scopes need to have some kind of key associated with them - set_temporary_property would also register that key, and remove_temporary_property would take that key and only remove values that were registered with that key. Let script writers have a gentlemanly aggreement on what keys they use, this is not a highly secure zero-trust environment.
But now you also have choice about what to do if you are about to remove value that's on the stack, but not on top of the stack:
2a - remove ALL the values that need to pop before you get to the one you actually want to remove
2b - surgically remove that one value from the middle of the stack, leave the rest be
2c - error out, loudly
2d - error out, silently
(I think a script should be able to ask for desired behavior b c or d, not sure about a - but I think *user* must be able to reset all the way back to empty stack, even)
You also have a similar choice for when you try to set_temporary_property with a key that already exists. New item on stack? Remove according to removal rules then reapply?

Either way, this will can have weird behavior when script A pushes a change, then script B pushes a change, and then you run some functionality from A that expects the changes that A made ... but gets the changes that B made instead.

---

Questions
Q1: how should this interact with scripts that are not run explicitly by user action / from within other such explciitly called scripts?
I mean various handler scripts (healing script, between battle script, relay scripts, override scripts, chat scripts, ....)
Q2: should a script have the ability to say "disregard any temporary changes while this script is running"? (I think this exclusion only makes sense to be dynamiclly scoped)
Q3: if a script function makes temporary changes, do you need those changes to persist after the function ends (never/sometimes/always)?