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

Extract pclx to a random generated folder name to avoid name conflicts #1043

Merged
merged 6 commits into from
Aug 16, 2018

Conversation

chchwy
Copy link
Member

@chchwy chchwy commented Aug 7, 2018

Related issues #932 #752

The issue described by @scribblemaniac :
If a user opens the same project twice in a row, Penci2D will:

  1. Create data folder for file to load
  2. Load everything into the data folder
  3. Stop if there is an error
  4. Switch to using the new data folder
  5. Delete the old data folder

However, if the old data folder and new data folder are the same, then step 5 ends up deleting all the data from our project!

In order to resolve the name conflicts, the data folder will include an 8 character long random string.

For example:

  • YourProject_Y2xD_6xseyd51
  • Default_Y2xD_5zy4hsui

…avoid folder name conflicts.

If a user opens the same project twice in a row, Penci2D will:

1. Create data folder for file to load
2. Load everything into the data folder
3. Stop if there is an error
4. Switch to using the new data folder
5. Delete the old data folder

However, if the old data folder and new data folder are the same, then step 5 ends up deleting all the data from our project! (by scribblemaniac)
+ strFolderName
+ PFF_TMP_DECOMPRESS_EXT
+ "/";
const QString strWorkingDir =
Copy link
Member

@scribblemaniac scribblemaniac Aug 8, 2018

Choose a reason for hiding this comment

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

For my peace of mind, could we change the following few lines to this:

QString strWorkingDir;
QDir dir(QDir::tempPath());
do {
    strWorkingDir =
        QString("%1/Pencil2D/%2_%3_%4/").arg(QDir::tempPath()).arg(strFolderName).arg(PFF_TMP_DECOMPRESS_EXT).arg(uniqueString(8));
} while(dir.exists(strWorkingDir));

The chance of generating the same string is almost impossible, but I would feel better if we checked for it anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. thanks for reminding.

+ strFolderName
+ PFF_TMP_DECOMPRESS_EXT
+ "/";
const QString strWorkingDir =
Copy link
Member

Choose a reason for hiding this comment

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

The default folders are not getting deleted. This is because the Object destructor only calls deleteWorkingDir if filePath() is empty. Instead I think we should always call deleteWorkingDir, and just add a check in that function to make sure that mWorkingDirPath is not empty.

On a slightly unrelated note, is there any reason why we don't just call init in the Object constructor? That way we can be sure that it is called at least once.

Copy link
Member Author

Choose a reason for hiding this comment

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

The behaviours were introduced in a PR #591, I don't really remember why it's working in this way. So the working directory will be deleted anyway now.

For my understanding, Object::init() is for a new Object. In Pencil2D it's the starting default project and the project when you click from the menu File->New. It's no harm to call init() on an Object from a pclx file but all data will be overwritten immediately.

@J5lx
Copy link
Member

J5lx commented Aug 8, 2018

Seems good to me except for what scribblemaniac noted. Also, not directly related to the PR, but what does Y2xC and Y2xD stand for? It seems the last letter distinguishes between (de)compression, but what about the rest? Just curious.

@chchwy
Copy link
Member Author

chchwy commented Aug 9, 2018

Y2xC is legacy and not used anymore. I deleted it. Preserved Y2xD for the sake of debugging. I can quickly check all pencil2d working directories on my computer by a keyword.

@chchwy chchwy merged commit 504280a into pencil2d:master Aug 16, 2018
@chchwy chchwy added this to the 0.6.2 milestone Sep 25, 2018
@chchwy chchwy deleted the fix/load-twice-wipe branch January 23, 2020 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants