Directory and file delete to trash #149

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

leonuh commented Nov 26, 2012

Implemented only for osx

gruehle was assigned Nov 27, 2012

@gruehle gruehle commented on an outdated diff Nov 27, 2012

appshell/appshell_extensions.js
@@ -310,7 +310,7 @@ if (!appshell.app) {
*/
native function DeleteFileOrDirectory();
appshell.fs.unlink = function (path, callback) {
- DeleteFileOrDirectory(callback, path);
+ DeleteFileOrDirectory(callback, path, false);
@gruehle

gruehle Nov 27, 2012

Member

The unlink() function should be a permanent delete. We'll need a new moveToTrash() function for the non-permanent delete.

@gruehle gruehle commented on an outdated diff Nov 27, 2012

appshell/appshell_extensions_mac.mm
@@ -461,16 +461,16 @@ int32 DeleteFileOrDirectory(ExtensionString filename)
BOOL isDirectory;
// Contrary to the name of this function, we don't actually delete directories
- if ([[NSFileManager defaultManager] fileExistsAtPath:path isDirectory:&isDirectory]) {
- if (isDirectory) {
- return ERR_NOT_FILE;
@gruehle

gruehle Nov 27, 2012

Member

It looks like you removed the ERR_NOT_FILE condition. The unlink() function needs to return ERR_NOT_FILE if passed a directory. You can run the LowLevelFileIO unit tests in Brackets to verify this functionality.

@gruehle

gruehle Nov 27, 2012

Member

(and yes, this function has a poor name since it really only deletes files... )

Member

gruehle commented Nov 27, 2012

Done with initial review.

Contributor

leonuh commented Dec 5, 2012

Sorry for delayed commit but i was on holiday. The functions separated as you wish. Function for windows implemented, but environment not ready even, so i couldnt test it on windows. By the way i think its working.

For later: What is the best solution to create exe on osx from the project?

@gruehle gruehle commented on an outdated diff Dec 17, 2012

appshell/appshell_extensions_win.cpp
@@ -690,6 +690,23 @@ int32 DeleteFileOrDirectory(ExtensionString filename)
return NO_ERROR;
}
+int32 DeleteFileOrDirectoryToTrash(ExtensionString filename)
+{
+ DWORD dwAttr = GetFileAttributes(filename.c_str());
+
+ if (dwAttr == INVALID_FILE_ATTRIBUTES)
+ return ERR_NOT_FOUND;
+
+ SHFILEOPSTRUCT operation;
+ operation.wFunc = FO_DELETE;
+ operation.pFrom = filename.c_str();
+ operation.fFlags = FOF_ALLOWUNDO;
+
+ if (SHFileOperation(&operation))
+ return ConvertWinErrorCode(GetLastError());
@gruehle

gruehle Dec 17, 2012

Member

The documentation for SHFileOperation says that GetLastError() should not be used. You should pass the return value of SHFileOperation() to ConvertWinErrorCode(), and also make sure the important errors are getting mapped correctly.

@gruehle gruehle commented on an outdated diff Dec 17, 2012

appshell/appshell_extensions.js
+
+ /**
+ * Delete a file non-permanently (to trash).
+ *
+ * @param {string} path The path of the file to delete
+ * @param {function(err)} callback Asynchronous callback function. The callback gets one argument (err).
+ * Possible error values:
+ * NO_ERROR
+ * ERR_UNKNOWN
+ * ERR_INVALID_PARAMS
+ * ERR_NOT_FOUND
+ * ERR_NOT_FILE
+ *
+ * @return None. This is an asynchronous call that sends all return information to the callback.
+ */
+ native function DeleteFileOrDirectoryToTrash();
@gruehle

gruehle Dec 17, 2012

Member

Naming nit: this function would be better named MoveFileOrDirectoryToTrash().

@gruehle gruehle commented on the diff Dec 17, 2012

appshell/appshell_extensions_mac.mm
if ([[NSFileManager defaultManager] removeItemAtPath:path error:&error])
+ return NO_ERROR;
+
+ return ConvertNSErrorCode(error, false);
+}
+
+int32 DeleteFileOrDirectoryToTrash(ExtensionString filename)
+{
+ NSError* error = nil;
+
+ NSString* path = [NSString stringWithUTF8String:filename.c_str()];
+
+ if ([[NSFileManager defaultManager] moveItemAtPath:path toPath:[[@"~/.Trash/" stringByStandardizingPath] stringByAppendingPathComponent:[path lastPathComponent]] error:&error])
@gruehle

gruehle Dec 17, 2012

Member

Is there a reason why you're using moveItemAtPath:toPath here instead of NSWorkspaceRecycleOperation? I'm not sure which is best, but it seems like NSWorkspaceRecycleOperation is the preferred method based on a quick skim of the docs.

@leonuh

leonuh Dec 19, 2012

Contributor

Unfortunately NSWorkspaceRecycleOperation didnt work :(

Member

gruehle commented Dec 17, 2012

Hey @leonuh -- sorry it's taken so long to re-review. There are just a couple minor issues and a question. Thanks!

@gruehle gruehle commented on an outdated diff Jan 3, 2013

appshell/appshell_extensions.cpp
@@ -283,6 +283,22 @@ class ProcessMessageDelegate : public ClientHandler::ProcessMessageDelegate {
// No additional response args for this function
}
+ } else if (message_name == "DeleteFileOrDirectoryToTrash") {
@gruehle

gruehle Jan 3, 2013

Member

Looks like this name didn't get updated. Should be "MoveFileOrDirectoryToTrash".

@gruehle gruehle commented on an outdated diff Jan 3, 2013

appshell/appshell_extensions.cpp
@@ -283,6 +283,22 @@ class ProcessMessageDelegate : public ClientHandler::ProcessMessageDelegate {
// No additional response args for this function
}
+ } else if (message_name == "DeleteFileOrDirectoryToTrash") {
+ // Parameters:
+ // 0: int32 - callback id
+ // 1: string - filename
+ if (argList->GetSize() != 2 ||
+ argList->GetType(1) != VTYPE_STRING) {
+ error = ERR_INVALID_PARAMS;
+ }
+
+ if (error == NO_ERROR) {
+ ExtensionString filename = argList->GetString(1);
+
+ error = DeleteFileOrDirectoryToTrash(filename);
@gruehle

gruehle Jan 3, 2013

Member

Same here -- should be "MoveFileOrDirectoryToTrash()"

gruehle referenced this pull request May 13, 2013

Closed

Move to trash #245

Member

gruehle commented May 13, 2013

@leonuh - Sorry it has taken so long to get to this pull request! I merged with master, fixed some compile errors, and made a few changes to the mac implementation. Your original implementation, along with my modifications, are in a new pull request, #245.

I'm closing this pull request since it is superseded by #245.

gruehle closed this May 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment