From 68e75b4c55cfb60633e78f7c849af106a26ebd50 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Fri, 31 Jul 2026 20:39:04 +1200 Subject: [PATCH] Fix: potential-selector dialog can't actually be cancelled (#581) PotentialSelectorDialog::chosenProperties() built an OK-only dialog and discarded exec()'s return value entirely: dialog.exec(); for (QRadioButton *b : H.keys()) { if (b->isChecked()) return H.value(b); } return ConductorProperties(); Escape and the window close button already trigger QDialog::reject() on a plain QDialog, but since the result was never checked, dismissing the dialog without picking anything just silently returned blank ConductorProperties() -- the same value returned when a real potential was chosen but happened to produce empty properties. There was no way to distinguish "the user cancelled" from "the user chose an empty potential", so the caller always proceeded as if a choice had been made. Add a real Cancel button, check dialog.exec() == QDialog::Accepted, and report cancellation through a new optional `bool *cancelled` out-parameter. Also pre-select the first entry, closing a related gap where clicking OK without ever touching a radio button hit the exact same "silently returns blank properties" failure mode. Thread the result through ConductorCreator::setUpPropertieToUse() (now returning bool) so the calling constructor aborts and creates no conductors at all when the user cancels, instead of proceeding with blank properties. The sibling constructor-based PotentialSelectorDialog (used for conductor/report potential linking, a separate flow) already gates its side effects behind on_buttonBox_accepted(), so cancelling it was already safe -- gave it a visible Cancel button too for consistency while touching this file, no behavior change there. Verified with real Qt event simulation (QTest::mouseClick/keyClick) against the exact new dialog-building logic: clicking Cancel and pressing Escape both correctly report cancellation with empty properties; clicking OK untouched returns the pre-selected first entry; selecting the second option then OK returns that selection. See discussion #581. Co-Authored-By: Claude Sonnet 5 --- sources/ui/potentialselectordialog.cpp | 23 ++++++++++++++++++--- sources/ui/potentialselectordialog.h | 9 +++++++-- sources/ui/potentialselectordialog.ui | 2 +- sources/utils/conductorcreator.cpp | 28 +++++++++++++++++--------- sources/utils/conductorcreator.h | 2 +- 5 files changed, 48 insertions(+), 16 deletions(-) diff --git a/sources/ui/potentialselectordialog.cpp b/sources/ui/potentialselectordialog.cpp index f1e1ad963..2a39252c7 100644 --- a/sources/ui/potentialselectordialog.cpp +++ b/sources/ui/potentialselectordialog.cpp @@ -188,8 +188,11 @@ class LinkReportPotentialSelector : public AbstractPotentialSelector //### END PRIVATE CLASS ###// -ConductorProperties PotentialSelectorDialog::chosenProperties(QList list, QWidget *widget) +ConductorProperties PotentialSelectorDialog::chosenProperties(QList list, QWidget *widget, bool *cancelled) { + if (cancelled) + *cancelled = false; + if (list.isEmpty()) { return ConductorProperties() ; } else if (list.size() == 1) { @@ -222,11 +225,25 @@ ConductorProperties PotentialSelectorDialog::chosenProperties(QListsetChecked(true); + + QDialogButtonBox *button_box = new QDialogButtonBox( + QDialogButtonBox::Ok | QDialogButtonBox::Cancel, &dialog); layout.addWidget(button_box); connect(button_box, &QDialogButtonBox::accepted, &dialog, &QDialog::accept); + connect(button_box, &QDialogButtonBox::rejected, &dialog, &QDialog::reject); + + if (dialog.exec() != QDialog::Accepted) { + if (cancelled) + *cancelled = true; + return ConductorProperties(); + } - dialog.exec(); for (QRadioButton *b : H.keys()) { if(b->isChecked()) { return H.value(b); diff --git a/sources/ui/potentialselectordialog.h b/sources/ui/potentialselectordialog.h index 64d2f2bd8..d0217e8d3 100644 --- a/sources/ui/potentialselectordialog.h +++ b/sources/ui/potentialselectordialog.h @@ -64,7 +64,11 @@ namespace Ui { the static function chosenProperties, open a dialog who ask user to make a choice between the given - properties + properties. If the dialog is cancelled (Cancel button, Escape, or the + window's close button) and @a cancelled is non-null, *cancelled is set + to true and an empty ConductorProperties() is returned; callers that + care about a real cancellation (as opposed to "no properties to choose + from") should check it rather than relying on the returned value alone. */ class PotentialSelectorDialog : public QDialog { @@ -73,7 +77,8 @@ class PotentialSelectorDialog : public QDialog public: static ConductorProperties chosenProperties( QList list, - QWidget *parent = nullptr); + QWidget *parent = nullptr, + bool *cancelled = nullptr); public: explicit PotentialSelectorDialog( diff --git a/sources/ui/potentialselectordialog.ui b/sources/ui/potentialselectordialog.ui index dd6a31d0a..c914f1cec 100644 --- a/sources/ui/potentialselectordialog.ui +++ b/sources/ui/potentialselectordialog.ui @@ -55,7 +55,7 @@ Veuillez choisir les propriétées à appliquer au nouveau potentiel. Qt::Horizontal - QDialogButtonBox::Ok + QDialogButtonBox::Cancel|QDialogButtonBox::Ok diff --git a/sources/utils/conductorcreator.cpp b/sources/utils/conductorcreator.cpp index 6e088a560..167a2a37d 100644 --- a/sources/utils/conductorcreator.cpp +++ b/sources/utils/conductorcreator.cpp @@ -42,8 +42,10 @@ ConductorCreator::ConductorCreator(Diagram *d, QList terminals_list) return; } m_properties = m_terminals_list.first()->diagram()->defaultConductorProperties; - - setUpPropertieToUse(); + + if (!setUpPropertieToUse()) { + return; + } Terminal *hub_terminal = hubTerminal(); d->undoStack().beginMacro(QObject::tr("Création de conducteurs")); @@ -95,12 +97,15 @@ void ConductorCreator::create(Diagram *d, const QPolygonF &polygon) /** @brief ConductorCreator::propertieToUse - @return the conductor properties to use for the new conductors. + @return true if the caller should proceed with conductor creation, + false if the user cancelled the potential-selection dialog (in which + case no properties were chosen and creation must be aborted rather + than proceeding with blank/default properties). */ -void ConductorCreator::setUpPropertieToUse() +bool ConductorCreator::setUpPropertieToUse() { QList potentials = existingPotential(); - + //There is an existing potential //we get one of them if (!potentials.isEmpty()) @@ -111,8 +116,12 @@ void ConductorCreator::setUpPropertieToUse() for(Conductor *c : potentials) { cp_list.append(c->properties()); } - - m_properties = PotentialSelectorDialog::chosenProperties(cp_list); + + bool cancelled = false; + m_properties = PotentialSelectorDialog::chosenProperties(cp_list, nullptr, &cancelled); + if (cancelled) { + return false; + } for (Conductor *c : potentials) { if (c->properties() == m_properties) { m_sequential_number = c->sequenceNum(); @@ -124,11 +133,12 @@ void ConductorCreator::setUpPropertieToUse() m_properties = potentials.first()->properties(); m_sequential_number = potentials.first()->sequenceNum(); } - return; + return true; } - + //get a new properties ConductorAutoNumerotation::newProperties(m_terminals_list.first()->diagram(), m_properties, m_sequential_number); + return true; } /** diff --git a/sources/utils/conductorcreator.h b/sources/utils/conductorcreator.h index dd3ad7517..515081b1f 100644 --- a/sources/utils/conductorcreator.h +++ b/sources/utils/conductorcreator.h @@ -40,7 +40,7 @@ class ConductorCreator static void create(Diagram *d, const QPolygonF &polygon); private: - void setUpPropertieToUse(); + bool setUpPropertieToUse(); QList existingPotential(); Terminal *hubTerminal();