nat_linden avatar nat_linden committed 872e5ea

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.

Comments (0)

Files changed (3)

indra/llvfs/lldir.cpp

 #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;
 	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
 	// 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"
 		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
 		{
 			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;
 			}
 		}
 	}
 			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)
+	// The process we use depends on 'constraint'.
+	if (constraint != CURRENT_SKIN) // meaning ALL_SKINS
 	{
-		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)
+		// 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)
 		{
-			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)
-		{
-			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);
 			}
 		}
 	}

indra/llvfs/lldir.h

 	// 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

indra/llvfs/tests/lldir_test.cpp

         // 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)
         {
         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")
                           ("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
 
         /*------------------------ "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")));
 
         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")
         /*------------------------- "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")
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.