-
Notifications
You must be signed in to change notification settings - Fork 293
Filemanager improved error handling #1710
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
Conversation
cc3f990 to
623d411
Compare
If you opened the backup, saved and the project corrupted, a backup wouldn't be made. The file would instead be renamed.
623d411 to
0de72ae
Compare
| const bool isOldType = sFileName.endsWith(PFF_OLD_EXTENSION); | ||
| if (isOldType) | ||
| Status isArchiveSt = isArchiveFormat(sFileName); | ||
| if (isArchiveSt == Status::NOT_ARCHIVE_FORMAT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird to use Status in this way instead of reporting errors. Probably keep the old bool return value?
The function isArchiveFormat() returns only two values: OK or NOT_ARCHIVE_FORMAT and doesn't have any error handling things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right.. previously I was using the sanity check function, and used the Status details to enrich the error report but since we don't do that anymore, there's no need to use Status, I will change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| QDir directory(info.absoluteDir()); | ||
| QString backupString = "backup"; | ||
| for (QFileInfo info : directory.entryInfoList(QDir::Filter::Files)) { | ||
| if (info.completeBaseName().contains(backupString)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word "backup" is commonly used in terms of file naming. Chances are that users will use "backup" in their filename and then the backupCount number would be wrong.
I would suggest to check with full filenames such as sBackupFile below to get a correct backup count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. good point, I forgot about that! will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| } | ||
| } | ||
|
|
||
| QString sBackupFile = info.baseName() + "." + backupString + QString::number(backupCount) + "." + info.suffix(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completeBaseName() instead of baseName()? Just in case there are dots in filenames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
Thanks @MrStevns! I think generally it's good. It can be merged after some minor improvements as comments. |
113b84c to
2526ce5
Compare
This PR makes a few changes to the filemanager, improving the error handling and fixing a few situations where the project could be corrupted and a backup wouldn't be made.