Fix new name regexes, some assumptions about MAPSEC_NONE, memory leak

huderlem · Oct 17, 2024 · 10aa6f6 · 10aa6f6
1 parent 728355d
commit 10aa6f6
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 49 deletions.
diff --git a/include/project.h b/include/project.h
@@ -132,7 +132,6 @@ class Project : public QObject
     QString getProjectTitle();
 
     QString readMapLayoutId(QString map_name);
-    QString readMapLayoutName(QString mapName);
     QString readMapLocation(QString map_name);
 
     bool readWildMonData();
@@ -243,6 +242,7 @@ class Project : public QObject
     static bool mapDimensionsValid(int width, int height);
     bool calculateDefaultMapSize();
     static int getMaxObjectEvents();
+    static QString getEmptyMapsecName();
 
 private:
     void updateLayout(Layout *);

diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
@@ -1370,9 +1370,13 @@ void MainWindow::mapListAddGroup() {
     QLineEdit *newNameEdit = new QLineEdit(&dialog);
     newNameEdit->setClearButtonEnabled(true);
 
-    static const QRegularExpression re_validChars("[_A-Za-z0-9]*$");
-    QRegularExpressionValidator *validator = new QRegularExpressionValidator(re_validChars);
-    newNameEdit->setValidator(validator);
+    static const QRegularExpression re_validChars("[A-Za-z_]+[\\w]*");
+    newNameEdit->setValidator(new QRegularExpressionValidator(re_validChars, newNameEdit));
+
+    connect(&newItemButtonBox, &QDialogButtonBox::accepted, [&](){
+        if (!this->editor->project->groupNames.contains(newNameEdit->text()))
+            dialog.accept();
+    });
 
     QFormLayout form(&dialog);
 
@@ -1397,10 +1401,10 @@ void MainWindow::mapListAddLayout() {
     QLineEdit *newNameEdit = new QLineEdit(&dialog);
     newNameEdit->setClearButtonEnabled(true);
 
-    static const QRegularExpression re_validChars("[_A-Za-z0-9]*$");
-    QRegularExpressionValidator *validator = new QRegularExpressionValidator(re_validChars);
-    newNameEdit->setValidator(validator);
+    static const QRegularExpression re_validChars("[A-Za-z_]+[\\w]*");
+    newNameEdit->setValidator(new QRegularExpressionValidator(re_validChars, newNameEdit));
 
+    // TODO: Support arbitrary LAYOUT_ ID names (Note from GriffinR: This is already handled in an unopened PR)
     QLabel *newId = new QLabel("LAYOUT_", &dialog);
     connect(newNameEdit, &QLineEdit::textChanged, [&](QString text){
         newId->setText(Layout::layoutConstantFromName(text.remove("_Layout")));
@@ -1499,7 +1503,7 @@ void MainWindow::mapListAddLayout() {
             layoutSettings.tileset_secondary_label = secondaryCombo->currentText();
         }
         Layout *newLayout = this->editor->project->createNewLayout(layoutSettings);
-        QStandardItem *item = this->layoutTreeModel->insertLayoutItem(newLayout->id);
+        this->layoutTreeModel->insertLayoutItem(newLayout->id);
         setLayout(newLayout->id);
     }
 }
@@ -1511,13 +1515,18 @@ void MainWindow::mapListAddArea() {
     QDialogButtonBox newItemButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel, Qt::Horizontal, &dialog);
     connect(&newItemButtonBox, &QDialogButtonBox::rejected, &dialog, &QDialog::reject);
 
+    const QString prefix = projectConfig.getIdentifier(ProjectIdentifier::define_map_section_prefix);
     QLineEdit *newNameEdit = new QLineEdit(&dialog);
-    newNameEdit->setText(projectConfig.getIdentifier(ProjectIdentifier::define_map_section_prefix));
-    newNameEdit->setClearButtonEnabled(false);
+    QLineEdit *newNameDisplay = new QLineEdit(&dialog);
+    newNameDisplay->setText(prefix);
+    newNameDisplay->setEnabled(false);
+    connect(newNameEdit, &QLineEdit::textEdited, [newNameDisplay, prefix] (const QString &text) {
+        // As the user types a name, update the label to show the name with the prefix.
+        newNameDisplay->setText(prefix + text);
+    });
 
-    QRegularExpression re_validChars(QString("%1[_A-Za-z0-9]+$").arg(projectConfig.getIdentifier(ProjectIdentifier::define_map_section_prefix)));
-    QRegularExpressionValidator *validator = new QRegularExpressionValidator(re_validChars);
-    newNameEdit->setValidator(validator);
+    static const QRegularExpression re_validChars("[A-Za-z_]+[\\w]*");
+    newNameEdit->setValidator(new QRegularExpressionValidator(re_validChars, newNameEdit));
 
     connect(&newItemButtonBox, &QDialogButtonBox::accepted, [&](){
         if (!this->editor->project->mapSectionNameToValue.contains(newNameEdit->text()))
@@ -1527,26 +1536,26 @@ void MainWindow::mapListAddArea() {
     QFormLayout form(&dialog);
 
     form.addRow("New Map Section Name", newNameEdit);
+    form.addRow("Constant Name", newNameDisplay);
     form.addRow(&newItemButtonBox);
 
     if (dialog.exec() == QDialog::Accepted) {
-        QString newFieldName = newNameEdit->text();
-        if (newFieldName.isEmpty()) return;
-        this->mapAreaModel->insertAreaItem(newFieldName);
+        if (newNameEdit->text().isEmpty()) return;
+        this->mapAreaModel->insertAreaItem(newNameDisplay->text());
     }
 }
 
 void MainWindow::mapListAddItem() {
     if (!this->editor || !this->editor->project) return;
 
     switch (this->ui->mapListContainer->currentIndex()) {
-    case 0:
+    case MapListTab::Groups:
         this->mapListAddGroup();
         break;
-    case 1:
+    case MapListTab::Areas:
         this->mapListAddArea();
         break;
-    case 2:
+    case MapListTab::Layouts:
         this->mapListAddLayout();
         break;
     }
@@ -1596,14 +1605,14 @@ void MainWindow::mapListRemoveItem() {
     if (!this->editor || !this->editor->project) return;
 
     switch (this->ui->mapListContainer->currentIndex()) {
-    case 0:
+    case MapListTab::Groups:
         this->mapListRemoveGroup();
         break;
-    case 1:
+    case MapListTab::Areas:
         // Disabled
         // this->mapListRemoveArea();
         break;
-    case 2:
+    case MapListTab::Layouts:
         // Disabled
         // this->mapListRemoveLayout();
         break;
@@ -2913,10 +2922,8 @@ void MainWindow::onTilesetsSaved(QString primaryTilesetLabel, QString secondaryT
     } else {
         this->editor->project->getTileset(secondaryTilesetLabel, true);
     }
-    if (updated) {
-        this->editor->layout->clearBorderCache();
+    if (updated)
         redrawMapScene();
-    }
 }
 
 void MainWindow::onMapRulerStatusChanged(const QString &status) {
@@ -3232,6 +3239,7 @@ void MainWindow::do_CollapseAll() {
     }
 }
 
+// TODO: Save this state in porymapConfig
 void MainWindow::do_HideShow() {
     switch (ui->mapListContainer->currentIndex()) {
     case MapListTab::Groups:
@@ -3265,6 +3273,7 @@ void MainWindow::on_toolButton_CollapseAll_Groups_clicked() {
     }
 }
 
+// TODO: Save this state in porymapConfig
 void MainWindow::on_toolButton_EnableDisable_EditGroups_clicked() {
     this->ui->mapList->clearSelection();
     if (this->ui->toolButton_EnableDisable_EditGroups->isChecked()) {
@@ -3607,6 +3616,7 @@ bool MainWindow::closeProject() {
         return true;
 
     // Check loaded maps for unsaved changes
+    // TODO: This needs to check for unsaved changes in layouts too.
     bool unsavedChanges = false;
     for (auto map : editor->project->mapCache.values()) {
         if (map && map->hasUnsavedChanges()) {

diff --git a/src/project.cpp b/src/project.cpp
@@ -412,10 +412,6 @@ QString Project::readMapLayoutId(QString map_name) {
     return ParseUtil::jsonToQString(mapObj["layout"]);
 }
 
-QString Project::readMapLayoutName(QString mapName) {
-    return this->layoutIdsToNames[readMapLayoutId(mapName)];
-}
-
 QString Project::readMapLocation(QString map_name) {
     if (mapCache.contains(map_name)) {
         return mapCache.value(map_name)->location;
@@ -537,6 +533,8 @@ bool Project::loadMapLayout(Map* map) {
 void Project::clearMapLayouts() {
     qDeleteAll(mapLayouts);
     mapLayouts.clear();
+    qDeleteAll(mapLayoutsMaster);
+    mapLayoutsMaster.clear();
     mapLayoutsTable.clear();
     layoutIdsToNames.clear();
 }
@@ -763,14 +761,16 @@ void Project::saveMapSections() {
 
     longestLength += 1;
 
-    // mapSectionValueToName
+    // TODO: Maybe print as an enum now that we can?
     for (int value : this->mapSectionValueToName.keys()) {
         QString line = QString("#define %1  0x%2\n")
             .arg(this->mapSectionValueToName[value], -1 * longestLength)
             .arg(QString("%1").arg(value, 2, 16, QLatin1Char('0')).toUpper());
         text += line;
     }
 
+    // TODO: We should maybe consider another way to update MAPSEC values in this file, in case we break anything by relocating it to the bottom of the file.
+    //       (or alternatively keep separate strings for text before/after the MAPSEC values)
     text += "\n" + this->extraFileText[projectConfig.getFilePath(ProjectFilePath::constants_region_map_sections)] + "\n";
 
     text += QString("#endif // GUARD_REGIONMAPSEC_H\n");
@@ -2307,10 +2307,12 @@ bool Project::readRegionMapSections() {
             continue;
         }
         // include guards
+        // TODO: Assuming guard name is the same across projects (it isn't)
         else if (currentLine.contains("GUARD_REGIONMAPSEC_H")) {
             continue;
         }
-        // defines captured (not considering comments)
+        // defines captured
+        // TODO: Regex to consider comments/extra space
         else if (currentLine.contains("#define " + projectConfig.getIdentifier(ProjectIdentifier::define_map_section_prefix))) {
             continue;
         }
@@ -2324,18 +2326,28 @@ bool Project::readRegionMapSections() {
     return true;
 }
 
+QString Project::getEmptyMapsecName() {
+    return projectConfig.getIdentifier(ProjectIdentifier::define_map_section_prefix) + projectConfig.getIdentifier(ProjectIdentifier::define_map_section_empty);
+}
+
+// This function assumes a valid and unique name.
+// Will return the new index.
+// TODO: We're not currently tracking map/layout agonstic changes like this as unsaved, so there's no warning if you close the project after doing this.
 int Project::appendMapsec(QString name) {
-    // This function assumes a valid and unique name.
-    // Will return the new index.
-    const QString emptyMapsecName = projectConfig.getIdentifier(ProjectIdentifier::define_map_section_prefix)
-                                    + projectConfig.getIdentifier(ProjectIdentifier::define_map_section_empty);
-
-    int noneBefore = this->mapSectionNameToValue[emptyMapsecName];
-    this->mapSectionNameToValue[name] = noneBefore;
-    this->mapSectionValueToName[noneBefore] = name;
-    this->mapSectionNameToValue[emptyMapsecName] = noneBefore + 1;
-    this->mapSectionValueToName[noneBefore + 1] = emptyMapsecName;
-    return noneBefore;
+    const QString emptyMapsecName = getEmptyMapsecName();
+    int newMapsecValue = mapSectionValueToName.isEmpty() ? 0 : mapSectionValueToName.lastKey();
+
+    // If the user has the 'empty' MAPSEC value defined last in the list we'll shift it so that it stays last in the list.
+    if (this->mapSectionNameToValue.contains(emptyMapsecName) && this->mapSectionNameToValue.value(emptyMapsecName) == newMapsecValue) {
+        this->mapSectionNameToValue.insert(emptyMapsecName, newMapsecValue + 1);
+        this->mapSectionValueToName.insert(newMapsecValue + 1, emptyMapsecName);
+    }
+
+    // TODO: Update 'define_map_section_count'?
+
+    this->mapSectionNameToValue[name] = newMapsecValue;
+    this->mapSectionValueToName[newMapsecValue] = name;
+    return newMapsecValue;
 }
 
 // Read the constants to preserve any "unused" heal locations when writing the file later

diff --git a/src/scriptapi/apiutility.cpp b/src/scriptapi/apiutility.cpp
@@ -154,9 +154,6 @@ void ScriptUtility::setMainTab(int index) {
     // Can't select tab if it's disabled
     if (!window->ui->mainTabBar->isTabEnabled(index))
         return;
-    // don't change tab when not editing a map
-    if (!window->editor || !window->editor->map)
-        return;
     window->on_mainTabBar_tabBarClicked(index);
 }
 

diff --git a/src/ui/maplistmodels.cpp b/src/ui/maplistmodels.cpp
@@ -19,10 +19,9 @@ void MapTree::removeSelected() {
 
 QWidget *GroupNameDelegate::createEditor(QWidget *parent, const QStyleOptionViewItem &, const QModelIndex &) const {
     QLineEdit *editor = new QLineEdit(parent);
-    static const QRegularExpression expression("[A-Za-z0-9_]+");
+    static const QRegularExpression expression("[A-Za-z_]+[\\w]*");
     editor->setPlaceholderText("gMapGroup_");
-    QRegularExpressionValidator *validator = new QRegularExpressionValidator(expression, parent);
-    editor->setValidator(validator);
+    editor->setValidator(new QRegularExpressionValidator(expression, parent));
     editor->setFrame(false);
     return editor;
 }
@@ -401,7 +400,12 @@ QStandardItem *MapAreaModel::insertAreaItem(QString areaName) {
     int newAreaIndex = this->project->appendMapsec(areaName);
     QStandardItem *item = createAreaItem(areaName, newAreaIndex);
     this->root->insertRow(newAreaIndex, item);
-    this->areaItems["MAPSEC_NONE"]->setData(newAreaIndex + 1, MapListUserRoles::GroupRole);
+
+    // MAPSEC_NONE may have shifted to accomodate the new item, update it in the list.
+    const QString emptyMapsecName = Project::getEmptyMapsecName();
+    if (this->areaItems.contains(emptyMapsecName))
+        this->areaItems[emptyMapsecName]->setData(this->project->mapSectionNameToValue.value(emptyMapsecName), MapListUserRoles::GroupRole);
+
     return item;
 }
 
@@ -427,6 +431,7 @@ void MapAreaModel::initialize() {
     this->mapItems.clear();
     this->setSortRole(MapListUserRoles::GroupRole);
 
+    // TODO: Ignore 'define_map_section_count' and/or 'define_map_section_empty'?
     for (int i : this->project->mapSectionNameToValue) {
         QString mapsecName = project->mapSectionValueToName.value(i);
         QStandardItem *areaItem = createAreaItem(mapsecName, i);