Skip to content

Conversation

petersamokhin
Copy link
Contributor

Hello and thanks a lot for your work!

This PR adds an option to reveal multiple files, instead of just one.
Given the contracts of the methods used under the hood on all platforms, the single path was always wrapped into an array anyway.

Tested manually on macOS (and it works) — but I can also try Windows and Linux.

Please let me know where to add a change log entry, as currently it is a bit inconsistent between the plugins, and seems to be semi-automatic — I decided it's better to confirm it first.

Thanks!

@petersamokhin petersamokhin requested a review from a team as a code owner August 5, 2025 01:47
Copy link
Contributor

@Legend-Master Legend-Master left a comment

Choose a reason for hiding this comment

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

No a proper review yet, just some early feedbacks

Copy link
Contributor

github-actions bot commented Aug 5, 2025

Package Changes Through 2753528

There are 6 changes which include nfc with patch, nfc-js with patch, opener with minor, opener-js with minor, store with minor, store-js with minor

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
api-example 2.0.32 2.0.33
api-example-js 2.0.28 2.0.29
opener 2.4.0 2.5.0
opener-js 2.4.0 2.5.0
nfc 2.3.0 2.3.1
nfc-js 2.3.0 2.3.1
store 2.3.0 2.4.0
store-js 2.3.0 2.4.0

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@Legend-Master
Copy link
Contributor

Also just a question, should we support supplying paths with different parent directories here? That would open multiple windows I think but could be fine

@petersamokhin
Copy link
Contributor Author

@Legend-Master my testing shows:

  • macOS: works out of the box
  • Linux (Ubuntu 22.04 desktop): works out of the box
  • Windows: doesn't work (yet?).
    • The method that is being used forces us to provide the parent directory, so we handle this on the code level and filter out the files that are not placed together with the 1st file in the list.
    • Do you think it makes sense to complicate the logic and somehow handle it manually? TBH I don't see a smooth way to do this. I didn't test it on Windows, but on macOS if you call that method concurrently, it crashes without an option to catch the error. I assume on Windows it won't be reliable neither, if we just wrap it into a loop the same way. If the method is sync and we await its completion, it might work, but again doesn't look reliable + might take time on some devices. So, if there's no native method that supports this behaviour, I'd rather keep it as is, so it won't work only for Windows.

@Legend-Master
Copy link
Contributor

Makes sense, we could document this behavior though

@petersamokhin
Copy link
Contributor Author

@Legend-Master I added notes about this Windows-specific behaviour to the fn docs and to README

@Legend-Master
Copy link
Contributor

Legend-Master commented Aug 6, 2025

@Legend-Master I added notes about this Windows-specific behaviour to the fn docs and to README

Would be nice to also include it in the rust functions 🙃 never mind, I see cbd918e

@Legend-Master
Copy link
Contributor

Legend-Master commented Aug 6, 2025

Since I can't push to your branch, I opened getcompress#1 against this branch to support multiple roots on Windows

@petersamokhin
Copy link
Contributor Author

petersamokhin commented Aug 6, 2025

@Legend-Master I've approved your PR and seems like it fixes the rest of your comments, right?
Only question is that you removed some explanatory comments from code, shall we revert them?

UPD: actually the build was failing outside windows config, so I fixed it by adding dunce to all configs on the opener crate level

UPD2: tested your changes on Windows, it works 🚀

@Legend-Master
Copy link
Contributor

Legend-Master commented Aug 7, 2025

I've approved your PR and seems like it fixes the rest of your comments, right?

Yep

Only question is that you removed some explanatory comments from code, shall we revert them?

The limitation is now gone, so I removed those comments

UPD: actually the build was failing outside windows config, so I fixed it by adding dunce to all configs on the opener crate level

UPD2: tested your changes on Windows, it works 🚀

👍

Legend-Master
Legend-Master previously approved these changes Aug 9, 2025
Copy link
Contributor

@Legend-Master Legend-Master left a comment

Choose a reason for hiding this comment

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

Goods good, we just need a change file and format the plugins/opener/guest-js/index.ts, and we are good to go

https://github.com/tauri-apps/plugins-workspace/blob/v2/.changes/readme.md

@petersamokhin
Copy link
Contributor Author

@Legend-Master done 🫡

Copy link
Contributor

@Legend-Master Legend-Master left a comment

Choose a reason for hiding this comment

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

Thanks!

@Legend-Master Legend-Master merged commit b8056f4 into tauri-apps:v2 Aug 9, 2025
20 checks passed
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