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

Filemanager improved error handling #1710

Merged
merged 5 commits into from
May 13, 2022

Conversation

MrStevns
Copy link
Member

@MrStevns MrStevns commented May 5, 2022

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.

@MrStevns MrStevns force-pushed the file-manager-errr-handling branch from cc3f990 to 623d411 Compare May 5, 2022 17:06
@MrStevns MrStevns marked this pull request as draft May 7, 2022 18:29
@MrStevns MrStevns force-pushed the file-manager-errr-handling branch from 623d411 to 0de72ae Compare May 8, 2022 09:14
@MrStevns MrStevns marked this pull request as ready for review May 8, 2022 09:19
@chchwy chchwy self-assigned this May 10, 2022
core_lib/src/structure/filemanager.cpp Show resolved Hide resolved
const bool isOldType = sFileName.endsWith(PFF_OLD_EXTENSION);
if (isOldType)
Status isArchiveSt = isArchiveFormat(sFileName);
if (isArchiveSt == Status::NOT_ARCHIVE_FORMAT)
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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)) {
Copy link
Member

@chchwy chchwy May 12, 2022

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.

Copy link
Member Author

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.

Copy link
Member Author

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();
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@chchwy
Copy link
Member

chchwy commented May 12, 2022

Thanks @MrStevns! I think generally it's good. It can be merged after some minor improvements as comments.

@MrStevns MrStevns force-pushed the file-manager-errr-handling branch from 113b84c to 2526ce5 Compare May 12, 2022 17:26
@chchwy chchwy merged commit 86780f4 into pencil2d:master May 13, 2022
@chchwy chchwy added this to the 0.7.0 milestone May 20, 2022
@MrStevns MrStevns modified the milestones: 0.7.0, v0.6.7 Jul 5, 2023
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