shield system dialog ux clarity (#7473)

Author: MjnMixael

qtfred/help-src/doc/dialogs/ShieldSystemDialog.html

@@ -10,20 +10,28 @@
 <h1>Shield System</h1>
 <p>Opens via <strong>Editors &rsaquo; Shield System</strong>.</p>
 <p>A convenience tool for enabling or disabling shields on ships in bulk, by ship class
-or by IFF team. When applied, the dialog directly sets the shield flag on every
-existing ship in the mission; it is not a runtime override. If you add new ships
-after using this dialog, reopen it and apply again to include them.</p>
+or by IFF team. Each side of the dialog is an independent batch operation; pressing
+an <strong>Apply</strong> button writes the shield flag on every existing ship of the
+chosen class or team and also records the choice as the default for ships of that
+class or team placed afterwards. Close the dialog with the window's close button when
+you are done &mdash; there is no separate save step.</p>
 
 <h2>By ship class (left panel)</h2>
-<p>Select a ship class from the dropdown and choose
-<strong>Has shield system</strong> or <strong>No shield system</strong>. Applying
-sets the flag on all ships of that class currently in the mission.</p>
+<p>Pick a ship class from the dropdown. The <em>Currently:</em> label shows the
+current state for that class &mdash; <em>Has shields</em>, <em>No shields</em>, or
+<em>Mixed</em> when ships of the class disagree. Choose
+<strong>Has shield system</strong> or <strong>No shield system</strong> and click
+<strong>Apply to current class</strong> to commit. When the state is <em>Mixed</em>,
+neither radio is preselected and the Apply button stays disabled until you pick a
+value, so you cannot flatten per-ship customisation by accident.</p>
 
 <h2>By IFF team (right panel)</h2>
-<p>Select a team from the dropdown and choose
-<strong>Has shield system</strong> or <strong>No shield system</strong>. Applying
-sets the flag on all ships belonging to that team currently in the mission.</p>
+<p>Works the same way but operates on every ship of the selected team. Use
+<strong>Apply to current team</strong> to commit.</p>
 
+<div class="note">Each Apply only touches the axis whose button you pressed. The
+other side's selection is not affected, so you can switch back and forth and apply
+class- and team-level changes independently in the same session.</div>
 <div class="note">When both a ship class setting and a team setting apply to the same
 ship, the ship class setting takes precedence.</div>
 <div class="note">The Shield System dialog, the <a href="ShipEditorDialog.html">Ship
@@ -32,8 +40,9 @@ <h2>By IFF team (right panel)</h2>
 per-ship shield flag. There is no fixed precedence between them; whichever is
 applied last takes effect. A typical workflow is to use this dialog to establish a
 baseline across the mission, then make individual adjustments per ship in the Ship
-Editor. If you later reopen this dialog and a team or class shows as
-<em>Mixed</em> (some ships differ), applying without changing that setting will
-leave those individual ship values intact.</div>
+Editor. Reopening this dialog re-reads the state from the ships themselves, so if
+individual ships have been edited since the last bulk apply, the class or team will
+show as <em>Mixed</em>; applying without changing the radio choice is not possible
+in that state, which protects per-ship edits from being silently overwritten.</div>
 </body>
 </html>

qtfred/src/mission/Editor.cpp

@@ -1972,13 +1972,6 @@ SCP_vector<SCP_string> Editor::get_docking_list(int model_index) {
 	return out;
 }
 
-bool Editor::compareShieldSysData(const SCP_vector<GlobalShieldStatus>& teams, const SCP_vector<GlobalShieldStatus>& types) const {
-	Assertion(Shield_sys_teams.size() == teams.size(), "Mismatched shield data from global shield dialog!");
-	Assertion(Shield_sys_types.size() == types.size(), "Mismatched shield data from global shield dialog!");
-
-	return (Shield_sys_teams == teams) && (Shield_sys_types == types);
-}
-
 void Editor::exportShieldSysData(SCP_vector<GlobalShieldStatus>& teams, SCP_vector<GlobalShieldStatus>& types) const {
 	teams = Shield_sys_teams;
 	types = Shield_sys_types;

qtfred/src/mission/Editor.h

@@ -261,7 +261,6 @@ class Editor : public QObject {
 
 	static SCP_vector<SCP_string> get_docking_list(int model_index);
 
-	bool compareShieldSysData(const SCP_vector<GlobalShieldStatus>& teams, const SCP_vector<GlobalShieldStatus>& types) const;
 	void exportShieldSysData(SCP_vector<GlobalShieldStatus>& teams, SCP_vector<GlobalShieldStatus>& types) const;
 	void importShieldSysData(const SCP_vector<GlobalShieldStatus>& teams, const SCP_vector<GlobalShieldStatus>& types);
 	void normalizeShieldSysData();

qtfred/src/mission/dialogs/ShieldSystemDialogModel.cpp

@@ -15,34 +15,32 @@ void ShieldSystemDialogModel::initializeData() {
 	_currTeam = 0;
 	_currType = 0;
 
-	_editor->normalizeShieldSysData();
-
-	_editor->exportShieldSysData(_teams, _types);
-
 	for (const auto& info : Ship_info) {
 		_shipTypeOptions.emplace_back(info.name);
 	}
 
 	for (const auto& iff : Iff_info) {
 		_teamOptions.emplace_back(iff.iff_name);
 	}
+
+	refresh();
+}
+
+void ShieldSystemDialogModel::refresh() {
+	_editor->normalizeShieldSysData();
+	_editor->exportShieldSysData(_teams, _types);
 	_modified = false;
 }
 
 bool ShieldSystemDialogModel::apply() {
-	if (query_modified()) {
-		_editor->importShieldSysData(_teams, _types);
-	}
+	// Each Apply button commits immediately; nothing to do at dialog close.
 	return true;
 }
+
 void ShieldSystemDialogModel::reject() {
 	// nothing to do
 }
 
-bool ShieldSystemDialogModel::query_modified() const {
-	return !_editor->compareShieldSysData(_teams, _types);
-}
-
 int ShieldSystemDialogModel::getCurrentTeam() const
 {
 	return _currTeam;
@@ -54,12 +52,12 @@ int ShieldSystemDialogModel::getCurrentShipType() const
 void ShieldSystemDialogModel::setCurrentTeam(int team)
 {
 	Assertion(SCP_vector_inbounds(Iff_info, team), "Team index %d out of bounds (size: %d)", team, static_cast<int>(Iff_info.size()));
-	modify(_currTeam, team);
+	_currTeam = team;
 }
 void ShieldSystemDialogModel::setCurrentShipType(int type)
 {
 	Assertion(type >= 0 && type < MAX_SHIP_CLASSES, "Ship class index %d is invalid!", type); // NOLINT(readability-simplify-boolean-expr)
-	modify(_currType, type);
+	_currType = type;
 }
 
 GlobalShieldStatus ShieldSystemDialogModel::getCurrentTeamShieldSys() const
@@ -70,25 +68,23 @@ GlobalShieldStatus ShieldSystemDialogModel::getCurrentTypeShieldSys() const
 {
 	return _types[_currType];
 }
-void ShieldSystemDialogModel::setCurrentTeamShieldSys(bool value)
-{
-	// UI can only turn shields on or off, so just map to the appropriate enum value
 
-	if (value) {
-		modify(_teams[_currTeam], GlobalShieldStatus::HasShields);
-	} else {
-		modify(_teams[_currTeam], GlobalShieldStatus::NoShields);
-	}
-}
-void ShieldSystemDialogModel::setCurrentTypeShieldSys(bool value)
+void ShieldSystemDialogModel::applyTeam(bool hasShields)
 {
-	// UI can only turn shields on or off, so just map to the appropriate enum value
+	// Mutate only the currently selected team entry. Other entries already
+	// reflect current per-ship state (refresh() ran on open / after last
+	// apply), so importShieldSysData is a no-op for them. Mixed entries
+	// stay Mixed, Has/No entries already match each ship's flag.
+	_teams[_currTeam] = hasShields ? GlobalShieldStatus::HasShields : GlobalShieldStatus::NoShields;
+	_editor->importShieldSysData(_teams, _types);
+	refresh();
+}
 
-	if (value) {
-		modify(_types[_currType], GlobalShieldStatus::HasShields);
-	} else {
-		modify(_types[_currType], GlobalShieldStatus::NoShields);
-	}
+void ShieldSystemDialogModel::applyType(bool hasShields)
+{
+	_types[_currType] = hasShields ? GlobalShieldStatus::HasShields : GlobalShieldStatus::NoShields;
+	_editor->importShieldSysData(_teams, _types);
+	refresh();
 }
 
 const SCP_vector<SCP_string>& ShieldSystemDialogModel::getShipTypeOptions() const

qtfred/src/mission/dialogs/ShieldSystemDialogModel.h

@@ -12,6 +12,8 @@ class ShieldSystemDialogModel: public AbstractDialogModel {
 	ShieldSystemDialogModel(QObject* parent, EditorViewport* viewport);
 	~ShieldSystemDialogModel() override = default;
 
+	// AbstractDialogModel requires these, but the dialog no longer uses an
+	// OK/Cancel working-copy contract. Each Apply commits immediately.
 	bool apply() override;
 	void reject() override;
 
@@ -22,16 +24,16 @@ class ShieldSystemDialogModel: public AbstractDialogModel {
 
 	GlobalShieldStatus getCurrentTeamShieldSys() const;
 	GlobalShieldStatus getCurrentTypeShieldSys() const;
-	void setCurrentTeamShieldSys(bool value);
-	void setCurrentTypeShieldSys(bool value);
+
+	void applyTeam(bool hasShields);
+	void applyType(bool hasShields);
 
 	const SCP_vector<SCP_string>& getShipTypeOptions() const;
 	const SCP_vector<SCP_string>& getTeamOptions() const;
 
-	bool query_modified() const;
  private:
 	void initializeData();
-
+	void refresh();
 
 	SCP_vector<SCP_string> _shipTypeOptions;
 	SCP_vector<SCP_string> _teamOptions;

qtfred/src/ui/dialogs/ShieldSystemDialog.cpp

@@ -1,12 +1,40 @@
-#include <QCloseEvent>
-#include <QKeyEvent>
+#include <QRadioButton>
 #include "ShieldSystemDialog.h"
 #include "ui/util/SignalBlockers.h"
 #include "ui_ShieldSystemDialog.h"
-#include "mission/util.h"
 
 namespace fso::fred::dialogs {
 
+namespace {
+
+QString statusLabelText(GlobalShieldStatus status)
+{
+	switch (status) {
+	case GlobalShieldStatus::HasShields:
+		return QObject::tr("Currently: Has shields");
+	case GlobalShieldStatus::NoShields:
+		return QObject::tr("Currently: No shields");
+	case GlobalShieldStatus::MixedShields:
+	default:
+		return QObject::tr("Currently: Mixed");
+	}
+}
+
+// Clear the selection in a Qt exclusive button group. Without toggling
+// autoExclusive off, neither radio in an exclusive pair can be unchecked
+// programmatically.
+void clearRadioPair(QRadioButton* a, QRadioButton* b)
+{
+	a->setAutoExclusive(false);
+	b->setAutoExclusive(false);
+	a->setChecked(false);
+	b->setChecked(false);
+	a->setAutoExclusive(true);
+	b->setAutoExclusive(true);
+}
+
+} // namespace
+
 ShieldSystemDialog::ShieldSystemDialog(FredView* parent, EditorViewport* viewport) :
 	QDialog(parent),
 	_viewport(viewport),
@@ -23,32 +51,6 @@ ShieldSystemDialog::ShieldSystemDialog(FredView* parent, EditorViewport* viewpor
 
 ShieldSystemDialog::~ShieldSystemDialog() = default;
 
-void ShieldSystemDialog::accept()
-{
-	// If apply() returns true, close the dialog
-	if (_model->apply()) {
-		QDialog::accept();
-	}
-	// else: validation failed, don't close
-}
-
-void ShieldSystemDialog::reject()
-{
-	// Asks the user if they want to save changes, if any
-	// If they do, it runs _model->apply() and returns the success value
-	// If they don't, it runs _model->reject() and returns true
-	if (rejectOrCloseHandler(this, _model.get(), _viewport)) {
-		QDialog::reject(); // actually close
-	}
-	// else: do nothing, don't close
-}
-
-void ShieldSystemDialog::closeEvent(QCloseEvent* e)
-{
-	reject();
-	e->ignore(); // Don't let the base class close the window
-}
-
 void ShieldSystemDialog::initializeUi() {
 	util::SignalBlockers blockers(this);
 
@@ -73,58 +75,74 @@ void ShieldSystemDialog::updateUi() {
 	util::SignalBlockers blockers(this);
 
 	auto typeShieldSys = _model->getCurrentTypeShieldSys();
-	ui->typeHasShieldRadio->setChecked(typeShieldSys == GlobalShieldStatus::HasShields);
-	ui->typeNoShieldRadio->setChecked(typeShieldSys == GlobalShieldStatus::NoShields);
+	ui->typeCurrentStatusLabel->setText(statusLabelText(typeShieldSys));
+	if (typeShieldSys == GlobalShieldStatus::MixedShields) {
+		clearRadioPair(ui->typeHasShieldRadio, ui->typeNoShieldRadio);
+		ui->applyTypeButton->setEnabled(false);
+	} else {
+		ui->typeHasShieldRadio->setChecked(typeShieldSys == GlobalShieldStatus::HasShields);
+		ui->typeNoShieldRadio->setChecked(typeShieldSys == GlobalShieldStatus::NoShields);
+		ui->applyTypeButton->setEnabled(true);
+	}
 
 	auto teamShieldSys = _model->getCurrentTeamShieldSys();
-	ui->teamHasShieldRadio->setChecked(teamShieldSys == GlobalShieldStatus::HasShields);
-	ui->teamNoShieldRadio->setChecked(teamShieldSys == GlobalShieldStatus::NoShields);
-}
-
-void ShieldSystemDialog::on_okAndCancelButtons_accepted() {
-	accept();
-}
-
-void ShieldSystemDialog::on_okAndCancelButtons_rejected() {
-	reject();
+	ui->teamCurrentStatusLabel->setText(statusLabelText(teamShieldSys));
+	if (teamShieldSys == GlobalShieldStatus::MixedShields) {
+		clearRadioPair(ui->teamHasShieldRadio, ui->teamNoShieldRadio);
+		ui->applyTeamButton->setEnabled(false);
+	} else {
+		ui->teamHasShieldRadio->setChecked(teamShieldSys == GlobalShieldStatus::HasShields);
+		ui->teamNoShieldRadio->setChecked(teamShieldSys == GlobalShieldStatus::NoShields);
+		ui->applyTeamButton->setEnabled(true);
+	}
 }
 
 void ShieldSystemDialog::on_shipTypeCombo_currentIndexChanged(int index) {
 	if (index >= 0) {
-		_model->setCurrentTeam(index);
+		_model->setCurrentShipType(index);
 	}
 	updateUi();
 }
 
 void ShieldSystemDialog::on_shipTeamCombo_currentIndexChanged(int index) {
 	if (index >= 0) {
-		_model->setCurrentShipType(index);
+		_model->setCurrentTeam(index);
 	}
 	updateUi();
 }
 
 void ShieldSystemDialog::on_typeHasShieldRadio_toggled(bool checked) {
 	if (checked) {
-		_model->setCurrentTypeShieldSys(checked);
+		ui->applyTypeButton->setEnabled(true);
 	}
 }
 
 void ShieldSystemDialog::on_typeNoShieldRadio_toggled(bool checked) {
 	if (checked) {
-		_model->setCurrentTypeShieldSys(!checked);
+		ui->applyTypeButton->setEnabled(true);
 	}
 }
 
 void ShieldSystemDialog::on_teamHasShieldRadio_toggled(bool checked) {
 	if (checked) {
-		_model->setCurrentTeamShieldSys(checked);
+		ui->applyTeamButton->setEnabled(true);
 	}
 }
 
 void ShieldSystemDialog::on_teamNoShieldRadio_toggled(bool checked) {
 	if (checked) {
-		_model->setCurrentTeamShieldSys(!checked);
+		ui->applyTeamButton->setEnabled(true);
 	}
 }
 
+void ShieldSystemDialog::on_applyTypeButton_clicked() {
+	_model->applyType(ui->typeHasShieldRadio->isChecked());
+	updateUi();
+}
+
+void ShieldSystemDialog::on_applyTeamButton_clicked() {
+	_model->applyTeam(ui->teamHasShieldRadio->isChecked());
+	updateUi();
+}
+
 } // namespace fso::fred::dialogs

qtfred/src/ui/dialogs/ShieldSystemDialog.h

@@ -19,16 +19,7 @@ class ShieldSystemDialog : public QDialog
     explicit ShieldSystemDialog(FredView* parent, EditorViewport* viewport);
 	~ShieldSystemDialog() override;
 
-	void accept() override;
-	void reject() override;
-
-protected:
-	void closeEvent(QCloseEvent* e) override; // funnel all Window X presses through reject()
-
 private slots:
-	void on_okAndCancelButtons_accepted();
-	void on_okAndCancelButtons_rejected();
-
 	void on_shipTypeCombo_currentIndexChanged(int index);
 	void on_shipTeamCombo_currentIndexChanged(int index);
 
@@ -37,6 +28,9 @@ private slots:
 	void on_teamHasShieldRadio_toggled(bool checked);
 	void on_teamNoShieldRadio_toggled(bool checked);
 
+	void on_applyTypeButton_clicked();
+	void on_applyTeamButton_clicked();
+
 private:  // NOLINT(readability-redundant-access-specifiers)
 	void initializeUi();
 	void updateUi();