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

eugenesvk · 2024-12-04T18:16:28Z

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

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, think it should be the new default, but for now left the old method (via separate proc) as the default.
- Although stdout is eating quotation marks separating paths, so had to manually insert /// as list separators:
  - ?: can there be a "natural" path that contains ///, meaning this is a buggy separator?

Byron

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

since osakit can bug, but retain it as an option for those who might not encounter such bugs

disabled as it can timeout

eugenesvk · 2024-12-08T17:59:43Z

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?

Byron

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

eugenesvk · 2024-12-09T07:08:44Z

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's now resolved via the help from nextest-rs/nextest#1959 by using libtest-mimic instead of the default test harness

to force it run on the main thread

to make sure there are no threading issues

…a separate workflow item with a custom argument (also requires a custom harness supporting main threads)

joshuamegnauth54 · 2024-12-24T07:59:23Z

I wrote a unit test for #109 on the Freedesktop side, so you may be able to make use of that too. I didn't notice this PR until after I wrote it, but it shouldn't conflict with your work (I think).

Add unit test for delete-info jackpot51/trash-rs#1

eugenesvk force-pushed the fr-mac-return-url branch 4 times, most recently from 886c443 to e2d17ed Compare December 5, 2024 10:13

eugenesvk marked this pull request as ready for review December 5, 2024 10:30

eugenesvk marked this pull request as draft December 6, 2024 13:53

eugenesvk force-pushed the fr-mac-return-url branch 2 times, most recently from e4288c1 to ab7ca81 Compare December 6, 2024 16:23

eugenesvk marked this pull request as ready for review December 6, 2024 18:18

eugenesvk force-pushed the fr-mac-return-url branch 2 times, most recently from c6d2817 to d2189d1 Compare December 7, 2024 16:03

Byron requested changes Dec 8, 2024

View reviewed changes

eugenesvk added 18 commits December 8, 2024 18:24

Return trash items that are created by delete (old FR)

69e74a5

fix mac return signatures

8e5cb9d

update doc

029e5e9

dep: add chrono to macOS

3cd198e

add support for getting trashed items' data

6cc803b

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

return a single TrashItem for a single trashed item

ffe57a4

instead of currently returning a vec just because 1 delete still goes through delete_all

update docs

9a56e8b

__TODO why is this needed?

db0868b

mac: split delete functions into 2 categories: those that return tras…

251e786

…hed items and those that done so that you can avoid the overhead of storing all the info you might not need

fmt

d20a453

fix wrong delete arg order

a2db980

add stub arguments for Windows/Linux

bcb755e

fmt

34fe204

upd comment

3c393de

test: add test_delete_binary_path_with_ns_file_manager_with_info

17f212b

dep: add osakit to parse AppleScript's output

e9717d4

update description

1609ec1

mac: add support for Finder returned url to trashed items

5576cf7

replace cmd osascript with Objc bindings with osakit to allow proper results parsing

eugenesvk added 9 commits December 8, 2024 18:24

test: add 1-file delete test to test_delete_with_finder_with_info

9021350

fmt

cb7550a

fix variable type detection in AppleScript

7637858

set an empty return to AS with no info so it can be parsed by osakit

91b0bc6

test: add test_delete_with_finder

bc43ab2

add ScriptMethod

d2eaf19

add back cli applesript runs

401d5e3

since osakit can bug, but retain it as an option for those who might not encounter such bugs

test: update

b55ddca

test: add test_delete_with_finder_osakit_with_info

4e5c6e6

disabled as it can timeout

eugenesvk force-pushed the fr-mac-return-url branch from 8644079 to 4e5c6e6 Compare December 8, 2024 11:24

update doc

29c7a62

update doc to reflect the need to run Finder scripts on the main thread

8c69c26

Byron reviewed Dec 9, 2024

View reviewed changes

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 force-pushed the fr-mac-return-url branch from 027a5b3 to afc13df Compare December 9, 2024 07:15

eugenesvk added 4 commits December 9, 2024 17:54

test: move osakit to a separate test with no harness

1ec7904

to force it run on the main thread

test: update osakit

674d7be

test: run threading tests in a short loop

bbb1289

to make sure there are no threading issues

dep: add custom test harness supporting forcing main thread

2ee8045

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

move tests behind macos cfg compilation

369c4c8

eugenesvk force-pushed the fr-mac-return-url branch from be2fb0c to 369c4c8 Compare December 9, 2024 11:15

update comment

41acbf1

eugenesvk mentioned this pull request Dec 9, 2024

workflow: split running tests required to be on the main thread #134

Closed

eugenesvk added 2 commits December 9, 2024 22:13

test: fail unless on main thread

a166605

workflow: split running tests required to be on the main thread into …

75a23d2

…a separate workflow item with a custom argument (also requires a custom harness supporting main threads)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eugenesvk commented Dec 4, 2024 •

edited

Loading

Byron left a comment

eugenesvk commented Dec 8, 2024

Byron left a comment

eugenesvk commented Dec 9, 2024 •

edited

Loading

joshuamegnauth54 commented Dec 24, 2024

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

Are you sure you want to change the base?

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

Conversation

eugenesvk commented Dec 4, 2024 • edited Loading

Byron left a comment

Choose a reason for hiding this comment

eugenesvk commented Dec 8, 2024

Byron left a comment

Choose a reason for hiding this comment

eugenesvk commented Dec 9, 2024 • edited Loading

joshuamegnauth54 commented Dec 24, 2024

eugenesvk commented Dec 4, 2024 •

edited

Loading

eugenesvk commented Dec 9, 2024 •

edited

Loading