Allow any number of changes to the table schema in one alterTable call

In the Edit Table dialog we used to call our alterTable function (which
works around SQLite's missing full ALTER TABLE support by - besided
other things - copying all the data of the table) for pretty much every
change immediately. This was taking a lot of time for larger tables.

Our alterTable function allowed any number of changes if they affect
only one field of the table at once. So we could have reduced the number
of calls a lot by just using that capability. Instead however, this
commit improves the alterTable function to make possible transforming a
table completely in just one call. It does so by taking the new table
schema and using that without further modification. It also takes a new
parameter to keep track of what column in the old table becomes what
column in the new table, so the data can be preserved.

This commit obviously also changes the Edit Table dialog to make proper
use of the new features. This means that whatever changes you make to a
table, you will only have to wait once until for the alterTable call,
and that's when clicking the OK button.

See issue #1444.

src/EditTableDialog.cpp

@@ -37,6 +37,10 @@ EditTableDialog::EditTableDialog(DBBrowserDB& db, const sqlb::ObjectIdentifier&
         m_table = *(pdb.getObjectByName<sqlb::Table>(curTable));
         ui->labelEditWarning->setVisible(!m_table.fullyParsed());
 
+        // Initialise the list of tracked columns for table layout changes
+        for(const auto& field : m_table.fields)
+            trackColumns[field.name()] = field.name();
+
         // Set without rowid checkbox and schema dropdown. No need to trigger any events here as we're only loading a table exactly as it is stored by SQLite, so no need
         // for error checking etc.
         ui->checkWithoutRowid->blockSignals(true);
@@ -57,9 +61,6 @@ EditTableDialog::EditTableDialog(DBBrowserDB& db, const sqlb::ObjectIdentifier&
     // And create a savepoint
     pdb.setSavepoint(m_sRestorePointName);
 
-    // Check now if foreign keys are enabled so we don't have to query this later again and again
-    m_bForeignKeysEnabled = (pdb.getPragma("foreign_keys") == "1");
-
     // Update UI
     ui->editTableName->setText(curTable.name());
     updateColumnWidth();
@@ -170,14 +171,11 @@ void EditTableDialog::accept()
     } else {
         // Editing of old table
 
-        // Rename table if necessary
-        if(ui->editTableName->text() != curTable.name())
+        // Apply all changes to the actual table in the database
+        if(!pdb.alterTable(curTable, m_table, trackColumns, ui->comboSchema->currentText()))
         {
-            if(!pdb.renameTable(ui->comboSchema->currentText(), curTable.name(), ui->editTableName->text()))
-            {
-                QMessageBox::warning(this, QApplication::applicationName(), pdb.lastError());
-                return;
-            }
+            QMessageBox::warning(this, QApplication::applicationName(), pdb.lastError());
+            return;
         }
     }
 
@@ -214,13 +212,8 @@ void EditTableDialog::checkInput()
         const auto& fields = m_table.fields;
         for(const sqlb::Field& f : fields) {
             auto fk = std::dynamic_pointer_cast<sqlb::ForeignKeyClause>(m_table.constraint({f.name()}, sqlb::Constraint::ForeignKeyConstraintType));
-            if(fk) {
-                if (oldTableName == fk->table()) {
-                    fk->setTable(normTableName);
-                    if(!m_bForeignKeysEnabled)
-                        pdb.alterTable(curTable, m_table, f.name(), &f, 0);
-                }
-            }
+            if(fk && oldTableName == fk->table())
+                fk->setTable(normTableName);
         }
 
         populateFields();
@@ -238,16 +231,15 @@ void EditTableDialog::updateTypes(QObject *object)
         QString type = typeBox->currentText();
         QString column = typeBox->property("column").toString();
 
-        size_t index;
-        for(index=0; index < m_table.fields.size(); ++index)
+        for(size_t index=0; index < m_table.fields.size(); ++index)
         {
             if(m_table.fields.at(index).name() == column)
+            {
+                m_table.fields.at(index).setType(type);
                 break;
+            }
         }
 
-        m_table.fields.at(index).setType(type);
-        if(!m_bNewTable)
-            pdb.alterTable(curTable, m_table, column, &m_table.fields[index]);
         checkInput();
     }
 }
@@ -272,7 +264,6 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
     if(index < static_cast<int>(m_table.fields.size()))
     {
         sqlb::Field& field = m_table.fields.at(index);
-        bool callRenameColumn = false;
         QString oldFieldName = field.name();
 
         switch(column)
@@ -328,8 +319,16 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
             field.setName(item->text(column));
             m_table.renameKeyInAllConstraints(oldFieldName, item->text(column));
             qobject_cast<QComboBox*>(ui->treeWidget->itemWidget(item, kType))->setProperty("column", item->text(column));
+
+            // Update the field name in the map of old column names to new column names
             if(!m_bNewTable)
-                callRenameColumn = true;
+            {
+                for(const auto& key : trackColumns)
+                {
+                    if(trackColumns[key] == oldFieldName)
+                        trackColumns[key] = field.name();
+                }
+            }
             } break;
         case kType:
             // see updateTypes() SLOT
