From 2b3c1bd40d11b02cb097cb3dba7a661a4b528a91 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 17 Jul 2013 08:23:42 -0400 Subject: CHOP-962: Make LLControlGroup::declare* return LLControlVariable* LLControlGroup::declareControl(), declareString() etc. etc. all used to return BOOL -- which no one ever examines because it unconditionally returned TRUE. Make it return the (possibly new) LLControlVariable* instead. --- indra/llxml/llcontrol.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) (limited to 'indra/llxml/llcontrol.cpp') diff --git a/indra/llxml/llcontrol.cpp b/indra/llxml/llcontrol.cpp index 666c03e9ff..53550d6409 100755 --- a/indra/llxml/llcontrol.cpp +++ b/indra/llxml/llcontrol.cpp @@ -355,7 +355,7 @@ std::string LLControlGroup::typeEnumToString(eControlType typeenum) return mTypeString[typeenum]; } -BOOL LLControlGroup::declareControl(const std::string& name, eControlType type, const LLSD initial_val, const std::string& comment, BOOL persist, BOOL hidefromsettingseditor) +LLControlVariable* LLControlGroup::declareControl(const std::string& name, eControlType type, const LLSD initial_val, const std::string& comment, BOOL persist, BOOL hidefromsettingseditor) { LLControlVariable* existing_control = getControl(name); if (existing_control) @@ -374,66 +374,66 @@ BOOL LLControlGroup::declareControl(const std::string& name, eControlType type, { llwarns << "Control named " << name << " already exists, ignoring new declaration." << llendl; } - return TRUE; + return existing_control; } // if not, create the control and add it to the name table LLControlVariable* control = new LLControlVariable(name, type, initial_val, comment, persist, hidefromsettingseditor); mNameTable[name] = control; - return TRUE; + return control; } -BOOL LLControlGroup::declareU32(const std::string& name, const U32 initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareU32(const std::string& name, const U32 initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_U32, (LLSD::Integer) initial_val, comment, persist); } -BOOL LLControlGroup::declareS32(const std::string& name, const S32 initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareS32(const std::string& name, const S32 initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_S32, initial_val, comment, persist); } -BOOL LLControlGroup::declareF32(const std::string& name, const F32 initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareF32(const std::string& name, const F32 initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_F32, initial_val, comment, persist); } -BOOL LLControlGroup::declareBOOL(const std::string& name, const BOOL initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareBOOL(const std::string& name, const BOOL initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_BOOLEAN, initial_val, comment, persist); } -BOOL LLControlGroup::declareString(const std::string& name, const std::string& initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareString(const std::string& name, const std::string& initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_STRING, initial_val, comment, persist); } -BOOL LLControlGroup::declareVec3(const std::string& name, const LLVector3 &initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareVec3(const std::string& name, const LLVector3 &initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_VEC3, initial_val.getValue(), comment, persist); } -BOOL LLControlGroup::declareVec3d(const std::string& name, const LLVector3d &initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareVec3d(const std::string& name, const LLVector3d &initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_VEC3D, initial_val.getValue(), comment, persist); } -BOOL LLControlGroup::declareRect(const std::string& name, const LLRect &initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareRect(const std::string& name, const LLRect &initial_val, const std::string& comment, BOOL persist) { return declareControl(name, TYPE_RECT, initial_val.getValue(), comment, persist); } -BOOL LLControlGroup::declareColor4(const std::string& name, const LLColor4 &initial_val, const std::string& comment, BOOL persist ) +LLControlVariable* LLControlGroup::declareColor4(const std::string& name, const LLColor4 &initial_val, const std::string& comment, BOOL persist ) { return declareControl(name, TYPE_COL4, initial_val.getValue(), comment, persist); } -BOOL LLControlGroup::declareColor3(const std::string& name, const LLColor3 &initial_val, const std::string& comment, BOOL persist ) +LLControlVariable* LLControlGroup::declareColor3(const std::string& name, const LLColor3 &initial_val, const std::string& comment, BOOL persist ) { return declareControl(name, TYPE_COL3, initial_val.getValue(), comment, persist); } -BOOL LLControlGroup::declareLLSD(const std::string& name, const LLSD &initial_val, const std::string& comment, BOOL persist ) +LLControlVariable* LLControlGroup::declareLLSD(const std::string& name, const LLSD &initial_val, const std::string& comment, BOOL persist ) { return declareControl(name, TYPE_LLSD, initial_val, comment, persist); } @@ -887,6 +887,8 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v LLControlVariable* existing_control = getControl(name); if(existing_control) { + // set_default_values is true when we're loading the initial, + // immutable files from app_settings, e.g. settings.xml. if(set_default_values) { // Override all previously set properties of this control. @@ -908,6 +910,9 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v } else if(existing_control->isPersisted()) { + // save_values is specifically false for (e.g.) + // SessionSettingsFile and UserSessionSettingsFile -- in other + // words, for a file that's supposed to be transient. existing_control->setValue(control_map["Value"], save_values); } // *NOTE: If not persisted and not setting defaults, -- cgit v1.2.3 From fd14c250b8aa7b2d512194b164646ff7f658bef3 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 17 Jul 2013 11:30:09 -0400 Subject: 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. --- indra/llxml/llcontrol.cpp | 81 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 23 deletions(-) (limited to 'indra/llxml/llcontrol.cpp') 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; } -- cgit v1.2.3 From 35bc91fc5dee77133fff0c0066e3e34689941dfd Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 22 Jul 2013 22:06:48 -0400 Subject: CHOP-962: Emit unrecognized-var log message only for Global settings. LLControlGroup::loadFromFile() can of course detect which LLControlGroup instance it's loading. We only want to log unrecognized settings variables in LLControlGroup "Global". Settings for "Don't show me this again" notifications are in group "Warnings". --- indra/llxml/llcontrol.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'indra/llxml/llcontrol.cpp') diff --git a/indra/llxml/llcontrol.cpp b/indra/llxml/llcontrol.cpp index 9dc6292cb2..6e11a5f6f3 100755 --- a/indra/llxml/llcontrol.cpp +++ b/indra/llxml/llcontrol.cpp @@ -959,8 +959,20 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v // LLControlVariable persistent because the 'persistent' flag // is not itself persisted! control->forcePersist(); - LL_INFOS("LLControlGroup") << "preserving unrecognized user settings variable " - << name << LL_ENDL; + // We want to mention unrecognized user settings variables + // (e.g. from a newer version of the viewer) in the log. But + // we also arrive here for Boolean variables generated by + // the notifications subsystem when the user checks "Don't + // show me this again." These aren't declared in settings.xml; + // they're actually named for the notification they suppress. + // We don't want to mention those. Apologies, this is a bit of + // a hack: we happen to know that user settings go into an + // LLControlGroup whose name is "Global". + if (getKey() == "Global") + { + LL_INFOS("LLControlGroup") << "preserving unrecognized " << getKey() + << " settings variable " << name << LL_ENDL; + } } } -- cgit v1.2.3 From d2386652f8a44c5adcc7fbcc65741649dd90aa2b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 25 Jul 2013 16:46:51 -0400 Subject: CHOP-962: Make LLControlVariable::setPersist() accept only enum. Initial change made LLControlVariable::mPersist an enum, but retained bool/BOOL public API. setPersist(true) set one value, setPersist(false) set another, forcePersist() set the third. Per code review, expose enum to public, make setPersist() (and LLControlVariable constructor, and LLControlGroup:: declareControl(), and all the LLControlGroup::declareMumble() methods, and all the unit-test dummy declareMumble() method bodies) accept that enum. Remove forcePersist(). Fix calls to LLControlGroup::declareMumble() accordingly. Also rename PERSIST_YES to PERSIST_NONDFT, also per code review. --- indra/llxml/llcontrol.cpp | 75 +++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 39 deletions(-) (limited to 'indra/llxml/llcontrol.cpp') diff --git a/indra/llxml/llcontrol.cpp b/indra/llxml/llcontrol.cpp index 6e11a5f6f3..16f2290787 100755 --- a/indra/llxml/llcontrol.cpp +++ b/indra/llxml/llcontrol.cpp @@ -132,14 +132,14 @@ bool LLControlVariable::llsd_compare(const LLSD& a, const LLSD & b) LLControlVariable::LLControlVariable(const std::string& name, eControlType type, LLSD initial, const std::string& comment, - bool persist, bool hidefromsettingseditor) + ePersist persist, bool hidefromsettingseditor) : mName(name), mComment(comment), mType(type), - mPersist(persist? PERSIST_YES : PERSIST_NO), + mPersist(persist), mHideFromSettingsEditor(hidefromsettingseditor) { - if (persist && mComment.empty()) + if ((persist != PERSIST_NO) && mComment.empty()) { llerrs << "Must supply a comment for control " << mName << llendl; } @@ -260,14 +260,9 @@ void LLControlVariable::setDefaultValue(const LLSD& value) } } -void LLControlVariable::setPersist(bool state) +void LLControlVariable::setPersist(ePersist state) { - mPersist = state? PERSIST_YES : PERSIST_NO; -} - -void LLControlVariable::forcePersist() -{ - mPersist = PERSIST_ALWAYS; + mPersist = state; } void LLControlVariable::setHiddenFromSettingsEditor(bool hide) @@ -307,13 +302,13 @@ bool LLControlVariable::shouldSave(bool nondefault_only) if (mPersist == PERSIST_ALWAYS) return true; - // PERSIST_YES + // PERSIST_NONDFT // 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. + // PERSIST_NONDFT: caller only wants us to save this variable if its value + // differs from default. if (isDefault()) // never been altered return false; @@ -379,12 +374,12 @@ std::string LLControlGroup::typeEnumToString(eControlType typeenum) return mTypeString[typeenum]; } -LLControlVariable* LLControlGroup::declareControl(const std::string& name, eControlType type, const LLSD initial_val, const std::string& comment, BOOL persist, BOOL hidefromsettingseditor) +LLControlVariable* LLControlGroup::declareControl(const std::string& name, eControlType type, const LLSD initial_val, const std::string& comment, LLControlVariable::ePersist persist, BOOL hidefromsettingseditor) { LLControlVariable* existing_control = getControl(name); if (existing_control) { - if (persist && existing_control->isType(type)) + if ((persist != LLControlVariable::PERSIST_NO) && existing_control->isType(type)) { if (!existing_control->llsd_compare(existing_control->getDefault(), initial_val)) { @@ -407,57 +402,57 @@ LLControlVariable* LLControlGroup::declareControl(const std::string& name, eCont return control; } -LLControlVariable* LLControlGroup::declareU32(const std::string& name, const U32 initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareU32(const std::string& name, const U32 initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_U32, (LLSD::Integer) initial_val, comment, persist); } -LLControlVariable* LLControlGroup::declareS32(const std::string& name, const S32 initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareS32(const std::string& name, const S32 initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_S32, initial_val, comment, persist); } -LLControlVariable* LLControlGroup::declareF32(const std::string& name, const F32 initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareF32(const std::string& name, const F32 initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_F32, initial_val, comment, persist); } -LLControlVariable* LLControlGroup::declareBOOL(const std::string& name, const BOOL initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareBOOL(const std::string& name, const BOOL initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_BOOLEAN, initial_val, comment, persist); } -LLControlVariable* LLControlGroup::declareString(const std::string& name, const std::string& initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareString(const std::string& name, const std::string& initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_STRING, initial_val, comment, persist); } -LLControlVariable* LLControlGroup::declareVec3(const std::string& name, const LLVector3 &initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareVec3(const std::string& name, const LLVector3 &initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_VEC3, initial_val.getValue(), comment, persist); } -LLControlVariable* LLControlGroup::declareVec3d(const std::string& name, const LLVector3d &initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareVec3d(const std::string& name, const LLVector3d &initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_VEC3D, initial_val.getValue(), comment, persist); } -LLControlVariable* LLControlGroup::declareRect(const std::string& name, const LLRect &initial_val, const std::string& comment, BOOL persist) +LLControlVariable* LLControlGroup::declareRect(const std::string& name, const LLRect &initial_val, const std::string& comment, LLControlVariable::ePersist persist) { return declareControl(name, TYPE_RECT, initial_val.getValue(), comment, persist); } -LLControlVariable* LLControlGroup::declareColor4(const std::string& name, const LLColor4 &initial_val, const std::string& comment, BOOL persist ) +LLControlVariable* LLControlGroup::declareColor4(const std::string& name, const LLColor4 &initial_val, const std::string& comment, LLControlVariable::ePersist persist ) { return declareControl(name, TYPE_COL4, initial_val.getValue(), comment, persist); } -LLControlVariable* LLControlGroup::declareColor3(const std::string& name, const LLColor3 &initial_val, const std::string& comment, BOOL persist ) +LLControlVariable* LLControlGroup::declareColor3(const std::string& name, const LLColor3 &initial_val, const std::string& comment, LLControlVariable::ePersist persist ) { return declareControl(name, TYPE_COL3, initial_val.getValue(), comment, persist); } -LLControlVariable* LLControlGroup::declareLLSD(const std::string& name, const LLSD &initial_val, const std::string& comment, BOOL persist ) +LLControlVariable* LLControlGroup::declareLLSD(const std::string& name, const LLSD &initial_val, const std::string& comment, LLControlVariable::ePersist persist ) { return declareControl(name, TYPE_LLSD, initial_val, comment, persist); } @@ -688,11 +683,11 @@ U32 LLControlGroup::loadFromFileLegacy(const std::string& filename, BOOL require switch(declare_as) { case TYPE_COL4: - declareColor4(name, LLColor4::white, LLStringUtil::null, NO_PERSIST); + declareColor4(name, LLColor4::white, LLStringUtil::null, LLControlVariable::PERSIST_NO); break; case TYPE_STRING: default: - declareString(name, LLStringUtil::null, LLStringUtil::null, NO_PERSIST); + declareString(name, LLStringUtil::null, LLStringUtil::null, LLControlVariable::PERSIST_NO); break; } } @@ -877,13 +872,14 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v for(LLSD::map_const_iterator itr = settings.beginMap(); itr != settings.endMap(); ++itr) { - bool persist = true; + LLControlVariable::ePersist persist = LLControlVariable::PERSIST_NONDFT; std::string const & name = itr->first; LLSD const & control_map = itr->second; if(control_map.has("Persist")) { - persist = control_map["Persist"].asInteger(); + persist = control_map["Persist"].asInteger()? + LLControlVariable::PERSIST_NONDFT : LLControlVariable::PERSIST_NO; } // Sometimes we want to use the settings system to provide cheap persistence, but we @@ -939,13 +935,6 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v // 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 @@ -953,12 +942,12 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v // setting you changed. if (! set_default_values) { - // Using forcePersist() insists that saveToFile() (which calls + // Using PERSIST_ALWAYS 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(); + persist = LLControlVariable::PERSIST_ALWAYS; // We want to mention unrecognized user settings variables // (e.g. from a newer version of the viewer) in the log. But // we also arrive here for Boolean variables generated by @@ -974,6 +963,14 @@ U32 LLControlGroup::loadFromFile(const std::string& filename, bool set_default_v << " settings variable " << name << LL_ENDL; } } + + declareControl(name, + typeStringToEnum(control_map["Type"].asString()), + control_map["Value"], + control_map["Comment"].asString(), + persist, + hidefromsettingseditor + ); } ++validitems; -- cgit v1.2.3