Delete files not existent in project directory #6501

DavidMLPalma · 2024-04-03T20:55:54Z

Fix #4120 : When renaming scenes/extensions/external events/external layouts, duplicate files exist, one with the old name and one with the new name. Similarly, when deleting this items, the respective files are also not removed from the project directory. This is due to the fact that when saving the project, only the existing files in the project editor are written and saved, and nothing more is done. There may still remain old files that should not be there, potentially creating duplicates and confusion. Therefore, when saving, we can resolve this issue by deleting all files from the scenes/extensions/external events/external layouts directories, before writing the correct files.

…al layouts , duplicate files exist. When renaming scenes/extensions/external events/external layouts, duplicate files exist, one with the old name and one with the new name. When deleting this items, the respective files are also not removed from the project directory. This is due to the fact that when saving the project, only the existing files in the project editor are written and saved, and the old ones still remain, potentialy creating duplicates and confusion. Therefore, when saving, we can resolve this issue by deleting all files from the scenes/extensions/external events/external layouts directories, before writing the correct files.

AlexandreSi · 2024-04-23T08:30:36Z

Hi @DavidMLPalma,
Rhanks for the suggested PR!

I feel like this strategy is a bit brutal. We already have issues with users saving their projects in the installation directory and losing their projects because of that (when GDevelop is updated). So I think it's better not to have another potential cause for losing files.

May I suggest that you check if the directory name is in the list splittedProjectFolderNames in LocalProjectWriter?

DavidMLPalma · 2024-04-23T22:00:05Z

Hi @AlexandreSi ,
Thanks for your reply!

I totally get your point, and in fact by checking if the directory name is one of those present in the list may lead to a more secure solution. I will change it then.

AlexandreSi · 2024-05-06T16:29:59Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

@@ -142,6 +168,12 @@ export const onSaveProject = (
  };

  const projectPath = path.dirname(filePath);
