Final MapConnection testing fixes

include/core/map.h

@@ -99,9 +99,8 @@ public:
     void addEvent(Event *);
     void deleteConnections();
     QList<MapConnection*> getConnections() const;
-    bool takeConnection(MapConnection *);
+    void removeConnection(MapConnection *);
-    bool removeConnection(MapConnection *);
+    void addConnection(MapConnection *);
-    bool addConnection(MapConnection *);
     void loadConnection(MapConnection *);
     QRect getConnectionRect(const QString &direction, MapLayout *fromLayout = nullptr);
     QPixmap renderConnection(const QString &direction, MapLayout *fromLayout = nullptr);
@@ -133,6 +132,7 @@ public:
 private:
     void setNewDimensionsBlockdata(int newWidth, int newHeight);
     void setNewBorderDimensionsBlockdata(int newWidth, int newHeight);
+    void trackConnection(MapConnection*);
 
     // MapConnections in 'ownedConnections' but not 'connections' persist in the edit history.
     QList<MapConnection*> connections;

include/core/mapconnection.h

@@ -55,8 +55,8 @@ private:
 
 signals:
     void parentMapChanged(Map* before, Map* after);
-    void targetMapNameChanged(const QString &before, const QString &after);
+    void targetMapNameChanged(QString before, QString after);
-    void directionChanged(const QString &before, const QString &after);
+    void directionChanged(QString before, QString after);
     void offsetChanged(int before, int after);
 };
 

include/editor.h

@@ -78,7 +78,6 @@ public:
     void setMapEditingButtonsEnabled(bool enabled);
     void setConnectionsVisibility(bool visible);
     void updateDivingMapsVisibility();
-    void displayDivingConnections();
     void renderDivingConnections();
     void addConnection(MapConnection* connection);
     void removeConnection(MapConnection* connection);
@@ -185,13 +184,14 @@ private:
     void clearMapGrid();
     void clearWildMonTables();
     void updateBorderVisibility();
+    void disconnectMapConnection(MapConnection *connection);
     QPoint getConnectionOrigin(MapConnection *connection);
-    void removeConnectionPixmap(MapConnection* connection);
+    void removeConnectionPixmap(MapConnection *connection);
-    void updateConnectionPixmap(ConnectionPixmapItem* connectionItem);
+    void updateConnectionPixmap(ConnectionPixmapItem *connectionItem);
-    void displayConnection(MapConnection* connection);
+    void displayConnection(MapConnection *connection);
-    void displayDivingConnection(MapConnection* connection);
+    void displayDivingConnection(MapConnection *connection);
     void setDivingMapName(QString mapName, QString direction);
-    void removeDivingMapPixmap(MapConnection* connection);
+    void removeDivingMapPixmap(MapConnection *connection);
     void updateEncounterFields(EncounterFields newFields);
     QString getMovementPermissionText(uint16_t collision, uint16_t elevation);
     QString getMetatileDisplayMessage(uint16_t metatileId);
@@ -207,7 +207,7 @@ private slots:
     void setStraightPathCursorMode(QGraphicsSceneMouseEvent *event);
     void mouseEvent_map(QGraphicsSceneMouseEvent *event, MapPixmapItem *item);
     void mouseEvent_collision(QGraphicsSceneMouseEvent *event, CollisionPixmapItem *item);
-    void setSelectedConnectionItem(ConnectionPixmapItem* connectionItem);
+    void setSelectedConnectionItem(ConnectionPixmapItem *connectionItem);
     void onHoveredMovementPermissionChanged(uint16_t, uint16_t);
     void onHoveredMovementPermissionCleared();
     void onHoveredMetatileSelectionChanged(uint16_t);
@@ -219,7 +219,6 @@ private slots:
     void onSelectedMetatilesChanged();
     void onWheelZoom(int);
     void onToggleGridClicked(bool);
-    void onMapConnectionDoubleClicked(MapConnection*);
 
 signals:
     void objectsChanged();