@@ -362,8 +361,6 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
             } else {
                 item->setCheckState(kAutoIncrement, Qt::Unchecked);
             }
-            if(!m_bNewTable)
-                callRenameColumn = true;
         }
         break;
         case kNotNull:
@@ -395,8 +392,6 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
                 }
             }
             field.setNotNull(item->checkState(column) == Qt::Checked);
-            if(!m_bNewTable)
-                callRenameColumn = true;
         }
         break;
         case kAutoIncrement:
@@ -446,9 +441,6 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
                 }
             }
             field.setAutoIncrement(ischecked);
-
-            if(!m_bNewTable)
-                callRenameColumn = true;
         }
         break;
         case kUnique:
@@ -484,9 +476,6 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
                 }
             }
             field.setUnique(item->checkState(column) == Qt::Checked);
-
-            if(!m_bNewTable)
-                callRenameColumn = true;
         }
         break;
         case kDefault:
@@ -517,28 +506,15 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
                 }
             }
             field.setDefaultValue(new_value);
-            if(!m_bNewTable)
-                callRenameColumn = true;
         }
         break;
         case kCheck:
             field.setCheck(item->text(column));
-            if(!m_bNewTable)
-                callRenameColumn = true;
             break;
         case kForeignKey:
             // handled in delegate
-
-            if(!m_bNewTable)
-                callRenameColumn = true;
             break;
         }
-
-        if(callRenameColumn)
-        {
-            if(!pdb.alterTable(curTable, m_table, oldFieldName, &field))
-                QMessageBox::warning(this, qApp->applicationName(), tr("Modifying this column failed. Error returned from database:\n%1").arg(pdb.lastError()));
-        }
     }
 
     checkInput();
@@ -590,9 +566,9 @@ void EditTableDialog::addField()
     // add field to table object
     m_table.fields.emplace_back(tbitem->text(kName), typeBox->currentText());
 
-    // Actually add the new column to the table if we're editing an existing table
+    // Add the new column to the list of tracked columns to indicate it has been added
     if(!m_bNewTable)
-        pdb.addColumn(curTable, m_table.fields.back());
+        trackColumns.insert(QString(), tbitem->text(kName));
 
     checkInput();
 }
@@ -603,33 +579,27 @@ void EditTableDialog::removeField()
     if(!ui->treeWidget->currentItem())
         return;
 
-    // Are we creating a new table or editing an old one?
-    if(m_bNewTable)
+    // If we are editing an existing table, ask the user for confirmation
+    if(!m_bNewTable)
     {
-        // Creating a new one
-
-        // Just delete that item. At this point there is no DB table to edit or data to be lost anyway
-        m_table.fields.erase(m_table.fields.begin() + ui->treeWidget->indexOfTopLevelItem(ui->treeWidget->currentItem()));
-        m_table.removeKeyFromAllConstraints(ui->treeWidget->currentItem()->text(kName));
-        delete ui->treeWidget->currentItem();
-    } else {
-        // Editing an old one
-
-        // Ask user whether he really wants to delete that column
         QString msg = tr("Are you sure you want to delete the field '%1'?\nAll data currently stored in this field will be lost.").arg(ui->treeWidget->currentItem()->text(0));
-        if(QMessageBox::warning(this, QApplication::applicationName(), msg, QMessageBox::Yes | QMessageBox::No, QMessageBox::No) == QMessageBox::Yes)
+        if(QMessageBox::warning(this, QApplication::applicationName(), msg, QMessageBox::Yes | QMessageBox::No, QMessageBox::No) == QMessageBox::No)
+            return;
+
+        // Update the map of tracked columns to indicate the column is deleted
+        QString name = ui->treeWidget->currentItem()->text(0);
+        for(const auto& key : trackColumns)
         {
-            if(!pdb.alterTable(curTable, m_table, ui->treeWidget->currentItem()->text(0), nullptr))
-            {
-                QMessageBox::warning(nullptr, QApplication::applicationName(), pdb.lastError());
-            } else {
-                //relayout
-                m_table = *(pdb.getObjectByName<sqlb::Table>(curTable));
-                populateFields();
-            }
+            if(trackColumns[key] == name)
+                trackColumns[key] = QString();
         }
     }
 
+    // Just delete that item. At this point there is no DB table to edit or data to be lost anyway
+    m_table.fields.erase(m_table.fields.begin() + ui->treeWidget->indexOfTopLevelItem(ui->treeWidget->currentItem()));
+    m_table.removeKeyFromAllConstraints(ui->treeWidget->currentItem()->text(kName));
+    delete ui->treeWidget->currentItem();
+
     checkInput();
 }
 
@@ -665,54 +635,27 @@ void EditTableDialog::moveCurrentField(bool down)
     int currentRow = ui->treeWidget->currentIndex().row();
     int newRow = currentRow + (down ? 1 : -1);
 
-    // Are we creating a new table or editing an old one?
-    if(m_bNewTable)
-    {
-        // Creating a new one
+    // Save the combobox first by making a copy
+    QComboBox* oldCombo = qobject_cast<QComboBox*>(ui->treeWidget->itemWidget(ui->treeWidget->topLevelItem(currentRow), kType));
+    QComboBox* newCombo = new QComboBox(ui->treeWidget);
+    newCombo->setProperty("column", oldCombo->property("column"));
+    newCombo->installEventFilter(this);
+    connect(newCombo, SIGNAL(currentIndexChanged(int)), this, SLOT(updateTypes()));
+    newCombo->setEditable(true);
+    for(int i=0; i < oldCombo->count(); ++i)
+        newCombo->addItem(oldCombo->itemText(i));
+    newCombo->setCurrentIndex(oldCombo->currentIndex());
 
-        // Save the combobox first by making a copy
-        QComboBox* oldCombo = qobject_cast<QComboBox*>(ui->treeWidget->itemWidget(ui->treeWidget->topLevelItem(currentRow), kType));
-        QComboBox* newCombo = new QComboBox(ui->treeWidget);
-        newCombo->setProperty("column", oldCombo->property("column"));
-        newCombo->installEventFilter(this);
-        connect(newCombo, SIGNAL(currentIndexChanged(int)), this, SLOT(updateTypes()));
-        newCombo->setEditable(true);
-        for(int i=0; i < oldCombo->count(); ++i)
-            newCombo->addItem(oldCombo->itemText(i));
-        newCombo->setCurrentIndex(oldCombo->currentIndex());
+    // Now, just remove the item and insert it at it's new position, then restore the combobox
+    QTreeWidgetItem* item = ui->treeWidget->takeTopLevelItem(currentRow);
+    ui->treeWidget->insertTopLevelItem(newRow, item);
+    ui->treeWidget->setItemWidget(item, kType, newCombo);
 
-        // Now, just remove the item and insert it at it's new position, then restore the combobox
-        QTreeWidgetItem* item = ui->treeWidget->takeTopLevelItem(currentRow);
-        ui->treeWidget->insertTopLevelItem(newRow, item);
-        ui->treeWidget->setItemWidget(item, kType, newCombo);
+    // Select the old item at its new position
+    ui->treeWidget->setCurrentIndex(ui->treeWidget->currentIndex().sibling(newRow, 0));
 
-        // Select the old item at its new position
-        ui->treeWidget->setCurrentIndex(ui->treeWidget->currentIndex().sibling(newRow, 0));
-
-        // Finally update the table SQL
-        std::swap(m_table.fields[newRow], m_table.fields[currentRow]);
-    } else {
-        // Editing an old one
-
-        // Move the actual column
-        if(!pdb.alterTable(
-                    curTable,
-                    m_table,
-                    ui->treeWidget->currentItem()->text(0),
-                    &m_table.fields[ui->treeWidget->indexOfTopLevelItem(ui->treeWidget->currentItem())],
-                    (down ? 1 : -1)
-                ))
-        {
-            QMessageBox::warning(nullptr, QApplication::applicationName(), pdb.lastError());
-        } else {
-            // Reload table SQL
-            m_table = *(pdb.getObjectByName<sqlb::Table>(curTable));
-            populateFields();
-
-            // Select old item at new position
-            ui->treeWidget->setCurrentIndex(ui->treeWidget->indexAt(QPoint(1, 1)).sibling(newRow, 0));
-        }
-    }
+    // Finally update the table SQL
+    std::swap(m_table.fields[newRow], m_table.fields[currentRow]);
 
     // Update the SQL preview
     updateSqlText();
@@ -748,33 +691,10 @@ void EditTableDialog::setWithoutRowid(bool without_rowid)
 
     // Update the SQL preview
     updateSqlText();
-
-    // Update table if we're editing an existing table
-    if(!m_bNewTable)
-    {
-        if(!pdb.alterTable(curTable, m_table, QString(), nullptr, 0))
-        {
-            QMessageBox::warning(this, QApplication::applicationName(),
-                                 tr("Setting the rowid column for the table failed. Error message:\n%1").arg(pdb.lastError()));
-        }
-    }
 }
 
