refactor(internal): Move logging facility to its own class (#7125)

Co-authored-by: warp-core <[email protected]> Co-authored-by: Nick <[email protected]>
thomasballinger · Aug 14, 2022 · e8dbed8 · e8dbed8
1 parent f6f155a
commit e8dbed8
Show file tree

Hide file tree

Showing 31 changed files with 195 additions and 105 deletions.
diff --git a/.gitignore b/.gitignore
@@ -9,9 +9,6 @@ libendless-sky.a
 # dlls needed to run the binary
 *.dll
 
-# Files::LogError output from unit tests
-errors.txt
-
 # Build Directories and toolchain-generated files
 build/
 obj/

diff --git a/EndlessSky.xcodeproj/project.pbxproj b/EndlessSky.xcodeproj/project.pbxproj
@@ -167,6 +167,7 @@
 		C4264774A89C0001B6FFC60E /* Hazard.cpp in Sources */ = {isa = PBXBuildFile; fileRef = C49D4EA08DF168A83B1C7B07 /* Hazard.cpp */; };
 		C7354A3E9C53D6C5E3CC352F /* TestData.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 02D34A71AE3BC4C93FC6865B /* TestData.cpp */; };
 		CE3F49A88B6DCBE09A711110 /* opengl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = A3754152958FBC924E9F76A9 /* opengl.cpp */; };
+		DAC24E909BF453D18AEE3DA3 /* Logger.cpp in Sources */ = {isa = PBXBuildFile; fileRef = A7F240B4AC219841424A387A /* Logger.cpp */; };
 		DF8D57E11FC25842001525DA /* Dictionary.cpp in Sources */ = {isa = PBXBuildFile; fileRef = DF8D57DF1FC25842001525DA /* Dictionary.cpp */; };
 		DF8D57E51FC25889001525DA /* Visual.cpp in Sources */ = {isa = PBXBuildFile; fileRef = DF8D57E21FC25889001525DA /* Visual.cpp */; };
 		DFAAE2A61FD4A25C0072C0A8 /* BatchDrawList.cpp in Sources */ = {isa = PBXBuildFile; fileRef = DFAAE2A21FD4A25C0072C0A8 /* BatchDrawList.cpp */; };
@@ -272,6 +273,7 @@
 		A3134EC4B1CCA5546A10C3EF /* TestContext.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = TestContext.cpp; path = source/TestContext.cpp; sourceTree = "<group>"; };
 		A3754152958FBC924E9F76A9 /* opengl.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = opengl.cpp; path = source/opengl.cpp; sourceTree = "<group>"; };
 		A7E640C7A366679B27CCADAC /* GameLoadingPanel.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = GameLoadingPanel.cpp; path = source/GameLoadingPanel.cpp; sourceTree = "<group>"; };
+		A7F240B4AC219841424A387A /* Logger.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = Logger.cpp; path = source/Logger.cpp; sourceTree = "<group>"; };
 		A90633FD1EE602FD000DA6C0 /* LogbookPanel.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = LogbookPanel.cpp; path = source/LogbookPanel.cpp; sourceTree = "<group>"; };
 		A90633FE1EE602FD000DA6C0 /* LogbookPanel.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = LogbookPanel.h; path = source/LogbookPanel.h; sourceTree = "<group>"; };
 		A90C15D71D5BD55700708F3A /* Minable.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = Minable.cpp; path = source/Minable.cpp; sourceTree = "<group>"; };
@@ -517,6 +519,7 @@
 		A9CC52701950C9F6004E4E22 /* CoreData.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = CoreData.framework; path = System/Library/Frameworks/CoreData.framework; sourceTree = SDKROOT; };
 		A9CC52711950C9F6004E4E22 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = System/Library/Frameworks/Foundation.framework; sourceTree = SDKROOT; };
 		A9D40D19195DFAA60086EE52 /* OpenGL.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = OpenGL.framework; path = System/Library/Frameworks/OpenGL.framework; sourceTree = SDKROOT; };
+		AE57401AA43232EDEABFAE13 /* Logger.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = Logger.h; path = source/Logger.h; sourceTree = "<group>"; };
 		B55C239B2303CE8A005C1A14 /* GameWindow.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = GameWindow.cpp; path = source/GameWindow.cpp; sourceTree = "<group>"; };
 		B55C239C2303CE8A005C1A14 /* GameWindow.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = GameWindow.h; path = source/GameWindow.h; sourceTree = "<group>"; };
 		B590161121ED4A0E00799178 /* Utf8.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = Utf8.cpp; path = source/text/Utf8.cpp; sourceTree = "<group>"; };
@@ -878,6 +881,8 @@
 				61FA4C2BB89E08C5E2B8B4B9 /* Variant.cpp */,
 				5EBC44769E84CF0C953D08B3 /* Variant.h */,
 				086E48E490C9BAD6660C7274 /* ExclusiveItem.h */,