include/ui/connectionslistitem.h

@@ -42,8 +42,8 @@ signals:
     void openMapClicked(MapConnection*);
 
 private slots:
-    void on_comboBox_Direction_currentTextChanged(const QString &direction);
+    void on_comboBox_Direction_currentTextChanged(QString direction);
-    void on_comboBox_Map_currentTextChanged(const QString &mapName);
+    void on_comboBox_Map_currentTextChanged(QString mapName);
     void on_spinBox_Offset_valueChanged(int offset);
     void on_button_Delete_clicked();
     void on_button_OpenMap_clicked();

include/ui/mapimageexporter.h

@@ -50,9 +50,10 @@ private:
     ImageExporterMode mode = ImageExporterMode::Normal;
 
     void updatePreview();
+    void updateShowBorderState();
     void saveImage();
     QPixmap getStitchedImage(QProgressDialog *progress, bool includeBorder);
-    QPixmap getFormattedMapPixmap(Map *map, bool ignoreBorder);
+    QPixmap getFormattedMapPixmap(Map *map, bool ignoreBorder = false);
     bool historyItemAppliesToFrame(const QUndoCommand *command);
 
 private slots:

src/core/map.cpp

@@ -537,12 +537,12 @@ void Map::deleteConnections() {
 }
 
 QList<MapConnection*> Map::getConnections() const {
-    return connections;
+    return this->connections;
 }
 
