Final MapConnection testing fixes

include/core/map.h

@@ -99,9 +99,8 @@ class Map : public QObject
     void addEvent(Event *);
     void deleteConnections();
     QList<MapConnection*> getConnections() const;
-    bool takeConnection(MapConnection *);
-    bool removeConnection(MapConnection *);
-    bool addConnection(MapConnection *);
+    void removeConnection(MapConnection *);
+    void 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 @@ class Map : public QObject
 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 @@ class MapConnection : public QObject
 
 signals:
     void parentMapChanged(Map* before, Map* after);
-    void targetMapNameChanged(const QString &before, const QString &after);
-    void directionChanged(const QString &before, const QString &after);
+    void targetMapNameChanged(QString before, QString after);
+    void directionChanged(QString before, QString after);
     void offsetChanged(int before, int after);
 };
 

include/editor.h

@@ -78,7 +78,6 @@ class Editor : public QObject
     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 @@ public slots:
     void clearMapGrid();
     void clearWildMonTables();
     void updateBorderVisibility();
+    void disconnectMapConnection(MapConnection *connection);
     QPoint getConnectionOrigin(MapConnection *connection);
-    void removeConnectionPixmap(MapConnection* connection);
-    void updateConnectionPixmap(ConnectionPixmapItem* connectionItem);
-    void displayConnection(MapConnection* connection);
-    void displayDivingConnection(MapConnection* connection);
+    void removeConnectionPixmap(MapConnection *connection);
+    void updateConnectionPixmap(ConnectionPixmapItem *connectionItem);
+    void displayConnection(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 @@ class ConnectionsListItem : public QFrame
     void openMapClicked(MapConnection*);
 
 private slots:
-    void on_comboBox_Direction_currentTextChanged(const QString &direction);
-    void on_comboBox_Map_currentTextChanged(const QString &mapName);
+    void on_comboBox_Direction_currentTextChanged(QString direction);
+    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 @@ class MapImageExporter : public QDialog
     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,56 +537,63 @@ 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)
     // Any additional connections being added (this can happen via mirroring) are tracked for deleting but otherwise ignored.
     if (MapConnection::isDiving(connection->direction())) {
         for (auto i : this->connections) {
             if (i->direction() == connection->direction()) {
-                this->ownedConnections.insert(connection);
-                connection->setParentMap(this, false);
-                return true;
+                trackConnection(connection);
+                return;
             }
         }
     }
 
     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))
+        this->connections.append(connection);
+
+    trackConnection(connection);
+}
+
+void Map::trackConnection(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);
+            }
+        });
+    }
 }
 
-// connection should not be deleted here, a pointer to it is allowed to persist in the edit history
-bool Map::removeConnection(MapConnection *connection) {
+// 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(pixmapItem, &ConnectionPixmapItem::connectionItemDoubleClicked, this, &Editor::onMapConnectionDoubleClicked);
+    connect(listItem, &ConnectionsListItem::openMapClicked, this, &Editor::openConnectedMap);
+    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() {
-    if (!this->map)
-        return;
-
-    for (auto connection : this->map->getConnections())
-        displayDivingConnection(connection);
-}
-
-void Editor::displayDivingConnection(MapConnection* 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()) {
-            // Disconnect signals used by the display
-            QObject::disconnect(connection, &MapConnection::targetMapNameChanged, nullptr, nullptr);
-            QObject::disconnect(connection, &MapConnection::directionChanged, nullptr, nullptr);
-            QObject::disconnect(connection, &MapConnection::offsetChanged, nullptr, nullptr);
-        }
+        for (auto connection : map->getConnections())
+            disconnectMapConnection(connection);
     }
 
     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));