+				A7F240B4AC219841424A387A /* Logger.cpp */,
+				AE57401AA43232EDEABFAE13 /* Logger.h */,
 			);
 			name = source;
 			sourceTree = "<group>";
@@ -1271,6 +1276,7 @@
 				F35E4D6EA465D71CDA282EBA /* MenuAnimationPanel.cpp in Sources */,
 				497849B2A4C5EA58A64D316C /* MapPlanetCard.cpp in Sources */,
 				920F40E0ADECA8926F423FDA /* Variant.cpp in Sources */,
+				DAC24E909BF453D18AEE3DA3 /* Logger.cpp in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

diff --git a/EndlessSkyLib.cbp b/EndlessSkyLib.cbp
@@ -191,6 +191,8 @@
 		<Unit filename="source/LoadPanel.h" />
 		<Unit filename="source/LocationFilter.cpp" />
 		<Unit filename="source/LocationFilter.h" />
+		<Unit filename="source/Logger.cpp" />
+		<Unit filename="source/Logger.h" />
 		<Unit filename="source/LogbookPanel.cpp" />
 		<Unit filename="source/LogbookPanel.h" />
 		<Unit filename="source/MainPanel.cpp" />

diff --git a/source/Armament.cpp b/source/Armament.cpp
@@ -12,8 +12,8 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 
 #include "Armament.h"
 
-#include "Files.h"
 #include "FireCommand.h"
+#include "Logger.h"
 #include "Outfit.h"
 #include "Ship.h"
 
@@ -56,7 +56,7 @@ int Armament::Add(const Outfit *outfit, int count)
 	// Do not equip weapons that do not define how they are mounted.
 	if(!isTurret && !outfit->Get("gun ports"))
 	{
-		Files::LogError("Error: Skipping unmountable outfit \"" + outfit->Name() + "\". Weapon outfits must specify either \"gun ports\" or \"turret mounts\".");
+		Logger::LogError("Error: Skipping unmountable outfit \"" + outfit->Name() + "\". Weapon outfits must specify either \"gun ports\" or \"turret mounts\".");
 		return 0;
 	}
 

diff --git a/source/Audio.cpp b/source/Audio.cpp
@@ -13,6 +13,7 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #include "Audio.h"
 
 #include "Files.h"
+#include "Logger.h"
 #include "Music.h"
 #include "Point.h"
 #include "Random.h"
@@ -185,13 +186,13 @@ void Audio::CheckReferences()
 {
 	if(!isInitialized)
 	{
-		Files::LogError("Warning: audio could not be initialized. No audio will play.");
+		Logger::LogError("Warning: audio could not be initialized. No audio will play.");
 		return;
 	}
 
 	for(auto &&it : sounds)
 		if(it.second.Name().empty())
-			Files::LogError("Warning: sound \"" + it.first + "\" is referred to, but does not exist.");
+			Logger::LogError("Warning: sound \"" + it.first + "\" is referred to, but does not exist.");
 }
 
 
@@ -604,7 +605,7 @@ namespace {
 
 			// Unlock the mutex for the time-intensive part of the loop.
 			if(!sound->Load(path, name))
-				Files::LogError("Unable to load sound \"" + name + "\" from path: " + path);
+				Logger::LogError("Unable to load sound \"" + name + "\" from path: " + path);
 		}
 	}
 }
diff --git a/source/CollisionSet.cpp b/source/CollisionSet.cpp
@@ -13,8 +13,8 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #include "CollisionSet.h"
 
 #include "Body.h"
-#include "Files.h"
 #include "Government.h"
+#include "Logger.h"
 #include "Mask.h"
 #include "Point.h"
 #include "Projectile.h"
@@ -208,7 +208,7 @@ Body *CollisionSet::Line(const Point &from, const Point &to, double *closestHit,
 		// Cap projectile velocity to prevent integer overflows.
 		if(!warned)
 		{
-			Files::LogError("Warning: maximum projectile velocity is " + to_string(MAX_VELOCITY));
+			Logger::LogError("Warning: maximum projectile velocity is " + to_string(MAX_VELOCITY));
 			warned = true;
 		}
 		Point newEnd = from + pVelocity.Unit() * USED_MAX_VELOCITY;

diff --git a/source/ConditionSet.cpp b/source/ConditionSet.cpp
@@ -14,7 +14,7 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 
 #include "DataNode.h"
 #include "DataWriter.h"
-#include "Files.h"
+#include "Logger.h"
 #include "Random.h"
 
 #include <algorithm>
@@ -205,7 +205,7 @@ namespace {
 		string message = "Error decomposing complex condition expression:\nFound:\t";
 		for(const string &str : side)
 			message += " \"" + str + "\"";
-		Files::LogError(message);
+		Logger::LogError(message);
 	}
 
 	bool IsUnrepresentable(const string &token)
@@ -745,7 +745,7 @@ void ConditionSet::Expression::SubExpression::GenerateSequence()
 			{
 				if(operators.at(opIndex) != ")")
 				{
-					Files::LogError("Did not find matched parentheses:");
+					Logger::LogError("Did not find matched parentheses:");
 					PrintConditionError(ToStrings());
 					tokens.clear();
 					operators.clear();
@@ -768,7 +768,7 @@ void ConditionSet::Expression::SubExpression::GenerateSequence()
 
 		if(operators.at(workingIndex) == "(" || operators.at(workingIndex) == ")")
 		{
-			Files::LogError("Mismatched parentheses:" + ToString());
+			Logger::LogError("Mismatched parentheses:" + ToString());
 			tokens.clear();
 			operators.clear();
 			sequence.clear();
@@ -794,7 +794,7 @@ bool ConditionSet::Expression::SubExpression::AddOperation(vector<int> &data, si
 	if((leftIndex < tokens.size() && tokens.at(leftIndex).empty())
 			|| (rightIndex < tokens.size() && tokens.at(rightIndex).empty()))
 	{
-		Files::LogError("Unable to obtain valid operand for function \"" + operators.at(opIndex) + "\" with tokens:");
+		Logger::LogError("Unable to obtain valid operand for function \"" + operators.at(opIndex) + "\" with tokens:");
 		PrintConditionError(tokens);
 		tokens.clear();
 		operators.clear();

diff --git a/source/DataNode.cpp b/source/DataNode.cpp
@@ -12,7 +12,7 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 
 #include "DataNode.h"
 
-#include "Files.h"
+#include "Logger.h"
 
 #include <algorithm>
 #include <cctype>
@@ -121,7 +121,7 @@ double DataNode::Value(const string &token)
 	// Allowed format: "[+-]?[0-9]*[.]?[0-9]*([eE][+-]?[0-9]*)?".
 	if(!IsNumber(token))
 	{
-		Files::LogError("Cannot convert value \"" + token + "\" to a number.");
+		Logger::LogError("Cannot convert value \"" + token + "\" to a number.");
 		return 0.;
 	}
 	const char *it = token.c_str();
@@ -246,7 +246,7 @@ list<DataNode>::const_iterator DataNode::end() const noexcept
 int DataNode::PrintTrace(const string &message) const
 {
 	if(!message.empty())
-		Files::LogError(message);
+		Logger::LogError(message);
 
 	// Recursively print all the parents of this node, so that the user can
 	// trace it back to the right point in the file.
@@ -271,11 +271,11 @@ int DataNode::PrintTrace(const string &message) const
 		if(hasSpace)
 			line += hasQuote ? '`' : '"';
 	}
-	Files::LogError(line);
+	Logger::LogError(line);
 
 	// Put an empty line in the log between each error message.
 	if(!message.empty())
-		Files::LogError("");
+		Logger::LogError("");
 
 	// Tell the caller what indentation level we're at now.
 	return indent;

diff --git a/source/Engine.cpp b/source/Engine.cpp
@@ -18,7 +18,6 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #include "DamageDealt.h"
 #include "DamageProfile.h"
 #include "Effect.h"
-#include "Files.h"
 #include "FillShader.h"
 #include "Fleet.h"
 #include "Flotsam.h"
@@ -30,6 +29,7 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #include "Government.h"
 #include "Hazard.h"
 #include "Interface.h"
+#include "Logger.h"
 #include "MapPanel.h"
 #include "Mask.h"
 #include "Messages.h"
@@ -324,7 +324,7 @@ void Engine::Place()
 		else if(!ship->GetSystem())
 		{
 			// Log this error.
-			Files::LogError("Engine::Place: Set fallback system for the NPC \"" + ship->Name() + "\" as it had no system");
+			Logger::LogError("Engine::Place: Set fallback system for the NPC \"" + ship->Name() + "\" as it had no system");
 			ship->SetSystem(player.GetSystem());
 		}
 

diff --git a/source/EsUuid.cpp b/source/EsUuid.cpp
@@ -12,7 +12,7 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 
 #include "EsUuid.h"
 
-#include "Files.h"
+#include "Logger.h"
 #if defined(_WIN32)
 #include "text/Utf8.h"
 #endif
@@ -39,7 +39,7 @@ EsUuid::UuidType MakeUuid()
 	EsUuid::UuidType value;
 	RPC_STATUS status = UuidCreate(&value.id);
 	if(status == RPC_S_UUID_LOCAL_ONLY)
-		Files::LogError("Created locally unique UUID only");
+		Logger::LogError("Created locally unique UUID only");
 	else if(status == RPC_S_UUID_NO_ADDRESS)
 		throw std::runtime_error("Failed to create UUID");
 
@@ -72,7 +72,7 @@ std::string Serialize(const UUID &id)
 
 	std::string result = (status == RPC_S_OK) ? Utf8::ToUTF8(buf) : "";
 	if(result.empty())
-		Files::LogError("Failed to serialize UUID!");
+		Logger::LogError("Failed to serialize UUID!");
 	else
 		RpcStringFreeW(reinterpret_cast<RPC_WSTR *>(&buf));
 
@@ -183,7 +183,7 @@ EsUuid::EsUuid(const std::string &input)
 	}
 	catch (const std::invalid_argument &err)
 	{
-		Files::LogError(err.what());
+		Logger::LogError(err.what());
 	}
 }
 

diff --git a/source/Files.cpp b/source/Files.cpp
@@ -13,6 +13,7 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #include "Files.h"
 
 #include "File.h"
+#include "Logger.h"
 
 #include <SDL2/SDL.h>
 
@@ -47,7 +48,6 @@ namespace {
 	string savePath;
 	string testPath;
 
-	mutex errorMutex;
 	File errorLog;
 
 	// Convert windows-style directory separators ('\\') to standard '/'.
@@ -434,14 +434,14 @@ void Files::Copy(const string &from, const string &to)
 	// Preserve the timestamps of the original file.
 	struct stat buf;
 	if(stat(from.c_str(), &buf))
-		LogError("Error: Cannot stat \"" + from + "\".");
+		Logger::LogError("Error: Cannot stat \"" + from + "\".");
 	else
 	{
 		struct utimbuf times;
 		times.actime = buf.st_atime;
 		times.modtime = buf.st_mtime;
 		if(utime(to.c_str(), &times))
-			LogError("Error: Failed to preserve the timestamps for \"" + to + "\".");
+			Logger::LogError("Error: Failed to preserve the timestamps for \"" + to + "\".");
 	}
 #endif
 }
@@ -545,10 +545,8 @@ void Files::Write(FILE *file, const string &data)
 
 
 
-void Files::LogError(const string &message)
+void Files::LogErrorToFile(const string &message)
 {
-	lock_guard<mutex> lock(errorMutex);
-	cerr << message << endl;
 	if(!errorLog)
 	{
 		errorLog = File(config + "errors.txt", true);

diff --git a/source/Files.h b/source/Files.h
@@ -66,7 +66,10 @@ class Files {
 	static void Write(const std::string &path, const std::string &data);
 	static void Write(FILE *file, const std::string &data);
 
-	static void LogError(const std::string &message);
+	// Logging to the error-log. Actual calls should be done through Logger
+	// and not directly here to ensure that other logging actions also
+	// happen (and to ensure thread safety on the logging).
+	static void LogErrorToFile(const std::string &message);
 };
 
 

diff --git a/source/Fleet.cpp b/source/Fleet.cpp
@@ -13,9 +13,9 @@ PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #include "Fleet.h"
 
 #include "DataNode.h"
-#include "Files.h"
 #include "GameData.h"
 #include "Government.h"
+#include "Logger.h"
 #include "Phrase.h"
 #include "pi.h"
 #include "Planet.h"
@@ -256,7 +256,7 @@ void Fleet::RemoveInvalidVariants()
 	if(!count)
 		return;
 
-	Files::LogError("Warning: " + (fleetName.empty() ? "unnamed fleet" : "fleet \"" + fleetName + "\"")
+	Logger::LogError("Warning: " + (fleetName.empty() ? "unnamed fleet" : "fleet \"" + fleetName + "\"")
 		+ ": Removing " + to_string(count) + " invalid " + (count > 1 ? "variants" : "variant")
 		+ " (" + to_string(total - variants.TotalWeight()) + " of " + to_string(total) + " weight)");
 }
@@ -379,7 +379,7 @@ void Fleet::Enter(const System &system, list<shared_ptr<Ship>> &ships, const Pla
 		if(!object)
 		{
 			// Log this error.
-			Files::LogError("Fleet::Enter: Unable to find valid stellar object for planet \""
+			Logger::LogError("Fleet::Enter: Unable to find valid stellar object for planet \""
 				+ planet->TrueName() + "\" in system \"" + system.Name() + "\"");
 			return;
 		}
@@ -560,7 +560,7 @@ vector<shared_ptr<Ship>> Fleet::Instantiate(const vector<const Ship *> &ships) c
 		// At least one of this variant's ships is valid, but we should avoid spawning any that are not defined.
 		if(!model->IsValid())
 		{
-			Files::LogError("Warning: Skipping invalid ship model \"" + model->ModelName() + "\" in fleet \"" + fleetName + "\".");
+			Logger::LogError("Warning: Skipping invalid ship model \"" + model->ModelName() + "\" in fleet \"" + fleetName + "\".");
 			continue;
 		}