-bool Map::addConnection(MapConnection *connection) {
+void Map::addConnection(MapConnection *connection) {
     if (!connection || this->connections.contains(connection))
-        return false;
+        return;
 
     // Maps should only have one Dive/Emerge connection at a time.
     // (Users can technically have more by editing their data manually, but we will only display one at a time)
@@ -550,9 +550,8 @@ bool Map::addConnection(MapConnection *connection) {
     if (MapConnection::isDiving(connection->direction())) {
         for (auto i : this->connections) {
             if (i->direction() == connection->direction()) {
-                this->ownedConnections.insert(connection);
+                trackConnection(connection);
-                connection->setParentMap(this, false);
+                return;
-                return true;
             }
         }
     }
@@ -560,33 +559,41 @@ bool Map::addConnection(MapConnection *connection) {
     loadConnection(connection);
     modify();
     emit connectionAdded(connection);
-    return true;
+    return;
 }
 
 void Map::loadConnection(MapConnection *connection) {
     if (!connection)
         return;
-    this->connections.append(connection);
+
-    this->ownedConnections.insert(connection);
+    if (!this->connections.contains(connection))
-    connection->setParentMap(this, false);
+        this->connections.append(connection);
+
+    trackConnection(connection);
 }
 
-// connection should not be deleted here, a pointer to it is allowed to persist in the edit history
+void Map::trackConnection(MapConnection *connection) {
-bool Map::removeConnection(MapConnection *connection) {
+    connection->setParentMap(this, false);
+
+    if (!this->ownedConnections.contains(connection)) {
+        this->ownedConnections.insert(connection);
+        connect(connection, &MapConnection::parentMapChanged, [=](Map *, Map *after) {
+            if (after != this && after != nullptr) {
+                // MapConnection's parent has been reassigned, it's no longer our responsibility
+                this->ownedConnections.remove(connection);
+                QObject::disconnect(connection, &MapConnection::parentMapChanged, this, nullptr);
+            }
+        });
+    }
+}
+
+// We retain ownership of this MapConnection until it's assigned to a new parent map.
+void Map::removeConnection(MapConnection *connection) {
     if (!this->connections.removeOne(connection))
-        return false;
+        return;
+    connection->setParentMap(nullptr, false);
     modify();
     emit connectionRemoved(connection);
-    return true;
-}
-
-// Same as Map::removeConnection but caller takes ownership of connection.
-bool Map::takeConnection(MapConnection *connection) {
-    if (!this->removeConnection(connection))
-        return false;
-    connection->setParentMap(nullptr, false);
-    this->ownedConnections.remove(connection);
-    return true;
 }
 
 void Map::modify() {

src/core/mapconnection.cpp

@@ -79,7 +79,7 @@ void MapConnection::setParentMap(Map* map, bool mirror) {
     }
 
     if (m_parentMap)
-        m_parentMap->takeConnection(this);
+        m_parentMap->removeConnection(this);
 
     auto before = m_parentMap;
     m_parentMap = map;

src/editor.cpp

@@ -733,7 +733,15 @@ void Editor::updateEncounterFields(EncounterFields newFields) {
     project->wildMonFields = newFields;
 }
 
-void Editor::displayConnection(MapConnection* connection) {
+void Editor::disconnectMapConnection(MapConnection *connection) {
+    // Disconnect MapConnection's signals used by the display.
+    // It'd be nice if we could just 'connection->disconnect(this)' but that doesn't account for lambda functions.
+    QObject::disconnect(connection, &MapConnection::targetMapNameChanged, nullptr, nullptr);
+    QObject::disconnect(connection, &MapConnection::directionChanged, nullptr, nullptr);
+    QObject::disconnect(connection, &MapConnection::offsetChanged, nullptr, nullptr);
+}
+
+void Editor::displayConnection(MapConnection *connection) {
     if (!connection)
         return;
 
@@ -752,15 +760,9 @@ void Editor::displayConnection(MapConnection* connection) {
     ConnectionsListItem *listItem = new ConnectionsListItem(ui->scrollAreaContents_ConnectionsList, pixmapItem->connection, project->mapNames);
     ui->layout_ConnectionsList->insertWidget(ui->layout_ConnectionsList->count() - 1, listItem); // Insert above the vertical spacer
 
-    // It's possible for a map connection to be displayed repeatedly if it's being
-    // updated by mirroring and the target map is changing to/from the current map.
-    QObject::disconnect(connection, &MapConnection::targetMapNameChanged, nullptr, nullptr);
-    QObject::disconnect(connection, &MapConnection::directionChanged, nullptr, nullptr);
-    QObject::disconnect(connection, &MapConnection::offsetChanged, nullptr, nullptr);
-
     // Double clicking the pixmap or clicking the list item's map button opens the connected map
-    connect(listItem, &ConnectionsListItem::openMapClicked, this, &Editor::onMapConnectionDoubleClicked);
+    connect(listItem, &ConnectionsListItem::openMapClicked, this, &Editor::openConnectedMap);
-    connect(pixmapItem, &ConnectionPixmapItem::connectionItemDoubleClicked, this, &Editor::onMapConnectionDoubleClicked);
+    connect(pixmapItem, &ConnectionPixmapItem::connectionItemDoubleClicked, this, &Editor::openConnectedMap);
 
     // Sync the selection highlight between the list UI and the pixmap
     connect(pixmapItem, &ConnectionPixmapItem::selectionChanged, [=](bool selected) {
@@ -803,7 +805,7 @@ void Editor::displayConnection(MapConnection* connection) {
     }
 }
 
-void Editor::addConnection(MapConnection* connection) {
+void Editor::addConnection(MapConnection *connection) {
     if (!connection)
         return;
 
@@ -814,7 +816,7 @@ void Editor::addConnection(MapConnection* connection) {
     this->map->editHistory.push(new MapConnectionAdd(this->map, connection));
 }
 
-void Editor::removeConnection(MapConnection* connection) {
+void Editor::removeConnection(MapConnection *connection) {
     if (!connection)
         return;
     this->map->editHistory.push(new MapConnectionRemove(this->map, connection));
@@ -825,10 +827,12 @@ void Editor::removeSelectedConnection() {
         removeConnection(selected_connection_item->connection);
 }
 
-void Editor::removeConnectionPixmap(MapConnection* connection) {
+void Editor::removeConnectionPixmap(MapConnection *connection) {
     if (!connection)
         return;
 
+    disconnectMapConnection(connection);
+
     if (MapConnection::isDiving(connection->direction())) {
         removeDivingMapPixmap(connection);
         return;
@@ -857,15 +861,7 @@ void Editor::removeConnectionPixmap(MapConnection* connection) {
     delete pixmapItem;
 }
 
-void Editor::displayDivingConnections() {
+void Editor::displayDivingConnection(MapConnection *connection) {
-    if (!this->map)
-        return;
-
-    for (auto connection : this->map->getConnections())
-        displayDivingConnection(connection);
-}
-
-void Editor::displayDivingConnection(MapConnection* connection) {
     if (!connection)
         return;
 
@@ -994,7 +990,7 @@ QPoint Editor::getConnectionOrigin(MapConnection *connection) {
     return QPoint(x * 16, y * 16);
 }
 
-void Editor::updateConnectionPixmap(ConnectionPixmapItem* pixmapItem) {
+void Editor::updateConnectionPixmap(ConnectionPixmapItem *pixmapItem) {
     if (!pixmapItem)
         return;
 
@@ -1004,7 +1000,7 @@ void Editor::updateConnectionPixmap(ConnectionPixmapItem* pixmapItem) {
     maskNonVisibleConnectionTiles();
 }
 
-void Editor::setSelectedConnectionItem(ConnectionPixmapItem* pixmapItem) {
+void Editor::setSelectedConnectionItem(ConnectionPixmapItem *pixmapItem) {
     if (!pixmapItem || pixmapItem == selected_connection_item)
         return;
 
@@ -1025,14 +1021,9 @@ void Editor::setSelectedConnection(MapConnection *connection) {
     }
 }
 
-void Editor::onMapConnectionDoubleClicked(MapConnection* connection) {
-    if (connection)
-        emit openConnectedMap(connection);
-}
-
 void Editor::onBorderMetatilesChanged() {
     displayMapBorder();
-    updateBorderVisibility(); // TODO: Why do we need to call this here
+    updateBorderVisibility();
 }
 
 void Editor::onHoveredMovementPermissionChanged(uint16_t collision, uint16_t elevation) {
@@ -1214,12 +1205,8 @@ bool Editor::setMap(QString map_name) {
     if (map) {
         map->pruneEditHistory();
         map->disconnect(this);
-        for (auto connection : map->getConnections()) {
+        for (auto connection : map->getConnections())
-            // Disconnect signals used by the display
+            disconnectMapConnection(connection);
-            QObject::disconnect(connection, &MapConnection::targetMapNameChanged, nullptr, nullptr);
-            QObject::disconnect(connection, &MapConnection::directionChanged, nullptr, nullptr);
-            QObject::disconnect(connection, &MapConnection::offsetChanged, nullptr, nullptr);
-        }
     }
 
     if (project) {

src/mainwindow.cpp

@@ -1892,7 +1892,7 @@ void MainWindow::on_mainTabBar_tabBarClicked(int index)
     } else if (index == MainTab::Connections) {
         editor->setEditingConnections();
         // Stop the Dive/Emerge combo boxes from getting the initial focus
-        ui->graphicsView_Map->setFocus();
+        ui->graphicsView_Connections->setFocus();
     }
     if (index != MainTab::WildPokemon) {
         if (editor->project && editor->project->wildEncountersLoaded)
@@ -2542,6 +2542,8 @@ void MainWindow::clickToolButtonFromEditMode(QString editMode) {
 }
 
 void MainWindow::onOpenConnectedMap(MapConnection *connection) {
+    if (!connection)
+        return;
     if (userSetMap(connection->targetMapName(), true))
         editor->setSelectedConnection(connection->findMirror());
 }

src/ui/connectionslistitem.cpp

@@ -75,13 +75,13 @@ void ConnectionsListItem::mousePressEvent(QMouseEvent *) {
     this->setSelected(true);
 }
 
-void ConnectionsListItem::on_comboBox_Direction_currentTextChanged(const QString &direction) {
+void ConnectionsListItem::on_comboBox_Direction_currentTextChanged(QString direction) {
     this->setSelected(true);
     if (this->map)
         this->map->editHistory.push(new MapConnectionChangeDirection(this->connection, direction));
 }
 
-void ConnectionsListItem::on_comboBox_Map_currentTextChanged(const QString &mapName) {
+void ConnectionsListItem::on_comboBox_Map_currentTextChanged(QString mapName) {
     this->setSelected(true);
     if (this->map && ui->comboBox_Map->findText(mapName) >= 0)
         this->map->editHistory.push(new MapConnectionChangeMap(this->connection, mapName));

src/ui/mapimageexporter.cpp

@@ -33,7 +33,7 @@ MapImageExporter::MapImageExporter(QWidget *parent_, Editor *editor_, ImageExpor
     this->editor = editor_;
     this->mode = mode;
     this->setWindowTitle(getTitle(this->mode));
-    this->ui->groupBox_Connections->setVisible(this->mode == ImageExporterMode::Normal);
+    this->ui->groupBox_Connections->setVisible(this->mode != ImageExporterMode::Stitch);
     this->ui->groupBox_Timelapse->setVisible(this->mode == ImageExporterMode::Timelapse);
 
     this->ui->comboBox_MapSelection->addItems(editor->project->mapNames);
@@ -144,7 +144,7 @@ void MapImageExporter::saveImage() {
                         this->map->editHistory.redo();
                     }
                     progress.setValue(progress.maximum() - i);
-                    QPixmap pixmap = this->getFormattedMapPixmap(this->map, !this->showBorder);
+                    QPixmap pixmap = this->getFormattedMapPixmap(this->map);
                     if (pixmap.width() < maxWidth || pixmap.height() < maxHeight) {
                         QPixmap pixmap2 = QPixmap(maxWidth, maxHeight);
                         QPainter painter(&pixmap2);
@@ -167,7 +167,7 @@ void MapImageExporter::saveImage() {
                     }
                 }
                 // The latest map state is the last animated frame.
-                QPixmap pixmap = this->getFormattedMapPixmap(this->map, !this->showBorder);
+                QPixmap pixmap = this->getFormattedMapPixmap(this->map);
                 timelapseImg.addFrame(pixmap.toImage());
                 timelapseImg.save(filepath);
                 progress.close();
@@ -178,6 +178,9 @@ void MapImageExporter::saveImage() {
 }
 
 bool MapImageExporter::historyItemAppliesToFrame(const QUndoCommand *command) {
+    if (command->isObsolete())
+        return false;
+
     switch (command->id() & 0xFF) {
         case CommandId::ID_PaintMetatile:
         case CommandId::ID_BucketFillMetatile:
@@ -249,7 +252,9 @@ QPixmap MapImageExporter::getStitchedImage(QProgressDialog *progress, bool inclu
             int x = cur.x;
             int y = cur.y;
             int offset = connection->offset();
-            Map *connectionMap = this->editor->project->loadMap(connection->targetMapName());
+            Map *connectionMap = connection->targetMap();
+            if (!connectionMap)
+                continue;
             if (direction == "up") {
                 x += offset;
                 y -= connectionMap->getHeight();
@@ -318,7 +323,7 @@ QPixmap MapImageExporter::getStitchedImage(QProgressDialog *progress, bool inclu
             pixelX -= STITCH_MODE_BORDER_DISTANCE * 16;
             pixelY -= STITCH_MODE_BORDER_DISTANCE * 16;
         }
-        QPixmap pixmap = this->getFormattedMapPixmap(map.map, false);
+        QPixmap pixmap = this->getFormattedMapPixmap(map.map);
         painter.drawPixmap(pixelX, pixelY, pixmap);
     }
 
@@ -353,7 +358,7 @@ void MapImageExporter::updatePreview() {
         scene = nullptr;
     }
 
-    preview = getFormattedMapPixmap(this->map, false);
+    preview = getFormattedMapPixmap(this->map);
     scene = new QGraphicsScene;
     scene->addPixmap(preview);
     this->scene->setSceneRect(this->scene->itemsBoundingRect());
@@ -381,8 +386,7 @@ QPixmap MapImageExporter::getFormattedMapPixmap(Map *map, bool ignoreBorder) {
     // draw map border
     // note: this will break when allowing map to be selected from drop down maybe
     int borderHeight = 0, borderWidth = 0;
-    bool forceDrawBorder = showUpConnections || showDownConnections || showLeftConnections || showRightConnections;
+    if (!ignoreBorder && this->showBorder) {
-    if (!ignoreBorder && (showBorder || forceDrawBorder)) {
         int borderDistance = this->mode ? STITCH_MODE_BORDER_DISTANCE : BORDER_DISTANCE;
         map->renderBorder();
         int borderHorzDist = editor->getBorderDrawDistance(map->getBorderWidth());
@@ -401,10 +405,9 @@ QPixmap MapImageExporter::getFormattedMapPixmap(Map *map, bool ignoreBorder) {
         pixmap = newPixmap;
     }
 
-    if (!this->mode) {
+    if (!ignoreBorder && (this->showUpConnections || this->showDownConnections || this->showLeftConnections || this->showRightConnections)) {
         // if showing connections, draw on outside of image
         QPainter connectionPainter(&pixmap);
-        // TODO: Is this still the most sensible way to do this (esp. rendering pixmap)
         for (auto connectionItem : editor->connection_items) {
             const QString direction = connectionItem->connection->direction();
             if ((showUpConnections && direction == "up")
@@ -421,7 +424,7 @@ QPixmap MapImageExporter::getFormattedMapPixmap(Map *map, bool ignoreBorder) {
     QPainter eventPainter(&pixmap);
     QList<Event *> events = map->getAllEvents();
     int pixelOffset = 0;
-    if (!ignoreBorder && showBorder) {
+    if (!ignoreBorder && this->showBorder) {
         pixelOffset = this->mode == ImageExporterMode::Normal ? BORDER_DISTANCE * 16 : STITCH_MODE_BORDER_DISTANCE * 16;
     }
     for (Event *event : events) {
@@ -459,6 +462,18 @@ QPixmap MapImageExporter::getFormattedMapPixmap(Map *map, bool ignoreBorder) {
     return pixmap;
 }
 
+void MapImageExporter::updateShowBorderState() {
+    // If any of the Connections settings are enabled then this setting is locked (it's implicitly enabled)
+    const QSignalBlocker blocker(ui->checkBox_Border);
+    if (showUpConnections || showDownConnections || showLeftConnections || showRightConnections) {
+        ui->checkBox_Border->setChecked(true);
+        ui->checkBox_Border->setDisabled(true);
+        showBorder = true;
+    } else {
+        ui->checkBox_Border->setDisabled(false);
+    }
+}
+
 void MapImageExporter::on_checkBox_Elevation_stateChanged(int state) {
     showCollision = (state == Qt::Checked);
     updatePreview();
@@ -501,21 +516,25 @@ void MapImageExporter::on_checkBox_HealSpots_stateChanged(int state) {
 
 void MapImageExporter::on_checkBox_ConnectionUp_stateChanged(int state) {
     showUpConnections = (state == Qt::Checked);
+    updateShowBorderState();
     updatePreview();
 }
 
 void MapImageExporter::on_checkBox_ConnectionDown_stateChanged(int state) {
     showDownConnections = (state == Qt::Checked);
+    updateShowBorderState();
     updatePreview();
 }
 
 void MapImageExporter::on_checkBox_ConnectionLeft_stateChanged(int state) {
     showLeftConnections = (state == Qt::Checked);
+    updateShowBorderState();
     updatePreview();
 }
 
 void MapImageExporter::on_checkBox_ConnectionRight_stateChanged(int state) {
     showRightConnections = (state == Qt::Checked);
+    updateShowBorderState();
     updatePreview();
 }