Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Directory and file delete to trash #149

Closed
wants to merge 8 commits into from
16 changes: 16 additions & 0 deletions appshell/appshell_extensions.cpp
Expand Up @@ -281,6 +281,22 @@ class ProcessMessageDelegate : public ClientHandler::ProcessMessageDelegate {

error = DeleteFileOrDirectory(filename);

// No additional response args for this function
}
} else if (message_name == "DeleteFileOrDirectoryToTrash") {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Same here -- should be "MoveFileOrDirectoryToTrash()"


// No additional response args for this function
}
} else if (message_name == "ShowDeveloperTools") {
Expand Down
25 changes: 22 additions & 3 deletions appshell/appshell_extensions.js
Expand Up @@ -19,7 +19,7 @@
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
*
*/
*/

// This is the JavaScript code for bridging to native functionality
// See appshell_extentions_[platform] for implementation of native methods.
Expand All @@ -40,7 +40,7 @@ if (!appshell.fs) {
if (!appshell.app) {
appshell.app = {};
}
(function () {
(function () {
// Error values. These MUST be in sync with the error values
// at the top of appshell_extensions_platform.h.

Expand Down Expand Up @@ -295,7 +295,7 @@ if (!appshell.app) {
};

/**
* Delete a file.
* Delete a file permanently.
*
* @param {string} path The path of the file to delete
* @param {function(err)} callback Asynchronous callback function. The callback gets one argument (err).
Expand All @@ -312,6 +312,25 @@ if (!appshell.app) {
appshell.fs.unlink = function (path, callback) {
DeleteFileOrDirectory(callback, path);
};

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

Choose a reason for hiding this comment

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

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

appshell.fs.moveToTrash = function (path, callback) {
DeleteFileOrDirectoryToTrash(callback, path);
};

/**
* Return the number of milliseconds that have elapsed since the application
Expand Down
25 changes: 19 additions & 6 deletions appshell/appshell_extensions_mac.mm
Expand Up @@ -460,21 +460,34 @@ int32 DeleteFileOrDirectory(ExtensionString filename)
NSString* path = [NSString stringWithUTF8String:filename.c_str()];
BOOL isDirectory;

// Contrary to the name of this function, we don't actually delete directories
if ([[NSFileManager defaultManager] fileExistsAtPath:path isDirectory:&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;
}
return ERR_NOT_FILE;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}
} else {
return ERR_NOT_FOUND;
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately NSWorkspaceRecycleOperation didnt work :(

return NO_ERROR;

return ConvertNSErrorCode(error, false);
}


void NSArrayToCefList(NSArray* array, CefRefPtr<CefListValue>& list)
{
for (NSUInteger i = 0; i < [array count]; i++) {
Expand Down
2 changes: 2 additions & 0 deletions appshell/appshell_extensions_platform.h
Expand Up @@ -82,6 +82,8 @@ int32 SetPosixPermissions(ExtensionString filename, int32 mode);

int32 DeleteFileOrDirectory(ExtensionString filename);

int32 DeleteFileOrDirectoryToTrash(ExtensionString filename);

void OnBeforeShutdown();

void CloseWindow(CefRefPtr<CefBrowser> browser);
Expand Down
17 changes: 17 additions & 0 deletions appshell/appshell_extensions_win.cpp
Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

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.


return NO_ERROR;
}

void OnBeforeShutdown()
{
Expand Down