Apply suggestions from code review

Co-authored-by: Andrey Filipenkov <decapitator@ukr.net>

CMakeLists.txt

@@ -62,7 +62,6 @@ option(ENABLE_ERM "Enable compilation of ERM scripting module" OFF)
 option(ENABLE_LUA "Enable compilation of LUA scripting module" OFF)
 option(ENABLE_LAUNCHER "Enable compilation of launcher" ON)
 option(ENABLE_EDITOR "Enable compilation of map editor" ON)
-option(ENABLE_TEST "Enable compilation of unit tests" ON)
 if(APPLE_IOS)
 	set(BUNDLE_IDENTIFIER_PREFIX "" CACHE STRING "Bundle identifier prefix")
 else()

mapeditor/Animation.h

@@ -60,7 +60,7 @@ public:
 
 	//duplicates frame at [sourceGroup, sourceFrame] as last frame in targetGroup
 	//and loads it if animation is preloaded
-	void duplicateImage(const size_t sourceGroup, const size_t sourceFrame, const size_t targetGroup);
+	void duplicateImage(size_t sourceGroup, size_t sourceFrame, size_t targetGroup);
 
 	// adjust the color of the animation, used in battle spell effects, e.g. Cloned objects
 

mapeditor/BitmapHandler.cpp

@@ -117,7 +117,7 @@ namespace BitmapHandler
 			return image;
 		}
 		else
-		{ //loading via SDL_Image
+		{ //loading via QImage
 			QImage image(QString::fromStdString(fullpath->make_preferred().string()));
 			if(!image.isNull())
 			{
@@ -145,15 +145,13 @@ namespace BitmapHandler
 
 	QImage loadBitmap(const std::string & fname, bool setKey)
 	{
-		QImage image = loadBitmapFromDir("DATA/", fname, setKey);
+		for(const auto dir : {"DATA/", "SPRITES/"})
-		if(image.isNull())
 		{
-			image = loadBitmapFromDir("SPRITES/", fname, setKey);
+			auto image = loadBitmapFromDir(dir, fname, setKey);
-			if(image.isNull())
+			if(!image.isNull())
-			{
-				logGlobal->error("Error: Failed to find file %s", fname);
-			}
-		}
 				return image;
 		}
+		logGlobal->error("Error: Failed to find file %s", fname);
+		return {};
+	}
 }

mapeditor/StdInc.h

@@ -19,7 +19,8 @@ template<class Type>
 NumericPointer data_cast(Type * _pointer)
 {
 	static_assert(sizeof(Type *) == sizeof(NumericPointer),
-				  "Compilied for 64 bit arcitecture. Use NumericPointer = unsigned int");
+				  "Compiled for 64 bit arcitecture. Use NumericPointer = unsigned int");
+
 	return reinterpret_cast<NumericPointer>(_pointer);
 }
 
@@ -27,7 +28,8 @@ template<class Type>
 Type * data_cast(NumericPointer _numeric)
 {
 	static_assert(sizeof(Type *) == sizeof(NumericPointer),
-				  "Compilied for 64 bit arcitecture. Use NumericPointer = unsigned int");
+				  "Compiled for 64 bit arcitecture. Use NumericPointer = unsigned int");
+
 	return reinterpret_cast<Type *>(_numeric);
 }
 

mapeditor/inspector/rewardswidget.cpp

@@ -212,7 +212,7 @@ bool RewardsWidget::commitChanges()
 
 void RewardsWidget::on_rewardList_activated(int index)
 {
-	ui->rewardAmount->setText(QString::number(1));
+	ui->rewardAmount->setText(QStringLiteral("1"));
 }
 
 void RewardsWidget::addReward(int typeId, int listId, int amount)

mapeditor/mainwindow.cpp

@@ -102,7 +102,7 @@ MainWindow::MainWindow(QWidget *parent) :
 	QDir::setCurrent(QApplication::applicationDirPath());
 
 	//configure logging
-	const boost::filesystem::path logPath = VCMIDirs::get().userCachePath() / "VCMI_Editor_log.txt";
+	const boost::filesystem::path logPath = VCMIDirs::get().userLogsPath() / "VCMI_Editor_log.txt";
 	console = new CConsoleHandler();
 	logConfig = new CBasicLogConfigurator(logPath, console);
 	logConfig->configureDefault();
@@ -282,9 +282,10 @@ void MainWindow::on_actionOpen_triggered()
 	if(!getAnswerAboutUnsavedChanges())
 		return;
 	
-	auto filenameSelect = QFileDialog::getOpenFileName(this, tr("Open Image"), QString::fromStdString(VCMIDirs::get().userCachePath().make_preferred().string()), tr("Homm3 Files (*.vmap *.h3m)"));
+	auto filenameSelect = QFileDialog::getOpenFileName(this, tr("Open map"),
-	
+		QString::fromStdString(VCMIDirs::get().userCachePath().make_preferred().string()),
-	if(filenameSelect.isNull())
+		tr("All supported maps (*.vmap *.h3m);;VCMI maps(*.vmap);;HoMM3 maps(*.h3m)"));
+	if(filenameSelect.isEmpty())
 		return;
 	
 	openMap(filenameSelect);
