From 87a010eb757e29fe16a6dd95da9b58a1e1e42519 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Sun, 2 Aug 2026 01:36:11 +1200 Subject: [PATCH 1/3] Never leave a collection folder without a name, and say when it is broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://qelectrotech.org/bugtracker/view.php?id=332 localName() set a non-root folder's label only inside the success path of loading its qet_directory file. If that load failed -- file missing, malformed, or unopenable because of the Windows path-encoding problem with accented characters that plc-user diagnosed on the tracker -- nothing was set at all, and since a fresh item's text() is null the folder rendered with a completely blank label. That is the reported symptom. Resolve the name into a local and always fall back to the folder's own directory name, so the label is never empty whatever went wrong. The fallback is applied *after* NamesList::name() rather than passed into it. This matters: name() returns a caller-supplied fallback before it reaches its "first available translation" step, so passing m_path in would replace a perfectly good name in some other language with the raw directory name. A folder named only in French, viewed under an English locale, previously showed "Accentué" and must keep doing so. Falling back on its own would then hide the broken file -- the user sees a plausible name and never learns there is anything to repair. So a folder whose qet_directory could not be read now says so in its tooltip, naming the file, above the collection path that tooltip already carried. Suggested by plc-user on PR #622. The flag is recorded in localName() and consumed in setUpData(), because setUpData() assigns the tooltip after localName() runs and would otherwise discard it. Only a file-level failure is flagged. A readable qet-directory with no entry for the current language is not an error; NamesList::name() resolves that itself and no warning is shown. Verified on a fixture collection of four folders -- valid, malformed, missing, and one named only in French: master this patch fr-only Accentué Accentué (no warning) malformed malformed (warning) no qet_directory no_file (warning) valid Valid Folder Valid Folder (no warning) --- .../fileelementcollectionitem.cpp | 43 +++++++++++++++++-- .../fileelementcollectionitem.h | 5 +++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/sources/ElementsCollection/fileelementcollectionitem.cpp b/sources/ElementsCollection/fileelementcollectionitem.cpp index b14e86088..013ee0153 100644 --- a/sources/ElementsCollection/fileelementcollectionitem.cpp +++ b/sources/ElementsCollection/fileelementcollectionitem.cpp @@ -136,18 +136,41 @@ QString FileElementCollectionItem::localName() } else { + // Fall back to the raw directory name (m_path) whenever the + // translated name can't be obtained -- qet_directory missing, + // unreadable (e.g. a Windows path-encoding issue with special + // characters, see bugtracker #332), malformed, or present but + // without a usable name entry -- rather than leaving the item + // blank. + QString display_name; + bool readable = false; QString str(fileSystemPath() % "/qet_directory"); pugi::xml_document docu; - if(docu.load_file(str.toStdWString().c_str())) + if (docu.load_file(str.toStdWString().c_str())) { if (QString(docu.document_element().name()) == "qet-directory") { + readable = true; NamesList nl; nl.fromXml(docu.document_element()); - setText(nl.name()); + // Deliberately no fallback argument: a non-empty one + // is returned *before* NamesList::name() reaches its + // "first available translation" step, so passing + // m_path here would replace a perfectly good name in + // some other language with the raw directory name. + // The fallback belongs after the chain, not inside it. + display_name = nl.name(); } } + setText(display_name.isEmpty() ? m_path : display_name); + + // Only a file-level failure counts: a readable qet-directory + // with no entry for the current language is not an error, + // NamesList::name() resolves that on its own. Recorded here + // and reported by setUpData(), which sets the tooltip after + // this runs. + m_qet_directory_unreadable = !readable; } } else if (isElement()) { @@ -350,7 +373,21 @@ void FileElementCollectionItem::setUpData() } } - setToolTip(collectionPath()); + // Falling back to the raw directory name keeps the folder usable, but + // on its own it hides the fact that a file is broken: the user sees a + // plausible name and never learns there is anything to repair. Say so + // above the collection path, which stays as the last line the way the + // element tooltip above builds it. + QStringList tip; + if (isDir() && m_qet_directory_unreadable) + { + tip << QObject::tr("Le fichier « %1 » est absent ou illisible : " + "le nom traduit de ce dossier n'a pas pu être lu, " + "son nom de dossier est affiché à la place.") + .arg(fileSystemPath() % "/qet_directory"); + } + tip << collectionPath(); + setToolTip(tip.join(QLatin1Char('\n'))); } /** diff --git a/sources/ElementsCollection/fileelementcollectionitem.h b/sources/ElementsCollection/fileelementcollectionitem.h index d2c735d72..4c9885f79 100644 --- a/sources/ElementsCollection/fileelementcollectionitem.h +++ b/sources/ElementsCollection/fileelementcollectionitem.h @@ -64,6 +64,11 @@ class FileElementCollectionItem : public ElementCollectionItem private: QString m_path; + /// True when this directory's qet_directory file is missing or + /// unreadable, so setUpData() can say so in the tooltip. Recorded + /// rather than acted on in localName(), because setUpData() resets + /// the tooltip afterwards and would otherwise discard it. + bool m_qet_directory_unreadable = false; }; #endif // FILEELEMENTCOLLECTIONITEM2_H From f6b448daa7b16731c3547026a8f5094c482b1598 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Tue, 4 Aug 2026 06:57:16 +1200 Subject: [PATCH 2/3] Badge unreadable-qet_directory folders with a warning icon in the tree @plc-user asked (review on #633) for a way to see a broken folder directly in the tree instead of only on tooltip hover, originally suggesting a "FixMe: " text prefix on the displayed name. That name is reused verbatim in dialog titles and status-bar messages elsewhere (elementscollectionwidget.cpp), so baking a prefix into it would leak into those too. An icon badge gets the same visibility without touching the name value. setUpIcon() overlays a small warning glyph on the folder icon when m_qet_directory_unreadable is set. Also drop the "already has an icon, skip" guard for directories specifically: that flag is only known once the async setUpData()/localName() job completes (QtConcurrent::map), so without this a directory painted before that finished would have its plain folder icon cached forever and never pick up the badge. --- .../fileelementcollectionitem.cpp | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/sources/ElementsCollection/fileelementcollectionitem.cpp b/sources/ElementsCollection/fileelementcollectionitem.cpp index 013ee0153..ccd58d0b9 100644 --- a/sources/ElementsCollection/fileelementcollectionitem.cpp +++ b/sources/ElementsCollection/fileelementcollectionitem.cpp @@ -22,7 +22,39 @@ #include "../qeticons.h" #include "elementslocation.h" +#include #include +#include +#include +#include + +namespace { + /** + @return the folder icon overlaid with a small warning badge in the + bottom-right corner. Used for a directory whose qet_directory could + not be read (@see FileElementCollectionItem::m_qet_directory_unreadable), + so the problem is visible in the tree itself and not only on hover + via the tooltip. Built once: same folder icon, same badge, every time. + */ + const QIcon &unreadableFolderIcon() + { + static const QIcon icon = []() { + QPixmap pixmap = QET::Icons::Folder.pixmap(16, 16); + const QPixmap badge = QApplication::style() + ->standardIcon(QStyle::SP_MessageBoxWarning) + .pixmap(9, 9); + + QPainter painter(&pixmap); + painter.drawPixmap(pixmap.width() - badge.width(), + pixmap.height() - badge.height(), + badge); + painter.end(); + + return QIcon(pixmap); + }(); + return icon; + } +} /** @brief FileElementCollectionItem::FileElementCollectionItem @@ -398,7 +430,13 @@ void FileElementCollectionItem::setUpData() */ void FileElementCollectionItem::setUpIcon() { - if (!icon().isNull()) + // Directories are cheap to (re-)decide: no early return for them. + // setUpData() -- which resolves m_qet_directory_unreadable via + // localName() -- runs asynchronously (QtConcurrent::map), so this can + // be called for a directory before that result is known; without + // this, the plain folder icon would get cached by the guard below + // and the warning badge would never appear for that item. + if (!isDir() && !icon().isNull()) return; if (isCollectionRoot()) { @@ -417,7 +455,8 @@ void FileElementCollectionItem::setUpIcon() else { if (isDir()) { - setIcon(QET::Icons::Folder); + setIcon(m_qet_directory_unreadable ? unreadableFolderIcon() + : QET::Icons::Folder); } else { if (m_path.endsWith(".qetmak")) { setIcon(QIcon()); From f83aa3f1bc18823e6b9ab6a93cf320b09f58d916 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Wed, 5 Aug 2026 08:29:16 +1200 Subject: [PATCH 3/3] Fix stack-overflow crash in FileElementCollectionItem::setUpIcon() The unconditional early return was narrowed to non-directories only, so that the just-added warning badge could be picked up once setUpData() resolved m_qet_directory_unreadable asynchronously. But every directory then called setIcon() on every single data(Qt::DecorationRole) query -- not just once -- and QStandardItem::setIcon() -> setData() emits dataChanged() unconditionally (QIcon has no equality check to suppress it). QTreeView handles dataChanged() by recomputing the row's size hint, which re-enters data() for the same index, calling setIcon() again: unbounded mutual recursion, confirmed by an isolated reproduction to overflow the stack in a single frame (100k+ frames) well before the first paint completes. Matches plc-user's report of a segfault right as the elements tree begins drawing. The race the guard was widened for doesn't actually occur: ElementsCollectionModel only attaches itself to the tree view (the only way data() becomes reachable) from loadingFinished(), which fires after the QtConcurrent::map over every item -- this one included -- has already finished. m_qet_directory_unreadable is therefore always final before setUpIcon() can run for the first time, so the plain, always-only- once guard is sufficient and the badge still works correctly. --- .../fileelementcollectionitem.cpp | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/sources/ElementsCollection/fileelementcollectionitem.cpp b/sources/ElementsCollection/fileelementcollectionitem.cpp index ccd58d0b9..5e5c7e844 100644 --- a/sources/ElementsCollection/fileelementcollectionitem.cpp +++ b/sources/ElementsCollection/fileelementcollectionitem.cpp @@ -430,13 +430,20 @@ void FileElementCollectionItem::setUpData() */ void FileElementCollectionItem::setUpIcon() { - // Directories are cheap to (re-)decide: no early return for them. - // setUpData() -- which resolves m_qet_directory_unreadable via - // localName() -- runs asynchronously (QtConcurrent::map), so this can - // be called for a directory before that result is known; without - // this, the plain folder icon would get cached by the guard below - // and the warning badge would never appear for that item. - if (!isDir() && !icon().isNull()) + // Must return unconditionally once an icon is set: setIcon() calls + // setData(), which emits dataChanged() regardless of whether the new + // icon differs from the old one (QIcon has no meaningful equality). + // QTreeView responds to dataChanged() by recomputing the row's size + // hint, which re-enters data() for this same index -- so without this + // guard, any repeated setIcon() here recurses until the stack + // overflows. Confirmed by crash report on PR #633. + // + // This item's m_qet_directory_unreadable is already final by the time + // this can run at all: ElementsCollectionModel only attaches itself + // to the tree view (making data() reachable) from loadingFinished(), + // which fires after the QtConcurrent::map over every item -- this one + // included -- has completed. So there is no race to work around here. + if (!icon().isNull()) return; if (isCollectionRoot()) {