这是indexloc提供的服务,不要输入任何密码
Skip to content

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 added 3 commits May 8, 2022 11:12
If you opened the backup, saved and the project corrupted, a backup wouldn't be made. The file would instead be renamed.
@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
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