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

Only migrate settings if first start=false #2437

Closed
wants to merge 1 commit into from
Closed

Conversation

CoBC
Copy link
Contributor

@CoBC CoBC commented Aug 16, 2024

When migrateSettings() is call with a blank ini file, a lot of settings aren't incorrectly set to empty values, E.G. sound events. Also some keys aren't incorrectly redefine, E.G. gender define to female instead of neutral.

@CoBC CoBC added this to the TeamTalk v5.17.1 milestone Aug 16, 2024
@CoBC CoBC requested a review from bear101 August 16, 2024 18:44
@CoBC CoBC self-assigned this Aug 16, 2024
@CoBC
Copy link
Contributor Author

CoBC commented Aug 16, 2024

@bear101 it seams some config keys are also migrate incorrectly in existing config, E.G. all sound events are disabled when running 5.17 on an existing 5.16.1 config.
I don't really understand why, do you have an ideA? I think we should fix this and than release a 5.17.1 because it's a major bug for a lot of users.

@bear101
Copy link
Contributor

bear101 commented Aug 21, 2024

With this change the following code is not called in migrate settings:

    if (ttSettings->value(SETTINGS_GENERAL_VERSION).toString() != SETTINGS_VERSION)
        ttSettings->setValue(SETTINGS_GENERAL_VERSION, SETTINGS_VERSION);

Also SETTINGS_GENERAL_FIRSTSTART was first introduced in v5.8 so settings prior to that will not be migrated.

@CoBC
Copy link
Contributor Author

CoBC commented Aug 21, 2024

Ah, yes, you're right. also I guess all other fixes are sufficient, so closing this PR

@CoBC CoBC closed this Aug 21, 2024
@CoBC CoBC deleted the migrate_first_start branch August 21, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants