Skip to content

Commit 978e3be

Browse files
Goober5000claude
andcommitted
add briefing icon ID collision validation to qtFRED
FRED2 rejects an icon ID edit that would collide with another icon, warning and reverting the field, but qtFRED's setIconId() applied the value unconditionally. Two icons could be given the same ID in a stage (making find_icon and forward propagation ambiguous), or a global rename could merge two distinct icon chains by reusing a downstream ID. setIconId() now mirrors FRED2: - An ID is a single-icon concept, so it operates on the current icon rather than the whole selection (applying one ID to several selected icons would itself create a same-stage collision). - Rejects an ID already used by another icon in the current stage. - When propagating forward (Change Locally off), rejects an ID already used in a later stage, since the rename would merge two icons. - Otherwise applies the change and renames the icon's forward copies by matching the old ID, respecting Change Locally. It returns false when a change is rejected; the dialog resets the spin box to the model's value. The spin box is switched to non-keyboard- tracking so validation runs when the edit is committed (focus-out/Enter) rather than on every keystroke, avoiding a modal warning on a transient value mid-typing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 48db13d commit 978e3be

3 files changed

Lines changed: 71 additions & 6 deletions

File tree

qtfred/src/mission/dialogs/BriefingEditorDialogModel.cpp

Lines changed: 59 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -785,13 +785,68 @@ int BriefingEditorDialogModel::getIconId() const
785785
return s.icons[_currentIcon].id;
786786
}
787787

788-
void BriefingEditorDialogModel::setIconId(int id)
788+
bool BriefingEditorDialogModel::setIconId(int id)
789789
{
790-
// briefing icon ids must never be negative
790+
auto& briefing = _wipBriefings[_currentTeam];
791+
if (briefing.num_stages <= 0 || _currentStage < 0 || _currentStage >= briefing.num_stages)
792+
return true;
793+
794+
auto& stage = briefing.stages[_currentStage];
795+
if (_currentIcon < 0 || _currentIcon >= stage.num_icons)
796+
return true;
797+
798+
// an id applies to a single icon, so operate on the current icon rather than the
799+
// whole selection; applying one id to several icons would itself create a collision
800+
const int oldId = stage.icons[_currentIcon].id;
801+
if (id == oldId)
802+
return true; // no change
803+
804+
// briefing icon ids must never be negative (the spin box also enforces this)
791805
if (id < 0)
792-
return;
806+
return false;
807+
808+
// an icon id must be unique within its stage
809+
for (int i = 0; i < stage.num_icons; ++i) {
810+
if (i != _currentIcon && stage.icons[i].id == id) {
811+
QMessageBox::warning(nullptr,
812+
tr("Icon ID"),
813+
tr("Icon ID %1 is already used by another icon in this stage. The ID has not been changed.").arg(id));
814+
return false;
815+
}
816+
}
793817

794-
applyToSelectedIconsCurrentAndForward([&](brief_icon& ic) { modify(ic.id, id); });
818+
// when propagating forward, the new id must not already be used by a different icon in a
819+
// later stage, or the global rename would merge two distinct icon chains into one
820+
if (!_changeLocally) {
821+
for (int st = _currentStage + 1; st < briefing.num_stages; ++st) {
822+
const auto& s = briefing.stages[st];
823+
for (int i = 0; i < s.num_icons; ++i) {
824+
if (s.icons[i].id == id) {
825+
QMessageBox::warning(nullptr,
826+
tr("Icon ID"),
827+
tr("Icon ID %1 is already used in a later stage. You can only change to that ID "
828+
"locally. The ID has not been changed.").arg(id));
829+
return false;
830+
}
831+
}
832+
}
833+
}
834+
835+
// apply the change to the current icon, and (unless local-only) to the same icon in later stages
836+
stage.icons[_currentIcon].id = id;
837+
if (!_changeLocally) {
838+
for (int st = _currentStage + 1; st < briefing.num_stages; ++st) {
839+
auto& s = briefing.stages[st];
840+
for (int i = 0; i < s.num_icons; ++i) {
841+
if (s.icons[i].id == oldId)
842+
s.icons[i].id = id;
843+
}
844+
}
845+
}
846+
847+
set_modified();
848+
modelChanged();
849+
return true;
795850
}
796851

797852
SCP_string BriefingEditorDialogModel::getIconLabel() const

qtfred/src/mission/dialogs/BriefingEditorDialogModel.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ class BriefingEditorDialogModel : public AbstractDialogModel {
7474
vec3d getIconPosition() const;
7575
void setIconPosition(const vec3d& pos);
7676
int getIconId() const;
77-
void setIconId(int id);
77+
// returns false if the requested id was rejected (e.g. it collides with another icon)
78+
bool setIconId(int id);
7879
SCP_string getIconLabel() const;
7980
void setIconLabel(const SCP_string& text);
8081
SCP_string getIconCloseupLabel() const;

qtfred/src/ui/dialogs/BriefingEditorDialog.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ BriefingEditorDialog::BriefingEditorDialog(FredView* parent, EditorViewport* vie
8989
ui->iconCloseupLabelLineEdit->setMaxLength(MAX_LABEL_LEN - 1);
9090
ui->voiceFileLineEdit->setMaxLength(MAX_FILENAME_LEN - 1);
9191

92+
// validate the icon id when the edit is committed (focus-out/Enter) rather than on every
93+
// keystroke, so a transient value while typing can't pop a collision warning
94+
ui->iconIdSpinBox->setKeyboardTracking(false);
95+
9296
setupMapWidget();
9397
initializeUi();
9498
updateUi();
@@ -562,7 +566,12 @@ void BriefingEditorDialog::on_disableGridCheckBox_toggled(bool checked)
562566

563567
void BriefingEditorDialog::on_iconIdSpinBox_valueChanged(int arg1)
564568
{
565-
_model->setIconId(arg1);
569+
if (!_model->setIconId(arg1)) {
570+
// the id was rejected; reset the spin box to the model's current value
571+
ui->iconIdSpinBox->blockSignals(true);
572+
ui->iconIdSpinBox->setValue(_model->getIconId());
573+
ui->iconIdSpinBox->blockSignals(false);
574+
}
566575
}
567576

568577
void BriefingEditorDialog::on_iconLabelLineEdit_textChanged(const QString& string)

0 commit comments

Comments
 (0)