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

fix: save settings on exit #125

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Conversation

smnppKDAB
Copy link
Collaborator

We add a way to save settings before exiting if there are changes. This make the tests to pass with the corrected knut.json

@dfaure-kdab
Copy link
Member

Looks good to me, but this is my idea in the first place, so I'll let @LeonMatthesKDAB review ;-)

Debugging indicated that with offscreen QPA the Settings timer had time to fire but without it, the app would exit due to lastWindowClosed(), without waiting for scriptFinished and without saving settings.

@smnppKDAB smnppKDAB requested review from LeonMatthesKDAB and removed request for LeonMatthesKDAB July 24, 2024 16:50
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall making sure we save the settings when the timer is still active definitely makes sense.
However, can you try calling saveOnExit() in the Settings destructor, instead of connecting it to scriptFinished?

It just seems like that would be the more fool-proof option and should cover more cases.

@LeonMatthesKDAB
Copy link
Collaborator

LeonMatthesKDAB commented Jul 25, 2024

Hm, I just tried testing knut locally and this PR actually doesn't seem to fix the issue 🤔
Strangely 2/3 tests fail without actually telling me which file is different.
And the other one tells me that knut.json is missing in the expected folder.
Which to me almost sounds like it's now saving settings that it didn't used to before.

Edit: I've got the output from the failing tests. mfc-utils was using qdebug instead of qwarning.
In any case, the diff is indeed in the knut.json.
I can't really tell which one is right here though...

@LeonMatthesKDAB
Copy link
Collaborator

LeonMatthesKDAB commented Jul 25, 2024

So for the mfc_convert_dialog test I get the following files:

expected

{
    "qtui": {
        "custom_widgets": [
            "MySliderCtrl|\"mysliderctrl.h\"|QSlider"
        ]
    }
}

actual

{
    "qtui": {
        "custom_widgets": [
            "MySliderCtrl|\"mysliderctrl.h\"|QSlider"
        ]
    },
    "rc": {
        "dialog_flags": [
            "UpdateGeometry",
            "UseIdForPixmap"
        ],
        "dialog_scalex": 1.5,
        "dialog_scaley": 1.65
    }
}

So is this change correct?

@LeonMatthesKDAB
Copy link
Collaborator

Hm, so for me the tests only pass with these patches:
https://codereview.kdab.com/c/kdab/mfc-utils/+/144535

Are these updates expected? I'm not too familiar with mfc-utils, so can't tell if they are sensible.

@dfaure-kdab
Copy link
Member

Don't forget the additional variable of running tests via ctest/CI versus running tests directly (QtCreator or command-line).
This patch here is about ensuring that both leads to the same result. Previously, ctest/CI would save on exit while QtCreator/cmdline would not (because of lastWindowClosed).

So yes, if you stay within the ctest/CI use case, this patch isn't needed. But better save in all cases to minimize surprises.

Calling saveOnExit() in the Settings destructor is an interesting idea, I'll let @smnppKDAB test it.

@smnppKDAB
Copy link
Collaborator Author

Hm, so for me the tests only pass with these patches: https://codereview.kdab.com/c/kdab/mfc-utils/+/144535

Are these updates expected? I'm not too familiar with mfc-utils, so can't tell if they are sensible.

Here is the corresponding MR : https://codereview.kdab.com/c/kdab/mfc-utils/+/144526

@smnppKDAB smnppKDAB force-pushed the sperret-saveOnExit branch from 343b1d8 to 9e8c55a Compare July 25, 2024 08:31
@LeonMatthesKDAB
Copy link
Collaborator

Hm, so for me the tests only pass with these patches: https://codereview.kdab.com/c/kdab/mfc-utils/+/144535
Are these updates expected? I'm not too familiar with mfc-utils, so can't tell if they are sensible.

Here is the corresponding MR : https://codereview.kdab.com/c/kdab/mfc-utils/+/144526

Oh, right, I didn't know about that MR. Please link any MRs that a PR is related to next time :)

@smnppKDAB smnppKDAB force-pushed the sperret-saveOnExit branch from 9e8c55a to a1a859f Compare July 25, 2024 09:24
We add a way to save settings before exiting if there are changes.
This make the tests to pass with the corrected knut.json
@dfaure-kdab dfaure-kdab merged commit be7aff3 into KDAB:main Jul 25, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants