Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qtFRED bug: closing the Initial Goals dialog does not save its data #6466

Open
Goober5000 opened this issue Dec 16, 2024 · 6 comments
Open
Assignees
Labels
bug An issue from unintended consequences qtfred A feature or issue related to qtFred.

Comments

@Goober5000
Copy link
Contributor

Steps to reproduce:

  1. Create a new mission in qtFRED.
  2. Place a new transport in the mission area.
  3. Open the ship editor and the initial goals dialog.
  4. Change the first goal from "None" to "Play dead".
  5. Close the dialog by clicking the close button, not the Ok button.
  6. The dialog will prompt to save the changes. Click Yes.
  7. Close the ship editor.
  8. Reopen the ship editor and the initial goals. The first goal will say "None" again.

I've traced this as far as determining that ShipGoalsDialog::closeEvent() reaches accept() and calls it, but the corresponding ShipGoalsDialogModel::apply() is not called. The following line exists in ShipGoalsDialog:

connect(this, &QDialog::accepted, _model.get(), &ShipGoalsDialogModel::apply);

But it apparently isn't working as intended. I'm not familiar enough with the Qt connection code to diagnose further.

Related to #3717. Pinging @TheForce172 for insight.

@Goober5000 Goober5000 added bug An issue from unintended consequences qtfred A feature or issue related to qtFred. labels Dec 16, 2024
@github-project-automation github-project-automation bot moved this to Open Tasks in qtFRED2 Dec 16, 2024
@TheForce172
Copy link
Contributor

This has probably been fixed by the refactor we're doing but I'll take a look tomorrow anyway

@TheForce172
Copy link
Contributor

TheForce172 commented Dec 17, 2024

Confirmed fixed in my dev branch. Do you want this specific dialog splitting of and a separate PR made or are you willing to wait till I'm done with all of them?

@Goober5000
Copy link
Contributor Author

Is it possible to bring over just this fix? Otherwise, yes, please make a separate PR for just the updates to the Initial Goals dialog. (In general, it's helpful to have one PR per dialog, following the example of #3674.)

The in-testing PR #6467 makes changes to ai goal data structures in several places, including qtFRED, so if your dev branch modifies the Initial Goals dialog, that's another reason to merge that section sooner rather than later.

@Goober5000
Copy link
Contributor Author

@TheForce172 will you have time to look at this in the next week or so?

@TheForce172
Copy link
Contributor

I've created the full pull request at #6493. Unfortunately this change requires a modification to the AbstractDialogModel so all models have to be updated simultaneously so we can't break up the pull request into individual dialogs.

@Goober5000
Copy link
Contributor Author

Ah. So, in addition to

Implements #6457 and half of #6458

it resolves this issue as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue from unintended consequences qtfred A feature or issue related to qtFred.
Projects
Status: Open Tasks
Development

No branches or pull requests

2 participants