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

feat(windows-trash): support for deleting to windows trash #243

Merged
merged 14 commits into from
Jan 3, 2024
Merged

feat(windows-trash): support for deleting to windows trash #243

merged 14 commits into from
Jan 3, 2024

Conversation

TheLeoP
Copy link
Contributor

@TheLeoP TheLeoP commented Dec 4, 2023

This only adds support for delete_to_trash to the Windows trash adapter. I'm not sure about how the other adapters (freedesktop and mac) work, but at a glance I think they allow restoring files from trash, browsing files from trash, etc. Am I correct? I would like to help with the Windows trash adapter as much as I can, but I'm a bit lost.

Thanks for the amazing pluging, by the way :3.

@stevearc
Copy link
Owner

stevearc commented Dec 5, 2023

Thanks for showing some interest in helping with Windows! Yes, the freedesktop and mac adapters support deleting to trash, viewing the trash, restoring from trash, and purging from trash. They work slightly differently because the freedesktop implementation has information about where the deleted files were deleted from. On mac that information is encoded in a proprietary format so we basically just have a single "trash bin" folder where everything goes. The mac implementation is a lot simpler for this reason, but also less featureful.

These adapters conform to the same structure as other adapters in oil

---@class (exact) oil.Adapter
---@field name string The unique name of the adapter (this will be set automatically)
---@field list fun(path: string, column_defs: string[], cb: fun(err?: string, entries?: oil.InternalEntry[], fetch_more?: fun())) Async function to list a directory.
---@field is_modifiable fun(bufnr: integer): boolean Return true if this directory is modifiable (allows for directories with read-only permissions).
---@field get_column fun(name: string): nil|oil.ColumnDefinition If the adapter has any adapter-specific columns, return them when fetched by name.
---@field normalize_url fun(url: string, callback: fun(url: string)) Before oil opens a url it will be normalized. This allows for link following, path normalizing, and converting an oil file url to the actual path of a file.
---@field get_entry_path? fun(url: string, entry: oil.Entry, callback: fun(path: string)) Similar to normalize_url, but used when selecting an entry
---@field render_action? fun(action: oil.Action): string Render a mutation action for display in the preview window. Only needed if adapter is modifiable.
---@field perform_action? fun(action: oil.Action, cb: fun(err: nil|string)) Perform a mutation action. Only needed if adapter is modifiable.
---@field read_file? fun(bufnr: integer) Used for adapters that deal with remote/virtual files. Read the contents of the file into a buffer.
---@field write_file? fun(bufnr: integer) Used for adapters that deal with remote/virtual files. Write the contents of a buffer to the destination.
---@field supported_cross_adapter_actions? table<string, oil.CrossAdapterAction> Mapping of adapter name to enum for all other adapters that can be used as a src or dest for move/copy actions.
---@field filter_action? fun(action: oil.Action): boolean When present, filter out actions as they are created
---@field filter_error? fun(action: oil.ParseError): boolean When present, filter out errors from parsing a buffer

Most likely the windows adapter will be more similar to mac than freedesktop, and it'll be easier to implement that way. So any M.function_name you see in the mac.lua file will probably need to have something equivalent in windows.

One note: vim.system is only in nightly right now, and oil still supports Neovim 0.8. For now, we should stick to the older APIs like jobstart

