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

.conf files are deleted when changes are saved. #493

Closed
romeojulietthotel opened this issue Feb 28, 2017 · 10 comments
Closed

.conf files are deleted when changes are saved. #493

romeojulietthotel opened this issue Feb 28, 2017 · 10 comments
Labels

Comments

@romeojulietthotel
Copy link

Using git master.

Steps to reproduce:

  1. make some changes and save them to atest.conf
  2. ls -l ~/.conf/gqrx
  3. make some changes and save them to atest.conf
  4. when asked to overwrite answer yes
  5. ls -l ~/.conf/gqrx

If I can figure it out I will send a PR.

@alexf91
Copy link
Contributor

alexf91 commented Feb 28, 2017

I can reproduce this.
The debug output for the second save:

Saving configuration to: "/home/alex/.config/gqrx/atest.conf"
File "/home/alex/.config/gqrx/atest.conf" already exists => DELETING...
Deleted "/home/alex/.config/gqrx/atest.conf"
Error saving configuration to "/home/alex/.config/gqrx/atest.conf"

Relevant code is in mainwindow.cpp:saveConfig.
When newfile = oldfile, then the copy operation fails, because the file does not exist.

@csete
Copy link
Collaborator

csete commented Feb 28, 2017

I assume we are talking about using the "Save as..." functionality and now the usual auto-save when the program exits.

@csete csete added the bug label Feb 28, 2017
@alexf91
Copy link
Contributor

alexf91 commented Mar 1, 2017

@csete That's right.

It would be enough to check if newfile == oldfile and don't do anything (just sync) if they are equal to resolve this bug and keep the current behavior.
The current "Save as" behavior seems counterintuitive to me, because all settings are written to the current configuration before saving them to another file. Except for the 'crashed' field, all settings will be equal.

@romeojulietthotel
Copy link
Author

We should use:
http://doc.qt.io/qt-5/qsavefile.html

@alexf91
Copy link
Contributor

alexf91 commented Mar 2, 2017

@romeojulietthotel I don't think this makes sense here. QSaveFile is for raw file IO. QSettings already uses QSaveFile internally when writing to disk.

@romeojulietthotel
Copy link
Author

QSaveFile makes sense to me.
The QSaveFile class provides an interface for safely writing to files.
Using copy is very clearly wrong.
http://doc.qt.io/qt-5/qfile.html#copy

Note that if a file with the name newName already exists, copy() returns false (i.e. QFile will not overwrite it).

@alexf91
Copy link
Contributor

alexf91 commented Mar 2, 2017

What QSaveFile does is writing to a temporary file, and then moving it to the desired location when commit() is called. It avoids incompletely written files (e.g. when the application crashes during writing).

In our case, the call to copy failed because the source file was deleted before calling copy. I don't even know how we would use QSaveFile here, other than implementing our own copy operation.
I don't see any advantage in doing that.

@romeojulietthotel
Copy link
Author

Copy will fail as it is used even if the file is not deleted.

http://doc.qt.io/qt-5/qtwidgets-mainwindows-application-mainwindow-cpp.html

@alexf91
Copy link
Contributor

alexf91 commented Mar 2, 2017

Copy won't fail if newfile doesn't exist.
I see three cases:

  • newfile == oldfile: Triggers the error described
  • newfile != oldfile && newfile doesn't exist: No delete, oldfile copied to newfile
  • newfile != oldfile && newfile exists: delete newfile, oldfile copied to newfile

The copy operation only fails in the first case, because the source file is deleted. This is addressed in #495.
I don't see a scenario where the copy operation fails due to an existing destination file.
Am I missing something?

@csete csete closed this as completed in #495 Mar 5, 2017
@csete
Copy link
Collaborator

csete commented Mar 5, 2017

@romeojulietthotel can you please check and confirm that the applied fix solves your issue? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants