CHOP-962: Preserve unrecognized user settings variables.

Change LLControlVariable::mPersist from bool to tri-state enum. Its 'true'
value used to mean "persist only if changed from default;" third state now
means "persist regardless of value." Add forcePersist() method to set that.
Replace isSaveValueDefault() method -- used only by logic to determine whether
to save the variable -- with shouldSave() method to encapsulate ALL that logic
rather than only part of it. shouldSave() recognizes PERSIST_ALWAYS state.
When loading an unrecognized control variable from a user settings file, use
forcePersist() to ensure that we later save that variable again.
Tweak one of the unit tests to adjust for new semantics.

3 files changed, 75 insertions, 29 deletions

diff --git a/indra/llxml/llcontrol.cpp b/indra/llxml/llcontrol.cpp
index 53550d6409..9dc6292cb2 100755
--- a/indra/llxml/llcontrol.cpp
+++ b/indra/llxml/llcontrol.cpp
@@ -136,10 +136,10 @@ LLControlVariable::LLControlVariable(const std::string& name, eControlType type,
 	: mName(name),
 	  mComment(comment),
 	  mType(type),
-	  mPersist(persist),
+	  mPersist(persist? PERSIST_YES : PERSIST_NO),
 	  mHideFromSettingsEditor(hidefromsettingseditor)
 {
-	if (mPersist && mComment.empty())
+	if (persist && mComment.empty())
 	{
 		llerrs << "Must supply a comment for control " << mName << llendl;
 	}
@@ -262,7 +262,12 @@ void LLControlVariable::setDefaultValue(const LLSD& value)
 
 void LLControlVariable::setPersist(bool state)
 {
-	mPersist = state;
+	mPersist = state? PERSIST_YES : PERSIST_NO;
+}
+
+void LLControlVariable::forcePersist()
+{
+	mPersist = PERSIST_ALWAYS;
 }
 
 void LLControlVariable::setHiddenFromSettingsEditor(bool hide)
@@ -292,10 +297,29 @@ void LLControlVariable::resetToDefault(bool fire_signal)
 	}
 }
 
-bool LLControlVariable::isSaveValueDefault()
-{ 
-    return (mValues.size() ==  1) 
-        || ((mValues.size() > 1) && llsd_compare(mValues[1], mValues[0]));
+bool LLControlVariable::shouldSave(bool nondefault_only)
+{
+	// This method is used to decide whether we should save a given
+	// variable. Two of the three values of mPersist are easy.
+	if (mPersist == PERSIST_NO)
+		return false;
+
+	if (mPersist == PERSIST_ALWAYS)
+		return true;
+
+	// PERSIST_YES
+	// If caller doesn't need us to filter, just save.
+	if (! nondefault_only)
+		return true;
+
+	// PERSIST_YES, but caller only wants us to save this variable if its
+	// value differs from default.
+	if (isDefault())                // never been altered
+		return false;
+
+	// We've set at least one other value: compare it to default. Save only if
+	// they differ.
+	return ! llsd_compare(getSaveValue(), getDefault());
 }
 
 LLSD LLControlVariable::getSaveValue() const
@@ -805,21 +829,12 @@ U32 LLControlGroup::saveToFile(const std::string& filename, BOOL nondefault_only
 		{
 			llwarns << "Tried to save invalid control: " << iter->first << llendl;
 		}
-
-		if( control && control->isPersisted() )
+		else if( control->shouldSave(nondefault_only) )
 		{
-			if (!(nondefault_only && (control->isSaveValueDefault())))
-			{
-				settings[iter->first]["Type"] = typeEnumToString(control->type());
-				settings[iter->first]["Comment"] = control->getComment();
-				settings[iter->first]["Value"] = control->getSaveValue();
-				++num_saved;
-			}
-			else
-			{
-				// Debug spam
-				// llinfos << "Skipping " << control->getName() << llendl;
-			}
+			settings[iter->first]["Type"] = typeEnumToString(control->type());
+			settings[iter->first]["Comment"] = control->getComment();
+			settings[iter->first]["Value"] = control->getSaveValue();
+			++num_saved;
 		}
 	}
 	llofstream file;
@@ -920,15 +935,35 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v
 		}
 		else
 		{
-			declareControl(name, 
+			// We've never seen this control before. Either we're loading up
+			// the initial set of default settings files (set_default_values)
+			// -- or we're loading user settings last saved by a viewer that
+			// supports a superset of the variables we know.
+			LLControlVariable* control = declareControl(name, 
 						   typeStringToEnum(control_map["Type"].asString()), 
 						   control_map["Value"], 
 						   control_map["Comment"].asString(), 
 						   persist,
 						   hidefromsettingseditor
 						   );
+			// CHOP-962: if we're loading an unrecognized user setting, make
+			// sure we save it later. If you try an experimental viewer, tweak
+			// a new setting, briefly revert to an old viewer, then return to
+			// the new one, we don't want the old viewer to discard the
+			// setting you changed.
+			if (! set_default_values)
+			{
+				// Using forcePersist() insists that saveToFile() (which calls
+				// LLControlVariable::shouldSave()) must save this control
+				// variable regardless of its value. We can safely set this
+				// LLControlVariable persistent because the 'persistent' flag
+				// is not itself persisted!
+				control->forcePersist();
+				LL_INFOS("LLControlGroup") << "preserving unrecognized user settings variable "
+										   << name << LL_ENDL;
+			}
 		}
-		
+
 		++validitems;
 	}
 
diff --git a/indra/llxml/llcontrol.h b/indra/llxml/llcontrol.h
index a7424c6780..5d6ac33780 100755
--- a/indra/llxml/llcontrol.h
+++ b/indra/llxml/llcontrol.h
@@ -101,13 +101,19 @@ public:
 	typedef boost::signals2::signal<void(LLControlVariable* control, const LLSD&, const LLSD&)> commit_signal_t;
 
 private:
+	enum
+	{
+		PERSIST_NO,                 // don't save this var
+		PERSIST_YES,                // save this var if differs from default
+		PERSIST_ALWAYS              // save this var even if has default value
+	} mPersist;
+
 	std::string		mName;
 	std::string		mComment;
 	eControlType	mType;
-	bool			mPersist;
 	bool			mHideFromSettingsEditor;
 	std::vector<LLSD> mValues;
-	
+
 	commit_signal_t mCommitSignal;
 	validate_signal_t mValidateSignal;
 	
@@ -131,8 +137,8 @@ public:
 	validate_signal_t* getValidateSignal() { return &mValidateSignal; }
 
 	bool isDefault() { return (mValues.size() == 1); }
-	bool isSaveValueDefault();
-	bool isPersisted() { return mPersist; }
+	bool shouldSave(bool nondefault_only);
+	bool isPersisted() { return mPersist != PERSIST_NO; }
 	bool isHiddenFromSettingsEditor() { return mHideFromSettingsEditor; }
 	LLSD get()			const	{ return getValue(); }
 	LLSD getValue()		const	{ return mValues.back(); }
@@ -142,7 +148,8 @@ public:
 	void set(const LLSD& val)	{ setValue(val); }
 	void setValue(const LLSD& value, bool saved_value = TRUE);
 	void setDefaultValue(const LLSD& value);
-	void setPersist(bool state);
+	void setPersist(bool state);    // persist or not depending on value
+	void forcePersist();            // always persist regardless of value
 	void setHiddenFromSettingsEditor(bool hide);
 	void setComment(const std::string& comment);
 
diff --git a/indra/llxml/tests/llcontrol_test.cpp b/indra/llxml/tests/llcontrol_test.cpp
index ede81956ec..c273773c9b 100755
--- a/indra/llxml/tests/llcontrol_test.cpp
+++ b/indra/llxml/tests/llcontrol_test.cpp
@@ -128,7 +128,11 @@ namespace tut
 	template<> template<>
 	void control_group_t::test<3>()
 	{
-		int results = mCG->loadFromFile(mTestConfigFile.c_str());
+		// Pass default_values = true. This tells loadFromFile() we're loading
+		// a default settings file that declares variables, rather than a user
+		// settings file. When loadFromFile() encounters an unrecognized user
+		// settings variable, it forcibly preserves it (CHOP-962).
+		int results = mCG->loadFromFile(mTestConfigFile.c_str(), true);
 		LLControlVariable* control = mCG->getControl("TestSetting");
 		LLSD new_value = 13;
 		control->setValue(new_value, FALSE);