Passing a temporary default -option=value setting via command line overwrites persistent non-default setting in settings.json

[dmaluka]

Description of the problem or steps to reproduce

Passing options via micro -option=value in the command line should only temporarily override the option value for the current micro session, not change it permanently. But currently it wrongly changes it permanently in the case when the value passed via command line is the default value of this option, while the permanent setting in settings.json is a non-default value.

For example, if the parsecursor option is currently set to its default value (i.e. false), i.e. is not overridden in settings.json, then if we run micro with -parsecursor=true command-line flag, then, as expected, it does not write this setting persistently to settings.json, it only temporarily overrides it for the current micro session.

But, if we change parsecursor to true (non-default value) in settings.json, then, if we run micro with -parsecursor=false (default value), it unexpectedly removes the non-default setting from settings.json (so that when we start micro next time, without passing -parsecursor=false, we can see that parsecursor is unexpectedly set to false).

Specifications

Commit hash: d8e9d61
OS: any
Terminal: any

[dmaluka]

I see the problem is in this logic in WriteSettings():

		// remove any options froms parsedSettings that have since been marked as default
		for k, v := range parsedSettings {
			if !strings.HasPrefix(reflect.TypeOf(v).String(), "map") {
				cur, okcur := GlobalSettings[k]
				if def, ok := defaults[k]; ok && okcur && reflect.DeepEqual(cur, def) {
					delete(parsedSettings, k)
				}
			}
		}

since it doesn't take into account that the current setting may be a temporary setting.

[JoeKar]

Maybe add after micro.go#L277 something similar to settings.go#L35/ModifiedSettings e.g. TemporarySettings, which then can be checked within WriteSettings()?

[dmaluka]

Yep, I already have a local patch doing something like that. I think I'll submit a PR soon.

Generally, the code for handling settings (initializing settings at startup, changing them at runtime, saving settings to settings.json, registering new settings at runtime...) is IMO quite messy and not quite readable. (And this fix is gonna make it even messier.) It would be nice to refactor this code, to make it built around a single data structure describing the state of a global setting, with fields like "value", "default value", "global only", "modified", "temporary" etc.

[JoeKar]

It would be nice to refactor this code, [...], with fields like "value", "default value", "global only", "modified", "temporary" etc.

Yes, that's definitely more structured then. So we can easily add something like option locking too in case a plugin would need such a feature.

[dmaluka]

I don't know what you mean by "option locking", but in this case I'm thinking not even about better extensibility for shiny new features, but rather about better robustness of the existing functionality (i.e. fewer bugs like this in the future, and less effort and more fun to debug and fix bugs like this in the future).

[JoeKar]

I don't know what you mean by "option locking", [...]

I was just imaging a scenario in which one could have a plugin locking one or more options (can't be changed (temporary) by the user then) due to whatever reason. This can then be realized with less effort. But never mind, it was just an idea without any reason so far.
Maybe I was a bit misleading, since this wasn't meant to be a direct request to add more and more fancy stuff, probably no one ever needs. 😉

[...] but in this case I'm thinking not even about better extensibility for shiny new features, but rather about better robustness of the existing functionality (i.e. fewer bugs like this in the future, and less effort and more fun to debug and fix bugs like this in the future).

That's definitely worth the effort and noble in case somebody takes care of such things too! 👍