From 23f80c58dc8f3c51a731392c6ed413d047d931c6 Mon Sep 17 00:00:00 2001 From: Nordsoft91 Date: Sat, 8 Oct 2022 22:55:15 +0400 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Andrey Filipenkov --- CMakeLists.txt | 1 - CMakePresets.json | 2 +- mapeditor/Animation.h | 2 +- mapeditor/BitmapHandler.cpp | 16 +++++++--------- mapeditor/StdInc.h | 6 ++++-- mapeditor/inspector/rewardswidget.cpp | 2 +- mapeditor/main.cpp | 4 ++-- mapeditor/mainwindow.cpp | 13 +++++++------ mapeditor/mainwindow.h | 2 +- mapeditor/maphandler.cpp | 6 +++--- mapeditor/mapview.cpp | 2 +- mapeditor/playerparams.cpp | 6 ++---- mapeditor/validator.cpp | 2 +- mapeditor/windownewmap.cpp | 2 +- mapeditor/windownewmap.h | 2 +- 15 files changed, 33 insertions(+), 35 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c47faa212..094116fd1 100644 --- a/CMakeLists.txt +++ b/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() diff --git a/CMakePresets.json b/CMakePresets.json index e823ce700..2c6f7dd39 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -110,7 +110,7 @@ "CMAKE_SYSTEM_NAME": "iOS", "FORCE_BUNDLED_FL": "ON", "FORCE_BUNDLED_MINIZIP": "ON", - "ENABLE_EDITOR" : "OFF" + "ENABLE_EDITOR" : "OFF" } }, { diff --git a/mapeditor/Animation.h b/mapeditor/Animation.h index e4212e67c..cf6dcc0e2 100644 --- a/mapeditor/Animation.h +++ b/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 diff --git a/mapeditor/BitmapHandler.cpp b/mapeditor/BitmapHandler.cpp index c91ff0e93..976474094 100644 --- a/mapeditor/BitmapHandler.cpp +++ b/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); - if(image.isNull()) + for(const auto dir : {"DATA/", "SPRITES/"}) { - image = loadBitmapFromDir("SPRITES/", fname, setKey); - if(image.isNull()) - { - logGlobal->error("Error: Failed to find file %s", fname); - } + auto image = loadBitmapFromDir(dir, fname, setKey); + if(!image.isNull()) + return image; } - return image; + logGlobal->error("Error: Failed to find file %s", fname); + return {}; } } diff --git a/mapeditor/StdInc.h b/mapeditor/StdInc.h index 5343f0bcf..ff91472b9 100644 --- a/mapeditor/StdInc.h +++ b/mapeditor/StdInc.h @@ -19,7 +19,8 @@ template 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(_pointer); } @@ -27,7 +28,8 @@ template 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(_numeric); } diff --git a/mapeditor/inspector/rewardswidget.cpp b/mapeditor/inspector/rewardswidget.cpp index cde3ceb64..5a7389063 100644 --- a/mapeditor/inspector/rewardswidget.cpp +++ b/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) diff --git a/mapeditor/main.cpp b/mapeditor/main.cpp index 766cd9af9..1d44c5f72 100644 --- a/mapeditor/main.cpp +++ b/mapeditor/main.cpp @@ -13,7 +13,7 @@ int main(int argc, char * argv[]) { - QApplication vcmieditor(argc, argv); + QApplication vcmieditor(argc, argv); MainWindow mainWindow; - return vcmieditor.exec(); + return vcmieditor.exec(); } diff --git a/mapeditor/mainwindow.cpp b/mapeditor/mainwindow.cpp index 4f2ae37a1..45cbe0510 100644 --- a/mapeditor/mainwindow.cpp +++ b/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(); @@ -180,7 +180,7 @@ MainWindow::MainWindow(QWidget *parent) : MainWindow::~MainWindow() { saveUserSettings(); //save window size etc. - delete ui; + delete ui; } bool MainWindow::getAnswerAboutUnsavedChanges() @@ -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)")); - - if(filenameSelect.isNull()) + auto filenameSelect = QFileDialog::getOpenFileName(this, tr("Open map"), + QString::fromStdString(VCMIDirs::get().userCachePath().make_preferred().string()), + 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); diff --git a/mapeditor/mainwindow.h b/mapeditor/mainwindow.h index e12b163fb..7b4bc936d 100644 --- a/mapeditor/mainwindow.h +++ b/mapeditor/mainwindow.h @@ -45,7 +45,7 @@ public: MapController controller; private slots: - void on_actionOpen_triggered(); + void on_actionOpen_triggered(); void on_actionSave_as_triggered(); diff --git a/mapeditor/maphandler.cpp b/mapeditor/maphandler.cpp index 2e57b921c..398eb4b75 100644 --- a/mapeditor/maphandler.cpp +++ b/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()) diff --git a/mapeditor/mapview.cpp b/mapeditor/mapview.cpp index 2881ac000..0e5cc09bb 100644 --- a/mapeditor/mapview.cpp +++ b/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(); diff --git a/mapeditor/playerparams.cpp b/mapeditor/playerparams.cpp index 9b23d0ec3..206aa584c 100644 --- a/mapeditor/playerparams.cpp +++ b/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)"; - if(ctown->faction) - name = town->getObjectName(); + const auto name = ctown->faction ? town->getObjectName() : town->name + ", (random)"; 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(); } diff --git a/mapeditor/validator.cpp b/mapeditor/validator.cpp index df0b87a3e..891a06268 100644 --- a/mapeditor/validator.cpp +++ b/mapeditor/validator.cpp @@ -88,7 +88,7 @@ std::list 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()]; } diff --git a/mapeditor/windownewmap.cpp b/mapeditor/windownewmap.cpp index 5f8ccbcbd..4730b57b9 100644 --- a/mapeditor/windownewmap.cpp +++ b/mapeditor/windownewmap.cpp @@ -193,7 +193,7 @@ std::unique_ptr 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; diff --git a/mapeditor/windownewmap.h b/mapeditor/windownewmap.h index bd780727d..dc28c0a5e 100644 --- a/mapeditor/windownewmap.h +++ b/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);