From ede7bd99e1dabe48d0c43be913b5177e6f7a3b77 Mon Sep 17 00:00:00 2001 From: Beat Hangartner Date: Thu, 8 Oct 2026 06:04:35 +0200 Subject: [PATCH] Never use a folio grid step below 1 from the settings The folio grid steps, diagrameditor/Xgrid and Ygrid, are read from the settings in six places with a plain toInt() and used as they come. The preferences page cannot store anything below 1, but a hand-edited or damaged settings file can hold 0, a negative number or text: - Diagram::snapToGrid() divides by the step, so 0 is a SIGFPE on the first click that places a symbol; - Diagram::drawBackground() runs "while (g_x % xGrid)" and then loops "gx += xGrid", so 0 crashes every repaint and a negative step never ends; - the paste, duplicate and align paths divide by it or multiply with it. Add foliogrid.h, a header-only helper: FolioGrid::step() reads a settings entry and returns the built-in step (Diagram::xGrid, 10) when the entry is missing, not a number, below 1 or above what an int holds (QVariant::toInt() wraps such a value around). Every reader of the two keys goes through it; the settings page, which only writes the spin box values, is unchanged. No file-format change, and no change for any step the preferences page can produce. Tests: tst_foliogrid covers the helper: 1, 10, "7", int max are kept; missing, 0, -5, "ten", "", "nan", 99999999999 (as text and as a number) and int max + 1 fall back; 7.9 rounds to 8 as before; and the two settings keys through a QSettings scope of the test's own. The helper is new, so the test cannot fail on master; the six readers are the replacements in the diff. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Beat Hangartner --- sources/diagram.cpp | 19 ++-- sources/diagramevent/diagrameventaddpaste.cpp | 13 +-- sources/diagramview.cpp | 7 +- sources/foliogrid.h | 54 ++++++++++ sources/undocommand/alignselectioncommand.cpp | 5 +- tests/qttest/CMakeLists.txt | 7 ++ tests/qttest/tst_foliogrid.cpp | 99 +++++++++++++++++++ 7 files changed, 178 insertions(+), 26 deletions(-) create mode 100644 sources/foliogrid.h create mode 100644 tests/qttest/tst_foliogrid.cpp diff --git a/sources/diagram.cpp b/sources/diagram.cpp index 978614106..75a9051ba 100644 --- a/sources/diagram.cpp +++ b/sources/diagram.cpp @@ -45,6 +45,7 @@ #include "diagramsortkeys.h" #include "itemgroups.h" #include "textgrid.h" +#include "foliogrid.h" #include #include #include @@ -339,10 +340,8 @@ void Diagram::drawBackground(QPainter *p, const QRectF &r) { p -> setBrush(Qt::NoBrush); - int xGrid = settings.value(QStringLiteral("diagrameditor/Xgrid"), - Diagram::xGrid).toInt(); - int yGrid = settings.value(QStringLiteral("diagrameditor/Ygrid"), - Diagram::yGrid).toInt(); + const int xGrid = FolioGrid::step(settings.value(FolioGrid::x_key), Diagram::xGrid); + const int yGrid = FolioGrid::step(settings.value(FolioGrid::y_key), Diagram::yGrid); qreal limit_x = rect.x() + rect.width(); qreal limit_y = rect.y() + rect.height(); @@ -2946,10 +2945,8 @@ DiagramPosition Diagram::convertPosition(const QPointF &pos) { QPointF Diagram::snapToGrid(const QPointF &p) { QSettings settings; - int xGrid = settings.value(QStringLiteral("diagrameditor/Xgrid"), - Diagram::xGrid).toInt(); - int yGrid = settings.value(QStringLiteral("diagrameditor/Ygrid"), - Diagram::yGrid).toInt(); + const int xGrid = FolioGrid::step(settings.value(FolioGrid::x_key), Diagram::xGrid); + const int yGrid = FolioGrid::step(settings.value(FolioGrid::y_key), Diagram::yGrid); //Return a point rounded to the nearest pixel if (QApplication::keyboardModifiers().testFlag(Qt::ControlModifier)) @@ -2981,10 +2978,8 @@ QPointF Diagram::snapToTextGrid(const QPointF &p) : settings.value(TextGrid::settings_key, 1).toReal(); return TextGrid::snap(p, - settings.value(QStringLiteral("diagrameditor/Xgrid"), - Diagram::xGrid).toInt(), - settings.value(QStringLiteral("diagrameditor/Ygrid"), - Diagram::yGrid).toInt(), + FolioGrid::step(settings.value(FolioGrid::x_key), Diagram::xGrid), + FolioGrid::step(settings.value(FolioGrid::y_key), Diagram::yGrid), divisor); } diff --git a/sources/diagramevent/diagrameventaddpaste.cpp b/sources/diagramevent/diagrameventaddpaste.cpp index 6b1491e52..aa7da7f7d 100644 --- a/sources/diagramevent/diagrameventaddpaste.cpp +++ b/sources/diagramevent/diagrameventaddpaste.cpp @@ -19,6 +19,7 @@ #include "../autoNum/ui/pastenumberingimport.h" #include "../diagram.h" +#include "../foliogrid.h" #include "../diagramcommands.h" #include "../qetapp.h" #include "../qetdiagrameditor.h" @@ -108,10 +109,8 @@ } const QPointF top_left = items_rect.topLeft(); QSettings settings; - const int xGrid = settings.value(QStringLiteral("diagrameditor/Xgrid"), - Diagram::xGrid).toInt(); - const int yGrid = settings.value(QStringLiteral("diagrameditor/Ygrid"), - Diagram::yGrid).toInt(); + const int xGrid = FolioGrid::step(settings.value(FolioGrid::x_key), Diagram::xGrid); + const int yGrid = FolioGrid::step(settings.value(FolioGrid::y_key), Diagram::yGrid); const auto snapGrid = [xGrid, yGrid](const QPointF &p) -> QPointF { return QPointF( qRound(p.x() / xGrid) * xGrid, @@ -287,10 +286,8 @@ void DiagramEventAddPaste::showHint() void DiagramEventAddPaste::moveTo(const QPointF &scene_pos) { QSettings settings; - const int xGrid = settings.value(QStringLiteral("diagrameditor/Xgrid"), - Diagram::xGrid).toInt(); - const int yGrid = settings.value(QStringLiteral("diagrameditor/Ygrid"), - Diagram::yGrid).toInt(); + const int xGrid = FolioGrid::step(settings.value(FolioGrid::x_key), Diagram::xGrid); + const int yGrid = FolioGrid::step(settings.value(FolioGrid::y_key), Diagram::yGrid); const auto snapGrid = [xGrid, yGrid](const QPointF &p) -> QPointF { return QPointF( diff --git a/sources/diagramview.cpp b/sources/diagramview.cpp index d842256a0..bb5772fcd 100644 --- a/sources/diagramview.cpp +++ b/sources/diagramview.cpp @@ -40,6 +40,7 @@ #include "utils/conductorcreator.h" #include "undocommand/addgraphicsobjectcommand.h" #include "diagram.h" +#include "foliogrid.h" #include "diagramcontexttoolbar.h" #include "diagramgestureoverlay.h" #include "gesturesettings.h" @@ -643,10 +644,8 @@ void DiagramView::duplicate(const QPoint &stepOffset) if (selection.isEmpty()) return; QSettings settings; - const int x_grid = settings.value(QStringLiteral("diagrameditor/Xgrid"), - Diagram::xGrid).toInt(); - const int y_grid = settings.value(QStringLiteral("diagrameditor/Ygrid"), - Diagram::yGrid).toInt(); + const int x_grid = FolioGrid::step(settings.value(FolioGrid::x_key), Diagram::xGrid); + const int y_grid = FolioGrid::step(settings.value(FolioGrid::y_key), Diagram::yGrid); const QPointF offset(stepOffset.x() * x_grid, stepOffset.y() * y_grid); // Mirrors copy(), but does not touch the system clipboard: Ctrl+D diff --git a/sources/foliogrid.h b/sources/foliogrid.h new file mode 100644 index 000000000..9fb3fe772 --- /dev/null +++ b/sources/foliogrid.h @@ -0,0 +1,54 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with QElectroTech. If not, see . +*/ +#ifndef FOLIOGRID_H +#define FOLIOGRID_H + +#include +#include +#include + +/** + The folio grid: the step, in pixels, that symbols snap to. Both steps + are kept in the settings; the preferences page only offers 1 and up, + but a hand-edited or damaged settings file can hold 0, a negative + number or text, and the code that divides by the step or loops over it + must never see such a value. +*/ +namespace FolioGrid +{ + /// QSettings keys holding the two steps. + inline const QString x_key{QStringLiteral("diagrameditor/Xgrid")}; + inline const QString y_key{QStringLiteral("diagrameditor/Ygrid")}; + + /** + @return the step held in @p value, a settings entry; or @p fallback, + the built-in step, when the entry is missing, not a number, less + than 1, or more than an int holds (QVariant::toInt() wraps such a + number around instead of failing). + */ + inline int step(const QVariant &value, int fallback) + { + bool ok = false; + const qlonglong s = value.toLongLong(&ok); + if (!ok || s < 1 || s > std::numeric_limits::max()) + return fallback; + return int(s); + } +} + +#endif // FOLIOGRID_H diff --git a/sources/undocommand/alignselectioncommand.cpp b/sources/undocommand/alignselectioncommand.cpp index f05267066..469358b1b 100644 --- a/sources/undocommand/alignselectioncommand.cpp +++ b/sources/undocommand/alignselectioncommand.cpp @@ -20,6 +20,7 @@ #include "../QPropertyUndoCommand/qpropertyundocommand.h" #include "../alignment.h" #include "../diagram.h" +#include "../foliogrid.h" #include "../diagramcontent.h" #include "../itemgroups.h" #include "../qetgraphicsitem/diagramimageitem.h" @@ -121,8 +122,8 @@ AlignSelectionCommand::AlignSelectionCommand(Diagram *diagram, Mode mode, QUndoC m_locked_count = dc.removeNonMovableItems(); QSettings settings; - const int x_grid = settings.value(QStringLiteral("diagrameditor/Xgrid"), Diagram::xGrid).toInt(); - const int y_grid = settings.value(QStringLiteral("diagrameditor/Ygrid"), Diagram::yGrid).toInt(); + const int x_grid = FolioGrid::step(settings.value(FolioGrid::x_key), Diagram::xGrid); + const int y_grid = FolioGrid::step(settings.value(FolioGrid::y_key), Diagram::yGrid); const qreal text_divisor = settings.value(TextGrid::settings_key, 1).toReal(); //Each kind goes where dragging it would have left it: symbols, diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 7e10b46fa..9dd2becde 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -128,6 +128,13 @@ add_test(NAME tst_textgrid COMMAND tst_textgrid) target_include_directories(tst_textgrid PRIVATE ${QET_DIR}/sources) target_link_libraries(tst_textgrid PRIVATE Qt::Test) +# foliogrid.h is header-only: the folio grid step as read from the +# settings, never 0 or negative, so no reader can divide by it or loop on it. +add_executable(tst_foliogrid tst_foliogrid.cpp) +target_include_directories(tst_foliogrid PRIVATE ${QET_DIR}/sources) +target_link_libraries(tst_foliogrid PRIVATE Qt::Test Qt::Core) +add_test(NAME tst_foliogrid COMMAND tst_foliogrid) + # textlines.h is header-only: the lines of a text as they are laid out, # for the exports that write a text line by line. add_executable(tst_textlines tst_textlines.cpp) diff --git a/tests/qttest/tst_foliogrid.cpp b/tests/qttest/tst_foliogrid.cpp new file mode 100644 index 000000000..b4f7a0462 --- /dev/null +++ b/tests/qttest/tst_foliogrid.cpp @@ -0,0 +1,99 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with QElectroTech. If not, see . +*/ +#include "foliogrid.h" + +#include +#include + +/** + FolioGrid::step() -- the folio grid step as read from the settings. + Every reader of diagrameditor/Xgrid and Ygrid goes through it, so a + settings file holding 0 (a division by zero in Diagram::snapToGrid() + and an endless "% 0" in drawBackground()), a negative number (an + endless loop) or text can no longer reach the code that uses the step. +*/ +class tst_foliogrid : public QObject +{ + Q_OBJECT + +private slots: + void usableValuesAreKept_data() + { + QTest::addColumn("value"); + QTest::addColumn("expected"); + QTest::newRow("one") << QVariant(1) << 1; + QTest::newRow("default") << QVariant(10) << 10; + QTest::newRow("text number") << QVariant(QStringLiteral("7")) << 7; + QTest::newRow("large") << QVariant(100000) << 100000; + QTest::newRow("int max") << QVariant(2147483647) << 2147483647; + } + + void usableValuesAreKept() + { + QFETCH(QVariant, value); + QFETCH(int, expected); + QCOMPARE(FolioGrid::step(value, 10), expected); + } + + void unusableValuesFallBack_data() + { + QTest::addColumn("value"); + QTest::newRow("missing") << QVariant(); + QTest::newRow("zero") << QVariant(0); + QTest::newRow("negative") << QVariant(-5); + QTest::newRow("text") << QVariant(QStringLiteral("ten")); + QTest::newRow("empty") << QVariant(QString()); + QTest::newRow("nan") << QVariant(QStringLiteral("nan")); + QTest::newRow("too large") << QVariant(QStringLiteral("99999999999")); + QTest::newRow("too large number") << QVariant(qlonglong(99999999999)); + QTest::newRow("int max + 1") << QVariant(qlonglong(2147483648)); + } + + void unusableValuesFallBack() + { + QFETCH(QVariant, value); + QCOMPARE(FolioGrid::step(value, 10), 10); + QCOMPARE(FolioGrid::step(value, 20), 20); + } + + // A decimal in the file is rounded to a whole number, as the plain + // QVariant::toInt() read did before. + void decimalIsRoundedLikeBefore() + { + QCOMPARE(FolioGrid::step(QVariant(7.9), 10), 8); + QCOMPARE(FolioGrid::step(QVariant(0.4), 10), 10); + } + + // Through a real settings file, in a scope of this test's own. + void readsTheSettingsKeys() + { + QCoreApplication::setOrganizationName(QStringLiteral("QElectroTech-tst_foliogrid")); + QCoreApplication::setApplicationName(QStringLiteral("tst_foliogrid")); + QSettings settings; + settings.clear(); + QCOMPARE(FolioGrid::step(settings.value(FolioGrid::x_key), 10), 10); + settings.setValue(FolioGrid::x_key, 0); + settings.setValue(FolioGrid::y_key, 15); + QCOMPARE(FolioGrid::step(settings.value(FolioGrid::x_key), 10), 10); + QCOMPARE(FolioGrid::step(settings.value(FolioGrid::y_key), 10), 15); + settings.clear(); + } +}; + +QTEST_GUILESS_MAIN(tst_foliogrid) +#include "tst_foliogrid.moc"