CodeEditApp
delete warning message uses enumerator instead of shallow search
Description
The delete function shows a warning before deleting files with the amount of files being deleted. It initially used fileManager.contentsOfDirectory() which only performs a shallow search and ignores files nested into folders it finds, producing an incorrect count of files. My change uses enumerator instead to get a deeper search of files and produce a correct file count in the warning.
Related Issues
- closes #1695
Checklist
- [x] I read and understood the contributing guide as well as the code of conduct
- [x] The issues this PR addresses are related to each other
- [x] My changes generate no new warnings
- [x] My code builds and runs on my machine
- [x] My changes are all related to the related issue above
- [x] I documented my code
Screenshots
How well does this perform if the folder contains many files, like a node_modules folder? I'd be willing to sacrifice exact file count for speed if a case like that is slow.
How well does this perform if the folder contains many files, like a node_modules folder? I'd be willing to sacrifice exact file count for speed if a case like that is slow.
the wait time is basically non existant, and I think accuracy is very valuable, in this case a user would have thought they were only deleting 20 files when they were actually deleting thousands.
https://github.com/CodeEditApp/CodeEdit/assets/118622417/613b07b6-b5ba-4d68-b00b-a43409c54272
https://github.com/CodeEditApp/CodeEdit/assets/35942988/bc362bac-19da-418d-a02b-498aafc9ad19
I'm thinking about this type of case, where there's many many nested directories. I do see your point about accuracy, but to keep the UI from ever beach-balling it should have an upper limit. Could we make this slightly more complicated and add a time limit to the enumerator? Something like:
let enumerator = fileManager.enumerator(atPath: file.url.path)
let start = CACurrentMediaTime()
var childrenCount = 0
var timedOut = false
// max out at half a second of loading, a little lag but (hopefully) rare.
while enumerator?.nextObject() != nil {
childrenCount += 1
if CACurrentMediaTime() - start > 0.5 {
timedOut = true
break
}
}
if timedOut {
messageText = "Failed to count items in time, this may be a large directory. Are you sure you want to delete the selected items?"
...
I have no idea if that compiles but you get what I mean.
Is it really important for developers to know how many files are deleted?
I believe when someone deletes things, they should be informed what is being deleted so they don't accidentally delete more than they intended, especially in the "Delete Permanently" option. we already count and show the amount of files, we might as well do it accurately.
I agree with @activcoding, The name of the folder being deleted is much more important than the count. Finder shows the following alert when deleting immediately...
Then it opens a small window which tells you the items remaining. We could do this in the activity viewer when it is finished. @armartinez is working on that now.
I see that Xcode has this alert.
...which has the count. I wonder if the count is included here because in Xcode you can just remove references.
For the record VS Code and Nova does not display the count.
@thecoolwinter, @activcoding, @knotbin What do y'all think?
@austincondiff theres also a chance it's using spotlight metadata to get that count but yeah could be because it's using Xcodes logical representation rather than the file system
I think if we want to be as close to Xcode UI as possible, we should display the count.
I think the best warning message would be: “Are you sure you want to delete “Folder” with all of its subdirectories?”
I think the best warning message would be: “Are you sure you want to delete “Folder” with all of its subdirectories?”
That's more descriptive, but having a big number display might make a user think twice before accidentally deleting an important directory. I agree with @knotbin's point that more accuracy for file count helps avoid errors.
@thecoolwinter while I agree that we need something descriptive, it doesn't necessarily need to recursively get the number all children. Can you imagine if a child was a node_modules folder? In this case, some arbitrary number isn't going to provide much information.
As a user, I might expect to see a message indicating what is about to be deleted. Currently, we only indicate a number. In my opinion, a number is not as good of an identifier as a name is.
Now, if a user selects multiple items, then it will only count the top-level items and use this in place of the name.
When only one file or folder is going to be deleted immediately, it should read…
Are you sure you want to delete "CodeEdit-test"? This item will be deleted immediately. You can't undo this action.
When more than one file or folders is going to be deleted, it should read…
Are you sure you want to delete the 2 selected items? 2 items will be deleted immediately. You can't undo this action.
Replace 2 with the number of selected items not including items in each selected folder.
[!NOTE] Moving to the trash should not warn users.
Sorry to pivot like this here. We typically shouldn’t plan features in the PR process, but this wasn’t properly thought all the way through.
currently you cannot select multiple files in the navigator @austincondiff so that would not be a possibility
We should probably add the ability to select multiple items in the project navigator, for example if the user wanted to delete multiple items at once. If multiple items are selected, we should disable all non-applicable items in the context menu. This should be handled in a separate PR though.