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

mac: Return trash items that are created by delete #128

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

eugenesvk
Copy link
Contributor

@eugenesvk eugenesvk commented Dec 4, 2024

Builds upon the previous Linux PR for easier rebasing, will add comment to the existing PR not to split the discussion

#109

  • add macos functions that return trashed items: optionally if called via _with_info functions (for both Finder with AppleScript and FS API), so there is no mandatory overhead for long lists

    • ✗ couldn't find a way to get OS trash timestamp even though it exists (Finders knows it, but the metadata field is returned empty for us, maybe this is also hidden in DSStore???), so using the same logic of calculating our own timestamps like for Linux
  • added a few tests for both trash types (though only checking return item's name/parent as time/path@trash can be inconsistent, so no guaranteed way for us to compare them)

  • added stub with_info function arguments for Linux/Win, but the actual logic is not implemented

  • added the new configurable faster/better method of launching AppleScript via direct Objc bindings to the automation kit, but unfortunately it seems to be buggy: script execution can stall for the default timeout of 2mins and then fail with execution error: Finder got an error: AppleEvent timed out.. So left the old option via cli as the default, but then stdout was eating quotation marks separating paths, so had to manually insert /// as list separators:

    • ?: can there be a "natural" path that contains ///?

(here is a much simpler test that doesn't delete any files via Finder #133 but still get this weird bug sometimes.)

@eugenesvk eugenesvk force-pushed the fr-mac-return-url branch 4 times, most recently from 886c443 to e2d17ed Compare December 5, 2024 10:13
@eugenesvk eugenesvk marked this pull request as ready for review December 5, 2024 10:30
@eugenesvk eugenesvk marked this pull request as draft December 6, 2024 13:53
@eugenesvk eugenesvk force-pushed the fr-mac-return-url branch 2 times, most recently from e4288c1 to ab7ca81 Compare December 6, 2024 16:23
@eugenesvk eugenesvk marked this pull request as ready for review December 6, 2024 18:18
@eugenesvk eugenesvk force-pushed the fr-mac-return-url branch 2 times, most recently from c6d2817 to d2189d1 Compare December 7, 2024 16:03
Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this.

Unfortunately master now is incompatible with this branch so it would need another round of rebasing and conflict resolution.

Regarding osakit, since it's also not really working, let's remove it right away and keep this PR focussed on returning the deleted items.

Thanks again.

actual deleted time as recorded in file metadata (`kMDItemDateAdded`) seems inaccessible to us when a file is in the trash, so this follows the Linux approach of getting system time instead
instead of currently returning a vec just because 1 delete still goes through delete_all
…hed items and those that done

so that you can avoid the overhead of storing all the info you might not need
replace cmd osascript with Objc bindings with osakit to allow proper results parsing
@eugenesvk
Copy link
Contributor Author

Rebased.
And it's too early to kill osakit :), it turns out the issue is in cargo's test harness: macos applescript thingies must be run on the main thread, and tests don't allow that (see mdevils/rust-osakit#2)
So the stalling issue won't appear for the main trash app, will update the docs then, but leave the cli method just in case someone would need to run trashing on a different thread

Do you know how to force tests to use the main thread?

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for setting this up!

And that's fair enough, let's keep osakit as it may be useful to some.

Do you know how to force tests to use the main thread?

First of all, I think you should be able to check if the current thread is the main thread.

use std::thread;

fn main() {
    println!("Is this the main thread? {:?}", thread::current().name());
}
Is this the main thread? Some("main")

Then the test can bail if it's not main, but be enabled by default.
It is possible to restrict the amount of threads to run the tests to 1, but I don't know if that will be the main thread.

src/macos/mod.rs Show resolved Hide resolved
src/macos/mod.rs Show resolved Hide resolved
src/macos/mod.rs Show resolved Hide resolved
@eugenesvk
Copy link
Contributor Author

It is possible to restrict the amount of threads to run the tests to 1, but I don't know if that will be the main thread.

no it unfortunately won't due to a regression in Rust, but it seems the alternative testing crate could add support for this nextest-rs/nextest#1959

@eugenesvk eugenesvk force-pushed the fr-mac-return-url branch 3 times, most recently from 378e8cb to be2fb0c Compare December 9, 2024 11:08
…a separate workflow item

with a custom argument (also requires a custom harness supporting main threads)
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