@@ -517,7 +518,7 @@ void MainWindow::loadObjectsTree()
 		throw std::runtime_error("object browser exists");
 
 	//model
-	objectsModel.setHorizontalHeaderLabels(QStringList() << QStringLiteral("Type"));
+	objectsModel.setHorizontalHeaderLabels(QStringList() << tr("Type"));
 	objectBrowser = new ObjectBrowser(this);
 	objectBrowser->setSourceModel(&objectsModel);
 	objectBrowser->setDynamicSortFilter(false);

mapeditor/maphandler.cpp

@@ -191,7 +191,7 @@ void MapHandler::initObjectRects()
 		auto image = animation->getImage(0, obj->ID == Obj::HERO ? 2 : 0);
 		if(!image)
 		{
-			//workaound for prisons
+			//workaround for prisons
 			image = animation->getImage(0, 0);
 			if(!image)
 				continue;
@@ -238,9 +238,9 @@ bool MapHandler::compareObjectBlitOrder(const CGObjectInstance * a, const CGObje
 	if(a->pos.y != b->pos.y)
 		return a->pos.y < b->pos.y;
 	
-	if(b->ID==Obj::HERO && a->ID!=Obj::HERO)
+	if(b->ID == Obj::HERO && a->ID != Obj::HERO)
 		return true;
-	if(b->ID!=Obj::HERO && a->ID==Obj::HERO)
+	if(b->ID != Obj::HERO && a->ID == Obj::HERO)
 		return false;
 	
 	if(!a->isVisitable() && b->isVisitable())

mapeditor/mapview.cpp

@@ -128,7 +128,7 @@ void MapView::mouseMoveEvent(QMouseEvent *mouseEvent)
 		break;
 
 	case MapView::SelectionTool::Area:
-		if(mouseEvent->buttons() & Qt::RightButton || !mouseEvent->buttons() & Qt::LeftButton)
+		if(mouseEvent->buttons() & Qt::RightButton || !(mouseEvent->buttons() & Qt::LeftButton))
 			break;
 
 		sc->selectionTerrainView.clear();

mapeditor/playerparams.cpp

@@ -54,9 +54,7 @@ PlayerParams::PlayerParams(MapController & ctrl, int playerId, QWidget *parent)
 			{
 				if(playerInfo.hasMainTown && playerInfo.posOfMainTown == town->pos)
 					foundMainTown = townIndex;
-				auto name = town->name + ", (random)";
+				const auto name = ctown->faction ? town->getObjectName() : town->name + ", (random)";
-				if(ctown->faction)
-					name = town->getObjectName();
 				ui->mainTown->addItem(tr(name.c_str()), QVariant::fromValue(i));
 				++townIndex;
 			}
@@ -75,7 +73,7 @@ PlayerParams::PlayerParams(MapController & ctrl, int playerId, QWidget *parent)
 		playerInfo.posOfMainTown = int3(-1, -1, -1);
 	}
 
-	ui->playerColor->setTitle(QString("PlayerID: %1").arg(playerId));
+	ui->playerColor->setTitle(tr("Player ID: %1").arg(playerId));
 	show();
 }
 

mapeditor/validator.cpp

@@ -88,7 +88,7 @@ std::list<Validator::Issue> Validator::validate(const CMap * map)
 			{
 				bool has = amountOfCastles.count(ins->getOwner().getNum());
 				if(!has && ins->getOwner() != PlayerColor::NEUTRAL)
-					issues.emplace_back(QString("Town %1 has undefined owner %s").arg(ins->instanceName.c_str(), ins->getOwner().getStr().c_str()), true);
+					issues.emplace_back(tr("Town %1 has undefined owner %2").arg(ins->instanceName.c_str(), ins->getOwner().getStr().c_str()), true);
 				if(has)
 					++amountOfCastles[ins->getOwner().getNum()];
 			}

mapeditor/windownewmap.cpp

@@ -193,7 +193,7 @@ std::unique_ptr<CMap> generateEmptyMap(CMapGenOptions & options)
 	return std::move(map);
 }
 
-void WindowNewMap::on_okButtong_clicked()
+void WindowNewMap::on_okButton_clicked()
 {
 	EWaterContent::EWaterContent water = EWaterContent::RANDOM;
 	EMonsterStrength::EMonsterStrength monster = EMonsterStrength::RANDOM;

mapeditor/windownewmap.h

@@ -57,7 +57,7 @@ public:
 private slots:
 	void on_cancelButton_clicked();
 
-	void on_okButtong_clicked();
+	void on_okButton_clicked();
 
 	void on_sizeCombo_activated(int index);