+
+  deleteExistingFilesFromDirs(projectPath).catch(err => {


Could you improve a bit the whole onSaveProject function so that it's an async function? (If you're not familiar with it, I can provide guidance).

Should I improve the deleteExistingFilesFromDirs function so it's an async functions as well?

No, I think it's okay to keep it synchronous

AlexandreSi · 2024-05-06T16:34:25Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

@@ -142,6 +168,12 @@ export const onSaveProject = (
  };

  const projectPath = path.dirname(filePath);
+
+  deleteExistingFilesFromDirs(projectPath).catch(err => {
+    console.error('Unable to delete files in the project:', err);


I think here we could gain clarity if you mention the context: Unable to clean project folder before saving project:

AlexandreSi · 2024-05-06T16:35:14Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

+    //Project file
+    if (entries.length === 1) resolve();
+
+    //If multiFile enabled in settings and directories exist!


You do not actually check if multifile is enabled in the project's settings. You should use project.isFolderProject() for that.

To make it one step clearer by removing the indent, you could do this:

if (!project.isFolderProject()) return; const entries = fs.readdirSync(projectPath); ...

Also, I'm not sure this comment is useful anymore, your code is quite clear.

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

AlexandreSi · 2024-05-06T16:39:00Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

+        filenames.forEach(file => {
+          let result = dirPath.concat('\\', file);
+          fs.unlink(result, err => {
+            if (err) return reject(err);


You could add context here, such as:

throw new Error(`Unable to remove file ${file}: ${err.message}`

Or something like that

DavidMLPalma · 2024-05-06T21:21:25Z

Hi @AlexandreSi ,
Thanks a lot for the patience and feedback! I will try to implement all the things you pointed out !

AlexandreSi · 2024-05-21T15:04:29Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

+  if (project.isFolderProject()) {
+    //If multiFile enabled in settings and directories exist!
+    entries.forEach(entry => {
+      let dirPath = projectPath.concat('\\', entry);


I forgot to check how you handle paths: Using strings that you concat yourself won't work because Windows user \ separator while Linux and macOS user /. So you should use path.join instead. You can find other users of path in the codebase.

Furthermore, you can use const instead of let since you never reassign the variable value

AlexandreSi · 2024-05-21T15:07:40Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

+      let dirPath = projectPath.concat('\\', entry);
+      if (
+        fs.statSync(dirPath).isDirectory() &&
+        splittedProjectFolderNames.includes(entry)


You could use this condition as an early return as well:

if (splittedProjectFolderNames.includes(entry)) return; let dirPath = projectPath.concat('\\', entry); ...

AlexandreSi · 2024-05-21T15:10:01Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

+      ) {
+        const filenames = fs.readdirSync(dirPath);
+        filenames.forEach(file => {
+          let result = dirPath.concat('\\', file);


You could rename result with a name that actually represents what it is: filePath or fileToRemovePath

….join

DavidMLPalma · 2024-05-21T21:23:37Z

Hi @AlexandreSi ,

Once again, thanks for your help and consideration !

AlexandreSi · 2024-05-23T13:57:59Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

+  try {
+    deleteExistingFilesFromDirs(project, projectPath);
+  } catch (e) {
+    return Promise.reject(


I think that deleting the existing files from the folders should fail silently.
You can use instead:
console.warn('Unable to clean project folder before saving project: ', error)
But stopping a save could mean preventing a user from saving if an error happens.

Also, since you made the change to an async function (thank you!), could you replace the return new Promise.reject line 158 by a throw new Error('...')

AlexandreSi · 2024-05-23T14:01:12Z

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js

+    if (!splittedProjectFolderNames.includes(entry)) return;
+
+    const dirPath = path.join(projectPath, entry);
+    if (fs.statSync(dirPath).isDirectory()) {


You can also replace the added intent by an early return here.

AlexandreSi

LGTM!
Thanks for staying with me though I asked for a lot of changes. I hope our coding style is clearer to you today!

DavidMLPalma and others added 2 commits April 3, 2024 15:19

Merge branch '4ian:master' into DeleteFilesNotExistent

73b8d5d

DavidMLPalma requested a review from 4ian as a code owner April 3, 2024 20:55

Added directory check to not delete files from unwanted directories

2b81975

AlexandreSi reviewed May 6, 2024

View reviewed changes

newIDE/app/src/ProjectsStorage/LocalFileStorageProvider/LocalProjectWriter.js Outdated Show resolved Hide resolved

AlexandreSi reviewed May 6, 2024

View reviewed changes

DavidMLPalma requested a review from AlexandreSi May 7, 2024 11:38

Improved onSaveProject to be an async function and made few adjustements

f77116f

AlexandreSi reviewed May 21, 2024

View reviewed changes

Fix: Small changes to variables and concatenation of paths using path…

8b88eca

….join

DavidMLPalma requested a review from AlexandreSi May 21, 2024 21:23

AlexandreSi reviewed May 23, 2024

View reviewed changes

Fix: Deleting files from folders fails silently

805b360

DavidMLPalma requested a review from AlexandreSi May 23, 2024 16:10

AlexandreSi approved these changes May 24, 2024

View reviewed changes

AlexandreSi merged commit eaebc16 into 4ian:master May 24, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete files not existent in project directory #6501

Delete files not existent in project directory #6501

DavidMLPalma commented Apr 3, 2024

AlexandreSi commented Apr 23, 2024

DavidMLPalma commented Apr 23, 2024

AlexandreSi May 6, 2024

DavidMLPalma May 7, 2024

AlexandreSi May 21, 2024

AlexandreSi May 6, 2024

AlexandreSi May 6, 2024

AlexandreSi May 21, 2024

AlexandreSi May 6, 2024

DavidMLPalma commented May 6, 2024

AlexandreSi May 21, 2024

AlexandreSi May 21, 2024

AlexandreSi May 21, 2024

AlexandreSi May 21, 2024

DavidMLPalma commented May 21, 2024

AlexandreSi May 23, 2024 •

edited

AlexandreSi May 23, 2024

DavidMLPalma May 23, 2024

AlexandreSi May 23, 2024

AlexandreSi left a comment

Delete files not existent in project directory #6501

Delete files not existent in project directory #6501

Conversation

DavidMLPalma commented Apr 3, 2024

AlexandreSi commented Apr 23, 2024

DavidMLPalma commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidMLPalma commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidMLPalma commented May 21, 2024

AlexandreSi May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandreSi left a comment

Choose a reason for hiding this comment

AlexandreSi May 23, 2024 •

edited