-void EditTableDialog::changeSchema(const QString& schema)
+void EditTableDialog::changeSchema(const QString& /*schema*/)
 {
     // Update the SQL preview
     updateSqlText();
-
-    // Update table if we're editing an existing table
-    if(!m_bNewTable)
-    {
-        if(pdb.alterTable(curTable, m_table, QString(), nullptr, 0, schema))
-        {
-            // Save the new schema name to use it from now on
-            curTable.setSchema(schema);
-        } else {
-            QMessageBox::warning(this, QApplication::applicationName(), tr("Changing the table schema failed. Error message:\n%1").arg(pdb.lastError()));
-            ui->comboSchema->setCurrentText(curTable.schema()); // Set it back to the original schema
-        }
-    }
 }

src/EditTableDialog.h

@@ -64,12 +64,10 @@ private:
     DBBrowserDB& pdb;
     ForeignKeyEditorDelegate* m_fkEditorDelegate;
     sqlb::ObjectIdentifier curTable;
+    QMap<QString, QString> trackColumns;
     sqlb::Table m_table;
-    QStringList types;
-    QStringList fields;
     bool m_bNewTable;
     QString m_sRestorePointName;
-    bool m_bForeignKeysEnabled;
 };
 
 #endif

src/sql/sqlitetypes.cpp

@@ -120,6 +120,18 @@ private:
     antlr::RefAST m_root;
 };
 
+bool Object::operator==(const Object& rhs) const
+{
+    if(m_name != rhs.m_name)
+        return false;
+    if(m_fullyParsed != rhs.m_fullyParsed)  // We check for the fully parsed flag to make sure not to lose anything in some corner cases
+        return false;
+
+    // We don't care about the original SQL text
+
+    return true;
+}
+
 QString Object::typeToString(Types type)
 {
     switch(type)
@@ -204,6 +216,28 @@ QString CheckConstraint::toSql(const QStringList&) const
     return result;
 }
 
+bool Field::operator==(const Field& rhs) const
+{
+    if(m_name != rhs.m_name)
+        return false;
+    if(m_type != rhs.m_type)
+        return false;
+    if(m_notnull != rhs.m_notnull)
+        return false;
+    if(m_check != rhs.m_check)
+        return false;
+    if(m_defaultvalue != rhs.m_defaultvalue)
+        return false;
+    if(m_autoincrement != rhs.m_autoincrement)
+        return false;
+    if(m_unique != rhs.m_unique)
+        return false;
+    if(m_collation != rhs.m_collation)
+        return false;
+
+    return true;
+}
+
 QString Field::toString(const QString& indent, const QString& sep) const
 {
     QString str = indent + escapeIdentifier(m_name) + sep + m_type;
@@ -319,6 +353,23 @@ Table& Table::operator=(const Table& rhs)
     return *this;
 }
 
+bool Table::operator==(const Table& rhs) const
+{
+    if(!Object::operator==(rhs))
+        return false;
+
+    if(m_rowidColumn != rhs.m_rowidColumn)
+        return false;
+    if(m_constraints != rhs.m_constraints)
+        return false;
+    if(m_virtual != rhs.m_virtual)
+        return false;
+    if(fields != rhs.fields)
+        return false;
+
+    return true;
+}
+
 Table::field_iterator Table::findPk()
 {
     // TODO This is a stupid function (and always was) which should be fixed/improved

src/sql/sqlitetypes.h

@@ -54,6 +54,7 @@ void setIdentifierQuoting(escapeQuoting toQuoting);
 
 QString escapeIdentifier(QString id);
 std::string escapeIdentifier(std::string id);
+QStringList escapeIdentifier(const QStringList& ids);
 
 class ObjectIdentifier
 {
@@ -187,6 +188,8 @@ public:
     explicit Object(const QString& name): m_name(name), m_fullyParsed(false) {}
     virtual ~Object() {}
 
+    virtual bool operator==(const Object& rhs) const;
+
     virtual Types type() const = 0;
     static QString typeToString(Types type);
 
@@ -353,10 +356,7 @@ public:
         , m_collation(collation)
     {}
 
-    bool operator==(const Field& rhs) const
-    {
-        return m_name == rhs.name();
-    }
+    bool operator==(const Field& rhs) const;
 
     QString toString(const QString& indent = "\t", const QString& sep = "\t") const;
 
@@ -405,6 +405,8 @@ public:
     ~Table() override;
     Table& operator=(const Table& rhs);
 
+    bool operator==(const Table& rhs) const;
+
     Types type() const override { return Object::Table; }
 
     FieldVector fields;
@@ -585,15 +587,22 @@ private:
 template<typename T>
 typename T::field_iterator findField(T* object, const QString& name)
 {
-    return std::find_if(object->fields.begin(), object->fields.end(), [&name](const typename T::field_type& f) {return f.name().compare(name, Qt::CaseInsensitive) == 0;});
+    return std::find_if(object->fields.begin(), object->fields.end(), [&name](const typename T::field_type& f) {
+        return f.name().compare(name, Qt::CaseInsensitive) == 0;
+    });
 }
 template<typename T>
-typename T::field_iterator findField(std::shared_ptr<T> object, const QString& name)
+typename T::field_iterator findField(const T* object, const QString& name)
+{
+    return findField(const_cast<T*>(object), name);
+}
+template<typename T>
+typename std::remove_reference<T>::type::field_iterator findField(std::shared_ptr<T> object, const QString& name)
 {
     return findField(object.get(), name);
 }
 template<typename T>
-typename T::field_iterator findField(T& object, const QString& name)
+typename std::remove_reference<T>::type::field_iterator findField(T& object, const QString& name)
 {
     return findField(&object, name);
 }

src/sqlitedb.cpp

@@ -1308,156 +1308,216 @@ bool DBBrowserDB::addColumn(const sqlb::ObjectIdentifier& tablename, const sqlb:
     return executeSQL(sql);
 }
 
-bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& table, QString name, const sqlb::Field* to, int move, QString newSchemaName)
+bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& new_table, AlterTableTrackColumns track_columns, QString newSchemaName)
 {
-    /*
-     * USE CASES:
-     * 1) Set table; unset name, to and move: Change table constraints only.
-     * 2) Set table and name; unset to and move: Change table constraints and remove column.
-     * 3) Set table, name and to; unset move: Change table constraints and rename/edit column.
-     * 4) Set table, name, to and move: Change table constraints, rename/edit column and move it.
-     */
+    // This function is split into three different parts:
+    // Part 1 checks the arguments and prepares them for processing. It also prepares the transaction etc.
+    // Part 2 uses the built-in abilities of SQLite to change as much of the table schema as possible.
+    // Part 3 does the rest of the changes to the table schema. It also finishes the transaction etc.
 
-    // TODO: This function needs to be cleaned up. It might make sense to split it up in several parts than can be reused
-    // more easily. Besides that, it might make sense to support some potential use cases in a more sophisticated way. These include:
-    // 1) Allow modifying multiple columns at once in order to only have to call this function (including all its overhead) once instead of once per change.
-    // 2) Include the addColumn() use case in here, so the calling side doesn't need to know anything about how this class handles table modifications.
+    //
+    // P A R T   1
+    //
 
     // If no new schema name has been set, we just use the old schema name
     if(newSchemaName.isNull())
     {
         newSchemaName = tablename.schema();
+
+        // When renaming the table in the current schema, check if it doesn't exist already in there
+        if(tablename.name() != new_table.name() && getObjectByName(sqlb::ObjectIdentifier(newSchemaName, new_table.name())) != nullptr)
+        {
+            lastErrorMessage = tr("A table with the name '%1' already exists in schema '%2'.").arg(new_table.name()).arg(newSchemaName);
+            return false;
+        }
     } else {
         // We're moving the table to a different schema. So check first if it doesn't already exist in the new schema.
-        if(getObjectByName(sqlb::ObjectIdentifier(newSchemaName, tablename.name())) != nullptr && newSchemaName != tablename.schema())
+        if(newSchemaName != tablename.schema() && getObjectByName(sqlb::ObjectIdentifier(newSchemaName, new_table.name())) != nullptr)
         {
-            lastErrorMessage = tr("A table with the name '%1' already exists in schema '%2'.").arg(tablename.name()).arg(newSchemaName);
+            lastErrorMessage = tr("A table with the name '%1' already exists in schema '%2'.").arg(new_table.name()).arg(newSchemaName);
             return false;
         }
     }
 
-    // Create table schema
-    sqlb::TablePtr oldSchema = getObjectByName<sqlb::Table>(tablename);
-
-    // Check if field actually exists
-    if(!name.isNull() && sqlb::findField(oldSchema, name) == oldSchema->fields.end())
+    // Get old table schema
+    sqlb::TablePtr old_table_ptr = getObjectByName<sqlb::Table>(tablename);
+    if(old_table_ptr == nullptr)
     {
-        lastErrorMessage = tr("renameColumn: cannot find column %1.").arg(name);
+        lastErrorMessage = tr("No table with name '%1' exists in schema '%2'.").arg(tablename.name()).arg(tablename.schema());
         return false;
     }
+    sqlb::Table old_table = *old_table_ptr;
+
+    // Check if tracked fields actually exist in the old table
+    for(const auto& old_name : track_columns.keys())
+    {
+        if(!old_name.isNull() && sqlb::findField(old_table, old_name) == old_table.fields.end())
+        {
+            lastErrorMessage = tr("Cannot find column %1.").arg(old_name);
+            return false;
+        }
+    }
+
+    // Check if there are any columns in the old table which are not mentioned in the tracked columns list.
+    // We do this before checking if all tracked fields are in the new table to make sure the following check includes them.
+    for(const auto& field : old_table.fields)
+    {
+        if(!track_columns.keys().contains(field.name()))
+        {
+            // If a field isn't tracked, add it to the list and indicate explicitly that it has the same name in the new table
+            track_columns[field.name()] = field.name();
+        }
+    }
+
+    // Check if tracked fields actually exist in the new table
+    for(const auto& new_name : track_columns.values())
+    {
+        if(!new_name.isNull() && sqlb::findField(new_table, new_name) == new_table.fields.end())
+        {
+            lastErrorMessage = tr("Cannot find column %1.").arg(new_name);
+            return false;
+        }
+    }
 
     // Create savepoint to be able to go back to it in case of any error
     QString savepointName = generateSavepointName("renamecolumn");
     if(!setSavepoint(savepointName))
     {
-        lastErrorMessage = tr("renameColumn: creating savepoint failed. DB says: %1").arg(lastErrorMessage);
+        lastErrorMessage = tr("Creating savepoint failed. DB says: %1").arg(lastErrorMessage);
         return false;
     }
 
     // No automatic schema updates from now on
     NoStructureUpdateChecks nup(*this);
 
-    // Newer versions of SQLite add a better ALTER TABLE support which we can use
-#if SQLITE_VERSION_NUMBER >= 3025000
-    // If the name of the field should be changed do that by using SQLite's ALTER TABLE feature
-    if(!name.isNull() && to && name != to->name())
+    //
+    // P A R T   2
+    //
+
+    // This variable is used to track whether something was changed by this part of the function
+    bool changed_something = false;
+
+    // Rename table if necessary
+    if(newSchemaName == tablename.schema() && tablename.name() != new_table.name())
     {
-        if(!executeSQL(QString("ALTER TABLE %1 RENAME COLUMN %2 TO %3;")
-                       .arg(tablename.toString())
-                       .arg(sqlb::escapeIdentifier(name))
-                       .arg(sqlb::escapeIdentifier(to->name()))))
+        if(!renameTable(tablename.schema(), old_table.name(), new_table.name()))
         {
-            QString error(tr("renameColumn: renaming the column failed. DB says:\n%1").arg(lastErrorMessage));
             revertToSavepoint(savepointName);
-            lastErrorMessage = error;
             return false;
         }
 
-        // Update our schema representation to get all the changed triggers, views and indices
-        updateSchema();
+        changed_something = true;
+    }
 
-        // Check if that was all we were asked to do. That's the case if the field is not to be deleted (which we already checked for above), if the field
-        // is not to be moved, if the table is not to be moved and if nothing besides the name of the field changed in the field definition.
-        sqlb::Field oldFieldWithNewName = *sqlb::findField(oldSchema, name);
-        oldFieldWithNewName.setName(to->name());
-        if(move == 0 && tablename.schema() == newSchemaName && oldFieldWithNewName == *to)
+    // Add columns if necessary
+    const auto new_fields = track_columns.values(QString());
+    for(const auto& field : new_table.fields)
+    {
+        // We loop through all the fields of the new table schema and check for each of them if they are new.
+        // If so, we add that field. The reason for looping through the new table schema instead of the track_columns
+        // map is that this way we make sure to preserve their order which increases our chances that we are done after
+        // this step.
+        if(new_fields.contains(field.name()))
         {
-            // We're done.
-
-            // Release the savepoint - everything went fine
-            if(!releaseSavepoint(savepointName))
+            if(!addColumn(sqlb::ObjectIdentifier(tablename.schema(), new_table.name()), field))
             {
-                lastErrorMessage = tr("renameColumn: releasing savepoint failed. DB says: %1").arg(lastErrorMessage);
+                revertToSavepoint(savepointName);
+                return false;
+            }
+        }
+
+        changed_something = true;
+    }
+
+    // Newer versions of SQLite add a better ALTER TABLE support which we can use
+#if SQLITE_VERSION_NUMBER >= 3025000
+    // If the name of a field should be changed do that by using SQLite's ALTER TABLE feature
+    for(const auto& old_name : track_columns.keys())
+    {
+        QString new_name = track_columns[old_name];
+        if(!old_name.isNull() && !new_name.isNull() && new_name != old_name)
+        {
+            if(!executeSQL(QString("ALTER TABLE %1 RENAME COLUMN %2 TO %3;")
+                           .arg(sqlb::ObjectIdentifier(tablename.schema(), new_table.name()).toString())
+                           .arg(sqlb::escapeIdentifier(old_name))
+                           .arg(sqlb::escapeIdentifier(new_name))))
+            {
+                QString error(tr("Renaming the column failed. DB says:\n%1").arg(lastErrorMessage));
+                revertToSavepoint(savepointName);
+                lastErrorMessage = error;
                 return false;
             }
 
-            return true;
-        } else {
-            // There's more to do.
-
-            // We can have the rest of the function deal with the remaining changes by reloading the table schema as it is now and updating the name of the column
-            // to change.
-            oldSchema = getObjectByName<sqlb::Table>(tablename);
-            name = to->name();
+            changed_something = true;
         }
     }
 #endif
 
-    // Create a new table with a name that hopefully doesn't exist yet.
-    // Its layout is exactly the same as the one of the table to change - except for the column to change
-    // of course, and the table constraints which are copied from the table parameter.
-    sqlb::Table newSchema = *oldSchema;
-    QString temp_table_name = generateTemporaryTableName(newSchemaName);
-    newSchema.setName(temp_table_name);
-    newSchema.setConstraints(table.allConstraints());
-    newSchema.setRowidColumn(table.rowidColumn());
-    QString select_cols;
-    if(!to)
+    // Update our schema representation to get the new table and all the changed triggers, views and indices
+    if(changed_something)
     {
-        // We want drop the column - so just remove the field. If the name is set to null, skip this step. This effectively leaves all fields as they are,
-        // thus only changing the table constraints.
-        if(!name.isNull())
-            sqlb::removeField(newSchema, name);
-
-        for(size_t i=0;i<newSchema.fields.size();++i)
-            select_cols.append(sqlb::escapeIdentifier(newSchema.fields.at(i).name()) + ',');
-        select_cols.chop(1);    // remove last comma
-    } else {
-        // We want to modify it
-
-        auto index = sqlb::findField(newSchema, name);
-
-        // Move field
-        if(move)
-            std::iter_swap(index, index + move);
-
-        // Get names of fields to select from old table now - after the field has been moved and before it might be renamed
-        for(size_t i=0;i<newSchema.fields.size();++i)
-            select_cols.append(sqlb::escapeIdentifier(newSchema.fields.at(i).name()) + ',');
-        select_cols.chop(1);    // remove last comma
-
-        // Modify field
-        newSchema.renameKeyInAllConstraints((index + move)->name(), to->name());
-        *(index + move) = *to;
+        updateSchema();
+        old_table = *getObjectByName<sqlb::Table>(sqlb::ObjectIdentifier(tablename.schema(), new_table.name()));
     }
 
-    // Create the new table
-    if(!executeSQL(newSchema.sql(newSchemaName), true, true))
+    // Check if there's still more work to be done or if we are finished now
+    if(tablename.schema() == newSchemaName && old_table == new_table)
     {
-        QString error(tr("renameColumn: creating new table failed. DB says: %1").arg(lastErrorMessage));
+        // Release the savepoint - everything went fine
+        if(!releaseSavepoint(savepointName))
+        {
+            lastErrorMessage = tr("Releasing savepoint failed. DB says: %1").arg(lastErrorMessage);
+            return false;
+        }
+
+        // Success, update the DB schema before returning
+        updateSchema();
+        return true;
+    }
+
+    //
+    // P A R T   3
+    //
+
+    // Create a new table with the desired schema and a name that doesn't exist yet
+    QString new_table_name = new_table.name();
+    sqlb::Table new_table_with_random_name = new_table;
+    new_table_with_random_name.setName(generateTemporaryTableName(newSchemaName));
+    if(!executeSQL(new_table_with_random_name.sql(newSchemaName), true, true))
+    {
+        QString error(tr("Creating new table failed. DB says: %1").arg(lastErrorMessage));
         revertToSavepoint(savepointName);
         lastErrorMessage = error;
         return false;
     }
 
+    // Assemble list of column names to copy from in the old table and list of column names to into into in the new table
+    QStringList copy_values_from;
+    QStringList copy_values_to;
+    for(const auto& from : track_columns.keys())
+    {
+        // Ignore new fields
+        if(from.isNull())
+            continue;
+
+        // Ignore deleted fields
+        QString to = track_columns[from];
+        if(to.isNull())
+            continue;
+
+        copy_values_from.push_back(from);
+        copy_values_to.push_back(to);
+    }
+
     // Copy the data from the old table to the new one
-    if(!executeSQL(QString("INSERT INTO %1.%2 SELECT %3 FROM %4;")
+    if(!executeSQL(QString("INSERT INTO %1.%2 (%3) SELECT %4 FROM %5;")
                    .arg(sqlb::escapeIdentifier(newSchemaName))
-                   .arg(sqlb::escapeIdentifier(temp_table_name))
-                   .arg(select_cols)
+                   .arg(sqlb::escapeIdentifier(new_table_with_random_name.name()))
+                   .arg(sqlb::escapeIdentifier(copy_values_to).join(","))
+                   .arg(sqlb::escapeIdentifier(copy_values_from).join(","))
                    .arg(tablename.toString())))
     {
-        QString error(tr("renameColumn: copying data to new table failed. DB says:\n%1").arg(lastErrorMessage));
+        QString error(tr("Copying data to new table failed. DB says:\n%1").arg(lastErrorMessage));
         revertToSavepoint(savepointName);
         lastErrorMessage = error;
         return false;
@@ -1476,19 +1536,26 @@ bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb
             {
                 sqlb::IndexPtr idx = std::dynamic_pointer_cast<sqlb::Index>(it);
 
-                // Are we updating a field name or are we removing a field entirely?
-                if(to)
+                // Loop through all changes to the table schema. For indices only the column names are relevant, so it suffices to look at the
+                // list of tracked columns
+                for(const auto& from : track_columns)
                 {
-                    // We're updating a field name. So search for it in the index and replace it whereever it is found
-                    for(size_t i=0;i<idx->fields.size();i++)
+                    QString to = track_columns[from];
+
+                    // Are we updating the field name or are we removing the field entirely?
+                    if(!to.isNull())
                     {
-                        if(idx->fields[i].name() == name)
-                            idx->fields[i].setName(to->name());
+                        // We're updating the field name. So search for it in the index and replace it whereever it is found
+                        for(size_t i=0;i<idx->fields.size();i++)
+                        {
+                            if(idx->fields[i].name() == from)
+                                idx->fields[i].setName(to);
+                        }
+                    } else {
+                        // We're removing a field. So remove it from any indices, too.
+                        while(sqlb::removeField(idx, from))
+                            ;
                     }
-                } else {
-                    // We're removing a field. So remove it from any indices, too.
-                    while(sqlb::removeField(idx, name))
-                        ;
                 }
 
                 // Only try to add the index later if it has any columns remaining. Also use the new schema name here, too, to basically move
@@ -1514,14 +1581,14 @@ bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb
     // Delete the old table
     if(!executeSQL(QString("DROP TABLE %1;").arg(tablename.toString()), true, true))
     {
-        QString error(tr("renameColumn: deleting old table failed. DB says: %1").arg(lastErrorMessage));
+        QString error(tr("Deleting old table failed. DB says: %1").arg(lastErrorMessage));
         revertToSavepoint(savepointName);
         lastErrorMessage = error;
         return false;
     }
 
     // Rename the temporary table
-    if(!renameTable(newSchemaName, temp_table_name, tablename.name()))
+    if(!renameTable(newSchemaName, new_table_with_random_name.name(), tablename.name()))
     {
         revertToSavepoint(savepointName);
         return false;
@@ -1548,7 +1615,7 @@ bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb
     // Release the savepoint - everything went fine
     if(!releaseSavepoint(savepointName))
     {
-        lastErrorMessage = tr("renameColumn: releasing savepoint failed. DB says: %1").arg(lastErrorMessage);
+        lastErrorMessage = tr("Releasing savepoint failed. DB says: %1").arg(lastErrorMessage);
         return false;
     }
 

src/sqlitedb.h

@@ -3,13 +3,13 @@
 
 #include "sql/sqlitetypes.h"
 
+#include <condition_variable>
 #include <memory>
 #include <mutex>
-#include <condition_variable>
 
-#include <QStringList>
-#include <QMultiMap>
 #include <QByteArray>
+#include <QMultiMap>
+#include <QStringList>
 
 struct sqlite3;
 class CipherSettings;
@@ -155,17 +155,25 @@ public:
     bool addColumn(const sqlb::ObjectIdentifier& tablename, const sqlb::Field& field);
 
     /**
-     * @brief alterTable Can be used to rename, modify or drop an existing column of a given table
-     * @param schema Specifies the name of the schema, i.e. the database name, of the table
-     * @param tablename Specifies the name of the table to edit
-     * @param table Specifies the table to edit. The table constraints are used from this but not the columns
-     * @param name Name of the column to edit
-     * @param to The new field definition with changed name, type or the like. If Null-Pointer is given the column is dropped.
-     * @param move Set this to a value != 0 to move the new column to a different position
+     * @brief This type maps from old column names to new column names. Given the old and the new table definition, this suffices to
+     * track fields between the two.
+     * USE CASES:
+     * 1) Don't specify a column at all or specify equal column names: Keep its name as-is.
+     * 2) Specify different column names: Rename the field.
+     * 3) Map from an existing column name to a Null string: Delete the column.
+     * 4) Map from a Null column name to a new column name: Add the column.
+     */
+    using AlterTableTrackColumns = QMap<QString, QString>;
+
+    /**
+     * @brief alterTable Can be used to rename, modify or drop existing columns of a given table
+     * @param tablename Specifies the schema and name of the table to edit
+     * @param new_table Specifies the new table schema. This is exactly how the new table is going to look like.
+     * @param track_columns Maps old column names to new column names. This is used to copy the data from the old table to the new one.
      * @param newSchema Set this to a non-empty string to move the table to a new schema
      * @return true if renaming was successful, false if not. In the latter case also lastErrorMessage is set
      */
-    bool alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& table, QString name, const sqlb::Field* to, int move = 0, QString newSchemaName = QString());
+    bool alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& new_table, AlterTableTrackColumns track_columns, QString newSchemaName = QString());
 
     objectMap getBrowsableObjects(const QString& schema) const;