Merge pull request #1119 from Kellermorph/fix-material

Fix material list reading, selection and article fields

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);
 	}
 

sources/materiallist/materiallist.h

@@ -37,6 +37,10 @@
 struct MaterialRecord
 {
 	QMap<QString, QString> 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);
 

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<QAbstractButton *>())
 	{
 		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;

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();
+	}
 }
 
 /**