From 8b54ea400b9cdf649c1ba1503ba3b443897841b3 Mon Sep 17 00:00:00 2001 From: Kellermorph Date: Mon, 28 Sep 2026 22:38:41 +0200 Subject: [PATCH] Fix material list reading, selection and article fields Follow-up to #1088, covering its review comments and one behaviour change found while testing the feature: - MaterialList::load() names the columns from the machine key line when the file has one, instead of from the translated label line above it. A catalogue written in another language fills the element fields again, and saving no longer replaces the key line with labels. - The entry picked after "New entry" is found by comparing the columns one by one instead of comparing the two maps as a whole: the entry form leaves the empty columns out, so the record never matched the line that had just been written and the search was cleared for nothing. The search is now only given up when it really hides the new line. - Applying a catalogue entry pushes an undo command only when the live edit is on. In the properties window, where there is none, the fields wait for "Apply", so "Cancel" gives the element its own values back instead of leaving the picked part in place. - Cells a file holds past the header are kept in MaterialRecord::extra and written back, so appending an article never shortens a line. - An empty cell of a column describing the article itself (MaterialList:: isArticleBound: description, designation, manufacturer, order number, supplier, model, ratings, dimensions, auxiliary block) clears the field, so an element never keeps the manufacturer of the part picked before. An empty cell of any other column (function, comment, notes, plant, location, quantity, unity), and any column the file does not hold at all, leaves the field alone. - Comments left where the review asked for them: the corner button lookup, the ';' separator fallback, the search filter cost. --- sources/materiallist/materiallist.cpp | 72 ++++++++++++++++++- sources/materiallist/materiallist.h | 5 ++ .../materiallist/materialselectiondialog.cpp | 46 +++++++++++- sources/ui/elementinfowidget.cpp | 34 +++++++-- 4 files changed, 148 insertions(+), 9 deletions(-) diff --git a/sources/materiallist/materiallist.cpp b/sources/materiallist/materiallist.cpp index e84b1ec1c..b9f8a2b8d 100644 --- a/sources/materiallist/materiallist.cpp +++ b/sources/materiallist/materiallist.cpp @@ -278,6 +278,44 @@ QString MaterialList::elementInfoKey(const QString &column, int block) return column + QStringLiteral("_auxiliary%1").arg(block); } +/** + @brief MaterialList::isArticleBound + Tell whether a column describes the article itself, rather than what + the element does or where it stands. + + An empty cell of such a column clears the field it feeds when the + entry is applied: an element cannot carry the manufacturer of two + parts at once, so the value of the article picked before has to go. + Every other column (function, comment, notes, plant, location, and + quantity and unity, which say how many pieces this element takes and + in which unit) is left alone by an empty cell, and so is a column the + file does not hold at all: a file which mentions nothing about a + field is no reason to empty it. + @param column a column of the material file + @return true when an empty cell of that column clears the field +*/ +bool MaterialList::isArticleBound(const QString &column) +{ + static const QStringList article_columns = { + QStringLiteral("description"), + QStringLiteral("designation"), + QStringLiteral("manufacturer"), + QStringLiteral("manufacturer_reference"), + QStringLiteral("machine_manufacturer_reference"), + QStringLiteral("supplier"), + QStringLiteral("model"), + QStringLiteral("category"), + QStringLiteral("voltage_rating"), + QStringLiteral("current_rating"), + QStringLiteral("width"), + QStringLiteral("height"), + QStringLiteral("depth"), + QStringLiteral("auxiliary") + }; + + return article_columns.contains(column); +} + /** @brief MaterialList::translatedColumn @param column a column of the material file @@ -443,6 +481,10 @@ QChar MaterialList::detectSeparator(const QString &first_line) const int commas = first_line.count(QLatin1Char(',')); if (semicolons == 0 && tabs == 0 && commas == 0) { + //A header holding no separator character at all is a file of a + //single column : falling back on the semicolon is deliberate, + //it is the one QElectroTech writes itself, and such a file + //needs no detection anyway. Leave this alone. return QLatin1Char(';'); } if (tabs >= semicolons && tabs >= commas && tabs > 0) { @@ -616,14 +658,25 @@ bool MaterialList::load(const QString &path, MaterialListData *data, QString *er //The label line is followed by the machine header QElectroTech //writes itself : the canonical names, so that the file is read the - //same way whatever the language it was written in. + //same way whatever the language it was written in. That machine + //line is the one naming the columns when it is there : the label + //line above it may be written in another language than the one + //running now, and would then match nothing. The label line stays + //on disk, it is only what the user reads. + QStringList machine_line; if (!rows.isEmpty() && isMachineHeaderLine(header, rows.first(), alias_map)) { - rows.takeFirst(); + machine_line = rows.takeFirst(); } QStringList used; - for (const QString &cell : header) + for (int i = 0; i < header.size(); ++i) { + //An empty machine cell means the column is named by the label + //above it (a column added by hand shows up in both lines). + QString cell = header.at(i); + if (i < machine_line.size() && !machine_line.at(i).isEmpty()) { + cell = machine_line.at(i); + } const QString column = canonicalColumn(cell, used, alias_map); used.append(column); data->columns.append(column); @@ -640,6 +693,16 @@ bool MaterialList::load(const QString &path, MaterialListData *data, QString *er empty = false; } record.setValue(data->columns.at(i), value); + } + //Cells written past the header are not thrown away : they are + //written back the way they were read, and they are content too, + //so a line only they fill is kept as well. + for (int i = data->columns.size(); i < row.size(); ++i) + { + if (!row.at(i).isEmpty()) { + empty = false; + } + record.extra.append(row.at(i)); } if (!empty) { data->records.append(record); @@ -673,6 +736,9 @@ bool MaterialList::writeFile(const QString &path, const MaterialListData &data, for (const QString &column : data.columns) { row.append(record.value(column)); } + //The cells the file holds past the header, if any, follow the + //columns : a line wider than the header stays that wide. + row.append(record.extra); rows.append(row); } diff --git a/sources/materiallist/materiallist.h b/sources/materiallist/materiallist.h index 66b5cda77..6e219c586 100644 --- a/sources/materiallist/materiallist.h +++ b/sources/materiallist/materiallist.h @@ -37,6 +37,10 @@ struct MaterialRecord { QMap values; + //Cells written after the last column of the header, kept as they + //are so that appending an article never shortens a line the user + //wrote wider than the header itself. + QStringList extra; QString value(const QString &column) const {return values.value(column);} void setValue(const QString &column, const QString &value) {values.insert(column, value);} @@ -91,6 +95,7 @@ class MaterialList static QStringList defaultColumns(); static QStringList columnsForBlock(int block); static QString elementInfoKey(const QString &column, int block); + static bool isArticleBound(const QString &column); static QString translatedColumn(const QString &column); static QStringList translatedHeader(const QStringList &columns); diff --git a/sources/materiallist/materialselectiondialog.cpp b/sources/materiallist/materialselectiondialog.cpp index d7265de7e..2c4c17f9e 100644 --- a/sources/materiallist/materialselectiondialog.cpp +++ b/sources/materiallist/materialselectiondialog.cpp @@ -173,6 +173,11 @@ void MaterialFilterProxy::setTokens(const QStringList &tokens) A row is kept when each token is found in at least one of its cells : the words may be spread over different columns, as a search for "Hilfsschalter Schneider" expects. + + Every token walks every cell of the row, which means the whole file + on every keystroke : well within reach for a catalogue, to be + revisited before someone imports a supplier export of tens of + thousands of lines. @param source_row @param source_parent @return @@ -235,6 +240,11 @@ MaterialSelectionDialog::MaterialSelectionDialog(const QString &path, //The row numbers are the order of the file : clicking them, or the //corner just above them, sorts the table by them, which means the //table is no longer sorted at all. + // + //QTableView keeps that corner button to itself, so the only way to + //reach it is the name Qt gives that private class. Nothing breaks + //if the name ever changes : the row numbers below stay connected + //and only this shortcut disappears. for (QAbstractButton *button : ui->m_table_view->findChildren()) { if (button->inherits("QTableCornerButton")) { @@ -511,6 +521,40 @@ MaterialRecord MaterialSelectionDialog::selectedRecord() const return m_model->record(source.row()); } +/** + @brief sameEntry + Tell whether two records describe the same article of the file. + + The maps are not compared as they are: an entry built by the entry + form leaves the empty columns out, load() gives every column to every + record, and a file may hold spaces the form has trimmed. Comparing + per column makes both shapes say the same thing. Cells the file holds + past the header are not part of it: they say nothing about which + article a line is. + @param lhs + @param rhs + @return true when both records describe the same article +*/ +static bool sameEntry(const MaterialRecord &lhs, const MaterialRecord &rhs) +{ + QStringList columns = lhs.values.keys(); + for (const QString &column : rhs.values.keys()) + { + if (!columns.contains(column)) { + columns.append(column); + } + } + + for (const QString &column : columns) + { + if (lhs.value(column).trimmed() != rhs.value(column).trimmed()) { + return false; + } + } + + return true; +} + /** @brief MaterialSelectionDialog::selectRecord Select the row holding that record, scrolling to it. @@ -525,7 +569,7 @@ bool MaterialSelectionDialog::selectRecord(const MaterialRecord &record) //the line which was just written. for (int i = records.size() - 1; i >= 0; --i) { - if (records.at(i) == record) + if (sameEntry(records.at(i), record)) { row = i; break; diff --git a/sources/ui/elementinfowidget.cpp b/sources/ui/elementinfowidget.cpp index f009bb374..78aab5395 100644 --- a/sources/ui/elementinfowidget.cpp +++ b/sources/ui/elementinfowidget.cpp @@ -438,9 +438,14 @@ void ElementInfoWidget::materialFromFile(int block) Write a catalogue entry into the fields of one block. Only the fields of that block are touched: applied to an auxiliary - article, the entry never reaches the main article. And a cell which is - empty in the file leaves the current value alone, so picking an entry - describing only the order reference never wipes a comment. + article, the entry never reaches the main article. + + An empty cell of a column describing the article (MaterialList:: + isArticleBound) clears the field: keeping the manufacturer of the + article picked before would describe a part which does not exist. + An empty cell of any other column, and any column the file does not + hold at all, leaves the field alone, so picking an entry describing + only the order reference never wipes a comment. @param record the entry taken from the material file @param block 0 for the main article, 1 to 4 for an auxiliary article */ @@ -459,8 +464,19 @@ void ElementInfoWidget::applyMaterialRecord(const MaterialRecord &record, int bl for (const QString &column : MaterialList::columnsForBlock(block)) { + //A column the file does not hold says nothing about the + //article: the field of the element stays as it is. + if (!record.values.contains(column)) { + continue; + } + const QString value = record.value(column); - if (value.isEmpty()) { + + //An empty cell only matters for the columns describing the + //article itself, where the previous value belongs to another + //part. For the others (function, comment, quantity...) an + //empty cell means the file has nothing to say about them. + if (value.isEmpty() && !MaterialList::isArticleBound(column)) { continue; } @@ -478,7 +494,15 @@ void ElementInfoWidget::applyMaterialRecord(const MaterialRecord &record, int bl enableLiveEdit(); } - apply(); + //Outside of the live edit the fields only carry the article: it + //is the properties window which applies them when the user presses + //"Apply" (ElementPropertiesWidget::apply() takes the undo command + //from these very fields). Applying right away would push the change + //on the undo stack before he has decided anything, and "Cancel" + //would give the fields back but never the element. + if (live_edit) { + apply(); + } } /**