Allow user to override either English, or localized, skinned file.

Original LLDir::findSkinnedFilenames() implementation used a tricky rule: a
given skin directory was only considered if it provided a default-language
override for the sought filename, regardless of whether it also provided
localizations for that filename. Discussion with Richard clarifies that we
want to allow the user to override neither, either or both. Change
findSkinnedFilenames() accordingly; update unit tests to verify new semantics.

3 files changed, 134 insertions, 78 deletions

diff --git a/indra/llvfs/lldir.cpp b/indra/llvfs/lldir.cpp
index 4c0978b39e..5e5aeefba1 100644
--- a/indra/llvfs/lldir.cpp
+++ b/indra/llvfs/lldir.cpp
@@ -46,6 +46,8 @@
 #include <boost/range/begin.hpp>
 #include <boost/range/end.hpp>
 #include <boost/assign/list_of.hpp>
+#include <boost/bind.hpp>
+#include <boost/ref.hpp>
 #include <algorithm>
 
 using boost::assign::list_of;
@@ -569,6 +571,43 @@ std::string LLDir::findSkinnedFilename(const std::string &subdir,
 	return found.back();
 }
 
+// This method exists because the two code paths for
+// findSkinnedFilenames(ALL_SKINS) and findSkinnedFilenames(CURRENT_SKIN) must
+// generate the list of candidate pathnames in identical ways. The only
+// difference is in the body of the inner loop.
+template <typename FUNCTION>
+void LLDir::walkSearchSkinDirs(const std::string& subdir,
+							   const std::vector<std::string>& subsubdirs,
+							   const std::string& filename,
+							   const FUNCTION& function) const
+{
+	BOOST_FOREACH(std::string skindir, mSearchSkinDirs)
+	{
+		std::string subdir_path(add(skindir, subdir));
+		BOOST_FOREACH(std::string subsubdir, subsubdirs)
+		{
+			std::string full_path(add(add(subdir_path, subsubdir), filename));
+			if (fileExists(full_path))
+			{
+				function(subsubdir, full_path);
+			}
+		}
+	}
+}
+
+// ridiculous little helper function that should go away when we can use lambda
+inline void push_back(std::vector<std::string>& vector, const std::string& value)
+{
+	vector.push_back(value);
+}
+
+typedef std::map<std::string, std::string> StringMap;
+// ridiculous little helper function that should go away when we can use lambda
+inline void store_in_map(StringMap& map, const std::string& key, const std::string& value)
+{
+	map[key] = value;
+}
+
 std::vector<std::string> LLDir::findSkinnedFilenames(const std::string& subdir,
 													 const std::string& filename,
 													 ESkinConstraint constraint) const
@@ -587,11 +626,10 @@ std::vector<std::string> LLDir::findSkinnedFilenames(const std::string& subdir,
 	// Cache the default language directory for each subdir we've encountered.
 	// A cache entry whose value is the empty string means "not localized,
 	// don't bother checking again."
-	typedef std::map<std::string, std::string> LocalizedMap;
-	static LocalizedMap sLocalized;
+	static StringMap sLocalized;
 
 	// Check whether we've already discovered if this subdir is localized.
-	LocalizedMap::const_iterator found = sLocalized.find(subdir);
+	StringMap::const_iterator found = sLocalized.find(subdir);
 	if (found == sLocalized.end())
 	{
 		// We have not yet determined that. Is it one of the subdirs "known"
@@ -599,7 +637,7 @@ std::vector<std::string> LLDir::findSkinnedFilenames(const std::string& subdir,
 		if (sUnlocalized.find(subdir) != sUnlocalized.end())
 		{
 			// This subdir is known to be unlocalized. Remember that.
-			found = sLocalized.insert(LocalizedMap::value_type(subdir, "")).first;
+			found = sLocalized.insert(StringMap::value_type(subdir, "")).first;
 		}
 		else
 		{
@@ -608,20 +646,20 @@ std::vector<std::string> LLDir::findSkinnedFilenames(const std::string& subdir,
 			if (fileExists(add(subdir_path, "en")))
 			{
 				// defaultSkinDir/subdir contains subdir "en". That's our
-				// default language; this subdir is localized. 
-				found = sLocalized.insert(LocalizedMap::value_type(subdir, "en")).first;
+				// default language; this subdir is localized.
+				found = sLocalized.insert(StringMap::value_type(subdir, "en")).first;
 			}
 			else if (fileExists(add(subdir_path, "en-us")))
 			{
 				// defaultSkinDir/subdir contains subdir "en-us" but not "en".
 				// Set as default language; this subdir is localized.
-				found = sLocalized.insert(LocalizedMap::value_type(subdir, "en-us")).first;
+				found = sLocalized.insert(StringMap::value_type(subdir, "en-us")).first;
 			}
 			else
 			{
 				// defaultSkinDir/subdir contains neither "en" nor "en-us".
 				// Assume it's not localized. Remember that assumption.
-				found = sLocalized.insert(LocalizedMap::value_type(subdir, "")).first;
+				found = sLocalized.insert(StringMap::value_type(subdir, "")).first;
 			}
 		}
 	}
@@ -648,48 +686,52 @@ std::vector<std::string> LLDir::findSkinnedFilenames(const std::string& subdir,
 			subsubdirs.push_back(mLanguage);
 		}
 	}
-	// Code below relies on subsubdirs not being empty: more specifically, on
-	// front() being valid. There may or may not be additional entries, but we
-	// have at least one. For an unlocalized subdir, it's the only one; for a
-	// localized subdir, it's the default one.
-	llassert(! subsubdirs.empty());
 
 	// Build results vector.
 	std::vector<std::string> results;
-	BOOST_FOREACH(std::string skindir, mSearchSkinDirs)
-	{
-		std::string subdir_path(add(skindir, subdir));
-		// Does subdir_path/subsubdirs[0]/filename exist? If there's more than
-		// one entry in subsubdirs, the first is the default language ("en"),
-		// the second is the current language. A skin that contains
-		// subdir/language/filename without also containing subdir/en/filename
-		// is ill-formed: skip any such skin. So to decide whether to keep
-		// this skin dir or skip it, we need only check for the existence of
-		// the first subsubdir entry ("en" or only).
-		std::string subsubdir_path(add(add(subdir_path, subsubdirs.front()), filename));
-		if (! fileExists(subsubdir_path))
-			continue;
-
-		// Here the desired filename exists in the first subsubdir. That means
-		// this is a skindir we want to record in results. But if the caller
-		// passed constraint=CURRENT_SKIN, we must discard all previous skindirs.
-		if (constraint == CURRENT_SKIN)
-		{
-			results.clear();
-		}
-
-		// Now add every subsubdir in which filename exists. We already know
-		// it exists in the first one.
-		results.push_back(subsubdir_path);
-
-		// Append all remaining subsubdirs in which filename exists.
-		for (std::vector<std::string>::const_iterator ssdi(subsubdirs.begin() + 1), ssdend(subsubdirs.end());
-			 ssdi != ssdend; ++ssdi)
+	// The process we use depends on 'constraint'.
+	if (constraint != CURRENT_SKIN) // meaning ALL_SKINS
+	{
+		// ALL_SKINS is simpler: just return every pathname generated by
+		// walkSearchSkinDirs(). Tricky bit: walkSearchSkinDirs() passes its
+		// FUNCTION the subsubdir as well as the full pathname. We just want
+		// the full pathname.
+		walkSearchSkinDirs(subdir, subsubdirs, filename,
+						   boost::bind(push_back, boost::ref(results), _2));
+	}
+	else                            // CURRENT_SKIN
+	{
+		// CURRENT_SKIN turns out to be a bit of a misnomer because we might
+		// still return files from two different skins. In any case, this
+		// value of 'constraint' means we will return at most two paths: one
+		// for the default language, one for the current language (supposing
+		// those differ).
+		// It is important to allow a user to override only the localization
+		// for a particular file, for all viewer installs, without also
+		// overriding the default-language file.
+		// It is important to allow a user to override only the default-
+		// language file, for all viewer installs, without also overriding the
+		// applicable localization of that file.
+		// Therefore, treat the default language and the current language as
+		// two separate cases. For each, capture the most-specialized file
+		// that exists.
+		// Use a map keyed by subsubdir (i.e. language code). This allows us
+		// to handle the case of a single subsubdirs entry with the same logic
+		// that handles two. For every real file path generated by
+		// walkSearchSkinDirs(), update the map entry for its subsubdir.
+		StringMap path_for;
+		walkSearchSkinDirs(subdir, subsubdirs, filename,
+						   boost::bind(store_in_map, boost::ref(path_for), _1, _2));
+		// Now that we have a path for each of the default language and the
+		// current language, copy them -- in proper order -- into results.
+		// Don't drive this by walking the map itself: it matters that we
+		// generate results in the same order as subsubdirs.
+		BOOST_FOREACH(std::string subsubdir, subsubdirs)
 		{
-			subsubdir_path = add(add(subdir_path, *ssdi), filename);
-			if (fileExists(subsubdir_path))
+			StringMap::const_iterator found(path_for.find(subsubdir));
+			if (found != path_for.end())
 			{
-				results.push_back(subsubdir_path);
+				results.push_back(found->second);
 			}
 		}
 	}
diff --git a/indra/llvfs/lldir.h b/indra/llvfs/lldir.h
index ffa2676838..eb36faa085 100644
--- a/indra/llvfs/lldir.h
+++ b/indra/llvfs/lldir.h
@@ -208,6 +208,13 @@ protected:
 	// build mSearchSkinDirs without adding duplicates
 	void addSearchSkinDir(const std::string& skindir);
 
+	// Internal to findSkinnedFilenames()
+	template <typename FUNCTION>
+	void walkSearchSkinDirs(const std::string& subdir,
+							const std::vector<std::string>& subsubdirs,
+							const std::string& filename,
+							const FUNCTION& function) const;
+
 	std::string mAppName;               // install directory under progams/ ie "SecondLife"   
 	std::string mExecutablePathAndName; // full path + Filename of .exe
 	std::string mExecutableFilename;    // Filename of .exe
diff --git a/indra/llvfs/tests/lldir_test.cpp b/indra/llvfs/tests/lldir_test.cpp
index 15a5b6d4f3..323f876c12 100644
--- a/indra/llvfs/tests/lldir_test.cpp
+++ b/indra/llvfs/tests/lldir_test.cpp
@@ -73,35 +73,41 @@ struct LLDir_Dummy: public LLDir
         // the real one.
         static const char* preload[] =
         {
+            // We group these fixture-data pathnames by basename, rather than
+            // sorting by full path as you might expect, because the outcome
+            // of each test strongly depends on which skins/languages provide
+            // a given basename.
             "install/skins/default/colors.xml",
+            "install/skins/steam/colors.xml",
+            "user/skins/default/colors.xml",
+            "user/skins/steam/colors.xml",
+
             "install/skins/default/xui/en/strings.xml",
             "install/skins/default/xui/fr/strings.xml",
+            "install/skins/steam/xui/en/strings.xml",
+            "install/skins/steam/xui/fr/strings.xml",
+            "user/skins/default/xui/en/strings.xml",
+            "user/skins/default/xui/fr/strings.xml",
+            "user/skins/steam/xui/en/strings.xml",
+            "user/skins/steam/xui/fr/strings.xml",
+
             "install/skins/default/xui/en/floater.xml",
             "install/skins/default/xui/fr/floater.xml",
+            "user/skins/default/xui/fr/floater.xml",
+
             "install/skins/default/xui/en/newfile.xml",
             "install/skins/default/xui/fr/newfile.xml",
+            "user/skins/default/xui/en/newfile.xml",
+
             "install/skins/default/html/en-us/welcome.html",
             "install/skins/default/html/fr/welcome.html",
+
             "install/skins/default/textures/only_default.jpeg",
-            "install/skins/default/future/somefile.txt",
-            "install/skins/steam/colors.xml",
-            "install/skins/steam/xui/en/strings.xml",
-            "install/skins/steam/xui/fr/strings.xml",
             "install/skins/steam/textures/only_steam.jpeg",
-            "user/skins/default/colors.xml",
-            "user/skins/default/xui/en/strings.xml",
-            "user/skins/default/xui/fr/strings.xml",
-            // This is an attempted override that doesn't work: for a
-            // localized subdir, a skin must have subdir/en/filename as well
-            // as subdir/language/filename.
-            "user/skins/default/xui/fr/floater.xml",
-            // This is an override that only specifies the "en" version
-            "user/skins/default/xui/en/newfile.xml",
             "user/skins/default/textures/only_user_default.jpeg",
-            "user/skins/steam/colors.xml",
-            "user/skins/steam/xui/en/strings.xml",
-            "user/skins/steam/xui/fr/strings.xml",
-            "user/skins/steam/textures/only_user_steam.jpeg"
+            "user/skins/steam/textures/only_user_steam.jpeg",
+
+            "install/skins/default/future/somefile.txt"
         };
         BOOST_FOREACH(const char* path, preload)
         {
@@ -594,7 +600,7 @@ namespace tut
         ensure_equals(lldir.findSkinnedFilenames(LLDir::TEXTURES, "only_default.jpeg"),
                       vec(list_of("install/skins/default/textures/only_default.jpeg")));
         // Nor should we have needed to check skins/default/textures/en
-        // because textures is known to be unlocalized.
+        // because textures is known not to be localized.
         lldir.ensure_not_checked("install/skins/default/textures/en");
 
         StringVec expected(vec(list_of("install/skins/default/xui/en/strings.xml")
@@ -661,20 +667,21 @@ namespace tut
                           ("user/skins/default/xui/en/strings.xml")
                           ("user/skins/default/xui/fr/strings.xml")));
 
-        // The most specific skin for our dummy floater.xml is the installed
-        // default. Although we have a user xui/fr/floater.xml, we would also
-        // need a xui/en/floater.xml file to consider the user skin for this.
+        // Our dummy floater.xml has a user localization (for "fr") but no
+        // English override. This is a case in which CURRENT_SKIN nonetheless
+        // returns paths from two different skins.
         ensure_equals(lldir.findSkinnedFilenames(LLDir::XUI, "floater.xml"),
                       vec(list_of
                           ("install/skins/default/xui/en/floater.xml")
-                          ("install/skins/default/xui/fr/floater.xml")));
+                          ("user/skins/default/xui/fr/floater.xml")));
 
-        // The user override for the default skin does define newfile.xml, but
-        // only an "en" version, not a "fr" version as well. Nonetheless
-        // that's the most specific skin we have, regardless of the existence
-        // of a "fr" version in the installed default skin.
+        // Our dummy newfile.xml has an English override but no user
+        // localization. This is another case in which CURRENT_SKIN
+        // nonetheless returns paths from two different skins.
         ensure_equals(lldir.findSkinnedFilenames(LLDir::XUI, "newfile.xml"),
-                      vec(list_of("user/skins/default/xui/en/newfile.xml")));
+                      vec(list_of
+                          ("user/skins/default/xui/en/newfile.xml")
+                          ("install/skins/default/xui/fr/newfile.xml")));
 
         ensure_equals(lldir.findSkinnedFilenames("html", "welcome.html"),
                       vec(list_of
@@ -683,8 +690,8 @@ namespace tut
 
         /*------------------------ "default", "zh" -------------------------*/
         lldir.setSkinFolder("default", "zh");
-        // Because the user default skins strings.xml has only a "fr" override
-        // but not a "zh" override, the most localized version we can find is "en".
+        // Because strings.xml has only a "fr" override but no "zh" override
+        // in any skin, the most localized version we can find is "en".
         ensure_equals(lldir.findSkinnedFilenames(LLDir::XUI, "strings.xml"),
                       vec(list_of("user/skins/default/xui/en/strings.xml")));
 
@@ -710,11 +717,11 @@ namespace tut
         ensure_equals(lldir.findSkinnedFilenames(LLDir::TEXTURES, "only_user_steam.jpeg"),
                       vec(list_of("user/skins/steam/textures/only_user_steam.jpeg")));
 
-        // merge=false
+        // CURRENT_SKIN
         ensure_equals(lldir.findSkinnedFilenames(LLDir::XUI, "strings.xml"),
                       vec(list_of("user/skins/steam/xui/en/strings.xml")));
 
-        // pass merge=true to request this filename in all relevant skins
+        // pass constraint=ALL_SKINS to request this filename in all relevant skins
         ensure_equals(lldir.findSkinnedFilenames(LLDir::XUI, "strings.xml", LLDir::ALL_SKINS),
                       vec(list_of
                           ("install/skins/default/xui/en/strings.xml")
@@ -725,13 +732,13 @@ namespace tut
         /*------------------------- "steam", "fr" --------------------------*/
         lldir.setSkinFolder("steam", "fr");
 
-        // pass merge=true to request this filename in all relevant skins
+        // pass CURRENT_SKIN to request only the most specialized files
         ensure_equals(lldir.findSkinnedFilenames(LLDir::XUI, "strings.xml"),
                       vec(list_of
                           ("user/skins/steam/xui/en/strings.xml")
                           ("user/skins/steam/xui/fr/strings.xml")));
 
-        // pass merge=true to request this filename in all relevant skins
+        // pass ALL_SKINS to request this filename in all relevant skins
         ensure_equals(lldir.findSkinnedFilenames(LLDir::XUI, "strings.xml", LLDir::ALL_SKINS),
                       vec(list_of
                           ("install/skins/default/xui/en/strings.xml")