Fix regression where autosave function is off by default

[zonkmachine]

Autosave regression #181 (comment)

Introduced here: 901fea5

[tresf]

@zonkmachine thanks. Although this fixes the problem in the UI, can you help explain what's happening in the config? Is this value being stored incorrectly as code looks backwards now. :)

[tresf]

Ok... I think I see what happened... @jasp00 complained about double-negatives in our variable names, but we don't currently seem to have a way to default an unconfigured property to enabled, right?

[zonkmachine]

I'm basically just copying the logic of the other 'default on' buttons. SetupDialog.cpp and I aren't really on speaking terms...

[zonkmachine]

Merge?

but we don't currently seem to have a way to default an unconfigured property to enabled, right?

Yes, something like that. In any case I've never had an easy time when I've touched that file.

[tresf]

Merge?

No unfortunately now the code is backwards, so the patch is masking the problem.

@jasp00's advice was pragmatic but impractical. We either revert the variable naming to what the project uses and keep this type of patch, or spend the time necessary to have default-on in our config file.

[zonkmachine]

We either revert the variable naming to what the project uses and keep this type of patch,

I didn't get this.

or spend the time necessary to have default-on in our config file.

Sorry. I don't have much time now.

[tresf]

I'm not sure how else to phrase this properly....

m_enableAutoSave( !ConfigManager::inst()->value( "ui", "enableautosave" ).toInt() ),

This essentially says

m_enableAutoSave = !enableautosave;

These are the same conceptual concerns @jasp00 raised in #3088.

There's likely a better way to handle this all in our ConfigManager class to actually fix the underlying problem of default int value is zero. Alternately, we can go back to the old disableautosave verbiage and just let it be zero blank -- resolving to zero -- naturally, but the logic proposed in this PR is incorrect.

[tresf]

Ok... I'd appreciate a second set of eyes on this, but if what I'm observing is true, all of our booleans can never be true by default, which punishes anyone that wants a "default on" value (e.g. enablefoo) without a "default off" sounding name (e.g. disablefoo)

Here's a hacked proposal... (better proposal below)

Note, this has some implications... namely, it potentially enables animateafp, displaydbfs and enableautosave by default, but it uses a parses a String to determine what we "meant". It's the wrong way, but illustrates the problem.

const QString & ConfigManager::value( const QString & cls,
					const QString & attribute ) const
{
	if( m_settings.contains( cls ) )
	{
		for( stringPairVector::const_iterator it =
						m_settings[cls].begin();
					it != m_settings[cls].end(); ++it )
		{
			if( ( *it ).first == attribute )
			{
				return ( *it ).second ;
			}
		}
	}
	static QString empty
+	// Default enable flags to true
+	static QString on("1");
-	return empty;
+	return attribute.startsWith("enable") ||
+		attribute.startsWith("animate")  ||
+		attribute.startsWith("display") ? on : empty;
}

The better proposal... add new function that accepts a default value when one can't be found... (pseudocode, untested).

Worth noting, this is the technique that every config parser I've ever seen handles default values.

const QString & ConfigManager::value( const QString & cls, const QString & attribute, const QString & defaultVal ) const
{
	QString val = value(cls, attribute);
        return val.isEmpty() ? defaultVal : val;
}

Usage:

m_enableAutoSave( ConfigManager::inst()->value( "ui", "enableautosave", "1" ).toInt() ),
//                                                  DEFAULT VALUE  ------^

[zonkmachine]

Alternately, we can go back to the old disableautosave verbiage and just let it be zero blank -- resolving to zero -- naturally, but the logic proposed in this PR is incorrect.

So, just reverting 901fea5 ?
I suggest you start a separate issue with the above proposed code. I'm not going to have time to implement anything like that here and now.

[tresf]

So, just reverting 901fea5 ?

That was my original thought, but please read the bottom of the post, I provided a better fix that's only a few lines and allows us to keep the enabled flag.

[zonkmachine]

That was my original thought, but please read the bottom of the post, I provided a better fix that's only a few lines and allows us to keep the enabled flag.

Still not within my time limit. What I can do is revert 901fea5. I won't have time to look any deeper into things right now and for a possible long time coming.

[tresf]

Still not within my time limit.

Someone else will have to tackle it then. This PR is as good as a bug report since it contains the valid info. We'll either supercede this will a new PR or use this PR directly for the code.

[tresf]

Superseded by #3551.