local function list_windows_drives(url, column_defs, cb)
local fetch_meta = columns.get_metadata_fetcher(M, column_defs)
local stdout = ""
local jid = vim.fn.jobstart({ "wmic", "logicaldisk", "get", "name" }, {

@TheLeoP
Copy link
Contributor Author

TheLeoP commented Dec 6, 2023

I just added support for viewing, restoring and purging windows trash :3.

The code is reaaaaaaaaally ugly right now and full of TODOs. Most of them are because of me not understanding if copying how the mac/freedesktop adapter works for certain things is ok (is_modifiable, get_column, normalize_url, render_action). I'll check how thesea are supposed to work and refactore the code soon.

Some of the TODOs are because I'm not sure about how should I handle the copy and create actions (from a quick glance, it seems like the mac and freedesktop adapters just work like for any regular file).

Do you maybe have some early feedback?

Also, I'm unable to run the tests localy because, among other thigs, plenary.nvim does not support running tests on windows :c.

Edit: recentrly I learned to use vim.lpeg and got really exited about it, so I was using it to parse the deleted date given by powershell until I realize that it is not present on 0.8. So, I added the option to parse those dates using a much simpler (in hindsight) lua pattern. I didn't deleted the vim.lpeg implementation only because I had already wrote it, but it can be deleted if you would preffer it that way.

Copy link
Owner

@stevearc stevearc left a comment

Choose a reason for hiding this comment

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

Making good progress! Left some comments. You'll also want to take the M.filter_action from freedesktop. Do you happen to know if there are any uniqueness guarantees for the names of files? If you delete two files with the same name, how do they appear?

lua/oil/adapters/trash/windows.lua Outdated Show resolved Hide resolved
lua/oil/adapters/trash/windows.lua Show resolved Hide resolved
lua/oil/adapters/trash/windows.lua Show resolved Hide resolved
lua/oil/adapters/trash/windows.lua Outdated Show resolved Hide resolved
lua/oil/adapters/trash/windows.lua Outdated Show resolved Hide resolved
lua/oil/adapters/trash/windows.lua Outdated Show resolved Hide resolved
lua/oil/adapters/trash/windows.lua Outdated Show resolved Hide resolved
lua/oil/adapters/trash/windows.lua Outdated Show resolved Hide resolved
Comment on lines 301 to 302
elseif action.type == "copy" then
-- TODO: ...
Copy link
Owner

Choose a reason for hiding this comment

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

Copy actions are a little weird for a trash adapter. Why would you want to restore a file but keep a copy in the trash? Why would you want to copy a file into the trash?

If Windows doesn't have a good way to perform these actions we can handle them differently. Maybe turn them into no-ops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was scared of using the luv functions directly with files from the recycle bin (or the oil utiliy functions), so wanted to try if using regular poweshell operations worked first. It seems like using luv (and oil utility functions) directly does work to move/rename/delete this files, so I'll change that to (the windows adapter will end up looking a lot like the freedesktop one xd).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I found this random answer in stackoverflow about how recycle bin works on Windows. Would it be preffered to handle this in a low level way like in the freedesktop implementation? Given the lack of proper Windows documentation, I'm inclined to let poweshell send files to the recycle bin, but use a implementation similar to the freedesktop one for moving, coping and purging files (I'm now realizing that this would mean not only handling the deleted file itself, but also the file with the metadata, just like on freedesktop)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed my last changes doing exactly this, let me know your thoughts on the matter. Powershell is still needed for listing recycle bin files and for sending files to the trash bin

lua/oil/adapters/trash/windows.lua Outdated Show resolved Hide resolved
@TheLeoP
Copy link
Contributor Author

TheLeoP commented Dec 10, 2023

Do you happen to know if there are any uniqueness guarantees for the names of files?

For the metadata that I'm currently querying, this is the result for two files with the same name, deleted from the same location:

[
  {
    "OriginalPath": "C:\\Users\\pcx\\Desktop\\a.txt",
    "Name": "a.txt",
    "ModifyDate": "2023-12-10T11:59:52",
    "Path": "C:\\$Recycle.Bin\\S-1-5-21-2576758376-3460131066-3669222130-1001\\$RIZBAJN.txt",
    "IsFolder": false
  },
  {
    "OriginalPath": "C:\\Users\\pcx\\Desktop\\a.txt",
    "Name": "a.txt",
    "ModifyDate": "2023-12-10T11:59:59",
    "Path": "C:\\$Recycle.Bin\\S-1-5-21-2576758376-3460131066-3669222130-1001\\$RXB3SZK.txt",
    "IsFolder": false
  }
]

The Name itself isn't unique, but the Path is. With the current implementation, this results in only one of the two files being shown in :Oil --trash.

@stevearc
Copy link
Owner

Got it, so we'll want to handle this the same was as the freedesktop implementation. That means including this filter_error function

---@param err oil.ParseError
---@return boolean
M.filter_error = function(err)
if err.message == "Duplicate filename" then
return false
end
return true
end

When you call cache.create_entry, pass in the Path for the name because cache entries must have a unique name. You may need to do some sanitization to remove the path separators.

In the metadata, set the display_name = Name (you're already doing this).

@TheLeoP
Copy link
Contributor Author

TheLeoP commented Dec 12, 2023

Also, the parts of the code that use powershell are noticeably slower (I guess that spawning a powershell process for each operation may be expensive). I was thinking about having two background powershell proccesses always listening for input (this processes would be started either when the first command that needs them is executed or at startup), one for the 0 namespace (Windows desktop) and one for the 0xa namespace (Recycle bin). The recycle bin would be the easiest, since there is no need to parse any output, just continously send commands to it, but the Windows Desktop one would need for us to parse the output as it comes.

What are your thoughts on this?

@stevearc
Copy link
Owner

Can't do a full review right now, but I'd say that unless it is unusably slow, I would lean towards making this PR focus on correct behavior over performance. It's easy enough to add those sorts of performance improvements later.

@TheLeoP
Copy link
Contributor Author

TheLeoP commented Dec 21, 2023

Is there something this PR may be missing? If the performance can be improved in a following PR, I think this PR is behaving as expected

@stevearc
Copy link
Owner

I don't see anything obvious! I'm traveling and don't have my windows machine with me, but I'll test this out next year and see about merging it

@stevearc
Copy link
Owner

stevearc commented Jan 3, 2024

Tested it out and it seems to work! I see what you mean about the performance; it is a bit slow for some of the operations. I think this is great for a first revision and I'm happy to take additional changes to speed it up. Thanks for the contribution!

@stevearc stevearc merged commit 553b7a0 into stevearc:master Jan 3, 2024
7 checks passed
@TheLeoP TheLeoP deleted the windows-trash-support branch January 3, 2024 11:04
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.

2 participants