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.

Sign up for GitHub

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

Jump to bottom

Don't trash userconfig on errors #3192

Merged
merged 6 commits into from
Nov 9, 2020
Merged

Don't trash userconfig on errors #3192

merged 6 commits into from
Nov 9, 2020

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented Oct 20, 2020 •
edited
Loading

Instead of opening the current config file for write operations,
write the user config file into a new file and rename after success.

This will no longer trash the config file if the file-system is full.

fixes launchpad bug #1881997

@poelzi
Instead of opening the current config file for write operations,
8eb499b 
write the user config file into a new file and rename after success.

fixes launchpad bug #1881997
@github-actions github-actions bot added the ui label Oct 20, 2020
@poelzi poelzi changed the title Instead of opening the current config file for write operations, Don't trash userconfig on errors Oct 20, 2020
Holzhaus
Holzhaus requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks!

src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
uklotzde
uklotzde reviewed Oct 20, 2020
View reviewed changes
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
@poelzi
Also check stream position when writing config
9873f99 
Calculate a minimum size the config must have and check the seek position after
flushing.
Check filesize on disk with expected size.
@poelzi
Copy link
Contributor Author

poelzi commented Oct 21, 2020

I did some more testing and added additional sanity checks.
It now also properly catches cases of partially written config.
Also print the configfile name as well where the problem occurred.

@poelzi poelzi requested review from uklotzde and Holzhaus October 23, 2020 19:08
Holzhaus
Holzhaus requested changes Oct 23, 2020
View reviewed changes
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
@Holzhaus Holzhaus marked this pull request as draft October 25, 2020 20:55
@poelzi poelzi requested a review from Holzhaus October 26, 2020 18:55
@poelzi poelzi marked this pull request as ready for review October 26, 2020 19:04
uklotzde
uklotzde suggested changes Oct 26, 2020
View reviewed changes
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
src/preferences/configobject.cpp Outdated Show resolved Hide resolved
@poelzi poelzi requested a review from uklotzde October 27, 2020 13:27
@Holzhaus Holzhaus mentioned this pull request Oct 27, 2020
Merged
@Holzhaus Holzhaus marked this pull request as draft October 31, 2020 20:31
@ronso0 ronso0 mentioned this pull request Nov 2, 2020
Merged
@poelzi poelzi marked this pull request as ready for review November 4, 2020 01:31
uklotzde
uklotzde approved these changes Nov 8, 2020
View reviewed changes
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Haven't tested it yet, but code-wise LGTM so far.

@uklotzde
Copy link
Contributor

uklotzde commented Nov 9, 2020

@Holzhaus Ready? No issues so far.

@Holzhaus
Copy link
Member

Holzhaus commented Nov 9, 2020

@Holzhaus Ready? No issues so far.

Ok for me.

@uklotzde
Copy link
Contributor

uklotzde commented Nov 9, 2020

Thank you! LGTM

@uklotzde uklotzde merged commit 6fdf4af into mixxxdj:2.3 Nov 9, 2020
@ronso0
Copy link
Member

ronso0 commented Nov 10, 2020

@poelzi
there 's now https://bugs.launchpad.net/mixxx/+bug/1903626 about not saving the beatgrid opacity, and I can confirm Mixxx doesn't save any skin config. Didn't proceed testing other configurations but I guess this PR is the cause.
I revert the commits one by one and it's the very first that causes the issue 8eb499b
Please provide a quickfix.

@Holzhaus
Copy link
Member

Holzhaus commented Nov 12, 2020 •
edited
Loading

I also noticed these warnings on every shutdown:

Warning [Main]: Could not remove old config file: "/home/jan/.mixxx/mixxx.cfg": "No such file or directory"
Warning [LibraryScanner 1]: QSqlDatabasePrivate::removeDatabase: connection 'MIXXX-2' is still in use, all queries will cease to work.
Warning [Main]: Could not remove old config file: "/home/jan/.mixxx/mixxx.cfg": "No such file or directory"
Warning [Main]: Could not remove old config file: "/home/jan/.mixxx/sandbox.cfg": "No such file or directory"

@ronso0
Copy link
Member

ronso0 commented Nov 13, 2020

ping @poelzi
We need a fix for this!

@ronso0
Copy link
Member

ronso0 commented Nov 15, 2020

@poelzi Please take a look at the error.
Currently it doesn't make sense to point Linux users to current 2.3 builds for troubleshooting if the config is not saved.

@Holzhaus
Copy link
Member

Revert for now?

@ronso0
Copy link
Member

ronso0 commented Nov 15, 2020

taking a look at this right now, trying to find out why it can't remove the old config.

@ronso0
Copy link
Member

ronso0 commented Nov 15, 2020

Hmm..suddenly it works for me here, all preferences and skin settings are saved.
maybe changes to 2.3 in the meantime fixed it??
@Holzhaus Can you confirm it works for you, too?

@uklotzde
Copy link
Contributor

Unfortunately, I cannot reproduce the issues on Fedora and Qt 5.15. No idea what is wrong with these changes!?

@Be-ing Be-ing mentioned this pull request Nov 26, 2020
Merged
@Be-ing
Copy link
Contributor

Be-ing commented Nov 26, 2020

I also noticed these warnings on every shutdown:

Fix is here: #3371

@mixxxbot mixxxbot mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uklotzde uklotzde uklotzde approved these changes

@Holzhaus Holzhaus Awaiting requested review from Holzhaus

Assignees
No one assigned
Labels
ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@poelzi @uklotzde @Holzhaus @ronso0 @Be-ing