Code review followup with additional refactor

Changes following review

mapeditor/graphics.cpp

@@ -48,7 +48,6 @@ void Graphics::loadPaletteAndColors()
 	for(int i = 0; i < 256; ++i)
 	{
 		QColor col;
-		// Cast to unsigned char to ensure values are in the range 0-255
 		col.setRed(std::clamp(static_cast<int>(pals[startPoint++]), 0, 255));
 		col.setGreen(std::clamp(static_cast<int>(pals[startPoint++]), 0, 255));
 		col.setBlue(std::clamp(static_cast<int>(pals[startPoint++]), 0, 255));

mapeditor/mapcontroller.cpp

@@ -381,9 +381,15 @@ void MapController::pasteFromClipboard(int level)
 	if(_clipboardShiftIndex == int3::getDirs().size())
 		_clipboardShiftIndex = 0;
 	
+	QStringList errors;
 	for(auto & objUniquePtr : _clipboard)
 	{
 		auto * obj = CMemorySerializer::deepCopy(*objUniquePtr).release();
+		QString errorMsg;
+		if (!canPlaceObject(level, obj, errorMsg))
+		{
+			errors.push_back(std::move(errorMsg));
+		}
 		auto newPos = objUniquePtr->pos + shift;
 		if(_map->isInTheMap(newPos))
 			obj->pos = newPos;
@@ -394,6 +400,8 @@ void MapController::pasteFromClipboard(int level)
 		_scenes[level]->selectionObjectsView.selectObject(obj);
 		_mapHandler->invalidate(obj);
 	}
+
+	QMessageBox::warning(main, QObject::tr("Can't place object"), errors.join('\n'));
 	
 	_scenes[level]->objectsView.draw();
 	_scenes[level]->passabilityView.update();
@@ -565,13 +573,13 @@ bool MapController::canPlaceObject(int level, CGObjectInstance * newObj, QString
 	{
 		auto typeName = QString::fromStdString(newObj->typeName);
 		auto subTypeName = QString::fromStdString(newObj->subTypeName);
-		error = QString("There can be only one grail object on the map");
+		error = QObject::tr("There can only be one grail object on the map.");
 		return false; //maplimit reached
 	}
 	
 	if(defaultPlayer == PlayerColor::NEUTRAL && (newObj->ID == Obj::HERO || newObj->ID == Obj::RANDOM_HERO))
 	{
-		error = "Hero cannot be created as NEUTRAL";
+		error = QObject::tr("Hero %1 cannot be created as NEUTRAL.").arg(QString::fromStdString(newObj->instanceName));
 		return false;
 	}
 	

mapeditor/validator.cpp

@@ -43,13 +43,13 @@ Validator::~Validator()
 	delete ui;
 }
 
-std::list<Validator::Issue> Validator::validate(const CMap * map)
+std::set<Validator::Issue> Validator::validate(const CMap * map)
 {
-	std::list<Validator::Issue> issues;
+	std::set<Validator::Issue> issues;
 	
 	if(!map)
 	{
-		issues.emplace_back(tr("Map is not loaded"), true);
+		issues.insert({ tr("Map is not loaded"), true });
 		return issues;
 	}
 	
@@ -64,21 +64,23 @@ std::list<Validator::Issue> Validator::validate(const CMap * map)
 		for(int i = 0; i < map->players.size(); ++i)
 		{
 			auto & p = map->players[i];
+			if (p.canAnyonePlay())
+				amountOfTowns[PlayerColor(i)] = 0;
 			if(p.canComputerPlay)
 				++cplayers;
 			if(p.canHumanPlay)
 				++hplayers;
 			if(p.allowedFactions.empty())
-				issues.emplace_back(QString(tr("No factions allowed for player %1")).arg(i), true);
+				issues.insert({ tr("No factions allowed for player %1").arg(i), true });
 		}
 		if(hplayers + cplayers == 0)
-			issues.emplace_back(tr("No players allowed to play this map"), true);
+			issues.insert({ tr("No players allowed to play this map"), true });
 		if(hplayers + cplayers == 1)
-			issues.emplace_back(tr("Map is allowed for one player and cannot be started"), true);
+			issues.insert({ tr("Map is allowed for one player and cannot be started"), true });
 		if(!hplayers)
-			issues.emplace_back(tr("No human players allowed to play this map"), true);
+			issues.insert({ tr("No human players allowed to play this map"), true });
 
-		std::set<const CHero*> allHeroesOnMap; //used to find hero duplicated
+		std::set<const CHero * > allHeroesOnMap; //used to find hero duplicated
 		
 		//checking all objects in the map
 		for(auto o : map->objects)
@@ -86,102 +88,111 @@ std::list<Validator::Issue> Validator::validate(const CMap * map)
 			//owners for objects
 			if(o->getOwner() == PlayerColor::UNFLAGGABLE)
 			{
-				if(dynamic_cast<CGMine*>(o.get()) ||
-				   dynamic_cast<CGDwelling*>(o.get()) ||
-				   dynamic_cast<CGTownInstance*>(o.get()) ||
-				   dynamic_cast<CGGarrison*>(o.get()) ||
-				   dynamic_cast<CGHeroInstance*>(o.get()))
+				if(dynamic_cast<CGMine *>(o.get()) ||
+				   dynamic_cast<CGDwelling *>(o.get()) ||
+				   dynamic_cast<CGTownInstance *>(o.get()) ||
+				   dynamic_cast<CGGarrison *>(o.get()) ||
+				   dynamic_cast<CGHeroInstance *>(o.get()))
 				{
-					issues.emplace_back(QString(tr("Armored instance %1 is UNFLAGGABLE but must have NEUTRAL or player owner")).arg(o->instanceName.c_str()), true);
+					issues.insert({ tr("Armored instance %1 is UNFLAGGABLE but must have NEUTRAL or player owner").arg(o->instanceName.c_str()), true });
 				}
 			}
 			if(o->getOwner() != PlayerColor::NEUTRAL && o->getOwner().getNum() < map->players.size())
 			{
 				if(!map->players[o->getOwner().getNum()].canAnyonePlay())
-					issues.emplace_back(QString(tr("Object %1 is assigned to non-playable player %2")).arg(o->instanceName.c_str(), o->getOwner().toString().c_str()), true);
+					issues.insert({ tr("Object %1 is assigned to non-playable player %2").arg(o->instanceName.c_str(), o->getOwner().toString().c_str()), true });
 			}
 			//count towns
-			if(auto * ins = dynamic_cast<CGTownInstance*>(o.get()))
+			if(auto * ins = dynamic_cast<CGTownInstance *>(o.get()))
 			{
 					++amountOfTowns[ins->getOwner()];
 			}
 			//checking and counting heroes and prisons
-			if(auto * ins = dynamic_cast<CGHeroInstance*>(o.get()))
+			if(auto * ins = dynamic_cast<CGHeroInstance *>(o.get()))
 			{
 				if(ins->ID == Obj::PRISON)
 				{
 					if(ins->getOwner() != PlayerColor::NEUTRAL)
-						issues.emplace_back(QString(tr("Prison %1 must be a NEUTRAL")).arg(ins->instanceName.c_str()), true);
+						issues.insert({ tr("Prison %1 must be a NEUTRAL").arg(ins->instanceName.c_str()), true });
 				}
 				else
 				{
 					if(ins->getOwner() == PlayerColor::NEUTRAL)
-						issues.emplace_back(QString(tr("Hero %1 must have an owner")).arg(ins->instanceName.c_str()), true);
+						issues.insert({ tr("Hero %1 must have an owner").arg(ins->instanceName.c_str()), true });
 
 					++amountOfHeroes[ins->getOwner()];
 				}
 				if(ins->type)
 				{
 					if(map->allowedHeroes.count(ins->getHeroType()) == 0)
-						issues.emplace_back(QString(tr("Hero %1 is prohibited by map settings")).arg(ins->type->getNameTranslated().c_str()), false);
+						issues.insert({ tr("Hero %1 is prohibited by map settings").arg(ins->type->getNameTranslated().c_str()), false });
 					
 					if(!allHeroesOnMap.insert(ins->type).second)
-						issues.emplace_back(QString(tr("Hero %1 has duplicate on map")).arg(ins->type->getNameTranslated().c_str()), false);
+						issues.insert({ tr("Hero %1 has duplicate on map").arg(ins->type->getNameTranslated().c_str()), false });
 				}
 				else if(ins->ID != Obj::RANDOM_HERO)
-					issues.emplace_back(QString(tr("Hero %1 has an empty type and must be removed")).arg(ins->instanceName.c_str()), true);
+					issues.insert({ tr("Hero %1 has an empty type and must be removed").arg(ins->instanceName.c_str()), true });
 			}
 			
 			//checking for arts
-			if(auto * ins = dynamic_cast<CGArtifact*>(o.get()))
+			if(auto * ins = dynamic_cast<CGArtifact *>(o.get()))
 			{
 				if(ins->ID == Obj::SPELL_SCROLL)
 				{
-					if(ins->storedArtifact)
+					if (ins->storedArtifact)
 					{
-						if(map->allowedSpells.count(ins->storedArtifact->getScrollSpellID()) == 0)
-							issues.emplace_back(QString(tr("Spell scroll %1 is prohibited by map settings")).arg(ins->storedArtifact->getScrollSpellID().toEntity(VLC->spells())->getNameTranslated().c_str()), false);
+						if (map->allowedSpells.count(ins->storedArtifact->getScrollSpellID()) == 0)
+							issues.insert({ tr("Spell scroll %1 is prohibited by map settings").arg(ins->storedArtifact->getScrollSpellID().toEntity(VLC->spells())->getNameTranslated().c_str()), false });
 					}
 					else
-						issues.emplace_back(QString(tr("Spell scroll %1 doesn't have instance assigned and must be removed")).arg(ins->instanceName.c_str()), true);
+						issues.insert({ tr("Spell scroll % 1 doesn't have instance assigned and must be removed").arg(ins->instanceName.c_str()), true });
 				}
 				else
 				{
 					if(ins->ID == Obj::ARTIFACT && map->allowedArtifact.count(ins->getArtifact()) == 0)
 					{
-						issues.emplace_back(QString(tr("Artifact %1 is prohibited by map settings")).arg(ins->getObjectName().c_str()), false);
+						issues.insert({ tr("Artifact % 1 is prohibited by map settings").arg(ins->getObjectName().c_str()), false });
 					}
 				}
 			}
 		}
 
 		//verification of starting towns
-		for(auto & mp : amountOfTowns)
-			if(mp.second == 0)
-				issues.emplace_back(QString(tr("Player %1 doesn't have any starting town")).arg(mp.first), false);
+		for (const auto & [player, counter] : amountOfTowns)
+		{
+			if (counter == 0)
+			{
+				// FIXME: heroesNames are empty even though heroes are on the map
+				// if(map->players[playerTCounter.first].heroesNames.empty())
+				if(amountOfHeroes.count(player) == 0)
+					issues.insert({ tr("Player %1 has no towns and heroes assigned").arg(player + 1), true });
+				else
+					issues.insert({ tr("Player %1 doesn't have any starting town").arg(player + 1), false });
+			}
+		}
 
 		//verification of map name and description
 		if(map->name.empty())
-			issues.emplace_back(tr("Map name is not specified"), false);
+			issues.insert({ tr("Map name is not specified"), false });
 		if(map->description.empty())
-			issues.emplace_back(tr("Map description is not specified"), false);
+			issues.insert({ tr("Map description is not specified"), false });
 		
 		//verificationfor mods
 		for(auto & mod : MapController::modAssessmentMap(*map))
 		{
 			if(!map->mods.count(mod.first))
 			{
-				issues.emplace_back(QString(tr("Map contains object from mod \"%1\", but doesn't require it")).arg(QString::fromStdString(VLC->modh->getModInfo(mod.first).getVerificationInfo().name)), true);
+				issues.insert({ tr("Map contains object from mod \"%1\", but doesn't require it").arg(QString::fromStdString(VLC->modh->getModInfo(mod.first).getVerificationInfo().name)), true });
 			}
 		}
 	}
 	catch(const std::exception & e)
 	{
-		issues.emplace_back(QString(tr("Exception occurs during validation: %1")).arg(e.what()), true);
+		issues.insert({ tr("Exception occurs during validation: %1").arg(e.what()), true });
 	}
 	catch(...)
 	{
-		issues.emplace_back(tr("Unknown exception occurs during validation"), true);
+		issues.insert({ tr("Unknown exception occurs during validation"), true });
 	}
 	
 	return issues;

mapeditor/validator.h

@@ -11,6 +11,7 @@
 #pragma once
 
 #include <QDialog>
+#include <set>
 
 VCMI_LIB_NAMESPACE_BEGIN
 class CMap;
@@ -30,13 +31,18 @@ public:
 		bool critical;
 		
 		Issue(const QString & m, bool c): message(m), critical(c) {}
+
+		bool operator <(const Issue & other) const
+		{
+			return message < other.message;
+		}
 	};
 	
 public:
 	explicit Validator(const CMap * map, QWidget *parent = nullptr);
 	~Validator();
 	
-	static std::list<Issue> validate(const CMap * map);
+	static std::set<Issue> validate(const CMap * map);
 
 private:
 	Ui::Validator *ui;