Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

add implementation to save files #100

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

Conversation

santiwanti
Copy link
Collaborator

@santiwanti santiwanti commented Dec 13, 2023

Addresses Issue #8

Milestones:

  • Android
  • Web The Web implementation was removed during a refactor due to not being able to implement it.
  • JVM
  • iOS
  • macOS
    • v1
    • Add support for more customization e.g. prompt, message, nameFieldLabel
    • Testing

@santiwanti
Copy link
Collaborator Author

@shalva97 @Wavesonics I got save working and tested on all platforms except macOS. On macOS it is implemented, but I haven't been able to test it for some reason. Running the example compiles it successfully, but it doesn't launch. Could it be because I'm running on Arm and not x64?

I will keep trying to get macOS working and tested. In the meantime any info on whether this feature is wanted in the library or if it should be kept in a fork or any feedback on the implementation details would be greatly appreciated.

Cheers

@akriese
Copy link

akriese commented Jan 11, 2024

Hi @santiwanti, I am also interested in the saveFilePicker you are implementing. I am wondering if the API youre implementing currently fits the other functions the package offers. Specifically, I would say, the picker should not handle the saving of the contents itself. It should instead take an onFilePicked-function as input which would be provided by the user where the user decides how to work with the selected file.
I'd like to contribute to this to push this PR through, so please tell me, if I can help you in any way :)

@santiwanti
Copy link
Collaborator Author

@akriese that's a good point. I'm refactoring the code now so that the callback is the same as for the other functions and also refactoring the name from SaveFilePicker to NewFilePicker although I don't love this name because the user could select an existing file.

As for contributing I will implement these changes today and after that the only things left will be to test it on MacOS which I haven't been able to do and getting feedback from maintainers to see if anything should be changed. I will ping you once it's ready to test on MacOS.

@akriese
Copy link

akriese commented Jan 11, 2024

A colleague of mine uses macOS (x86, I guess). I could let him test your code, if you give us instructions on how to test it. Can one simply clone the repo and run the example apps?

@akriese
Copy link

akriese commented Jan 11, 2024

I personally think SaveFilePicker is a good name, as that is usually what you're doing with the picker. Also, it is consistent with the TinyFileDialogs.tinyfd_saveFileDialog you're using, so I dont see a reason to rename it to NewFilePicker :)

With other filepickers it is similar, that the programmer (user of the package) has to handle file overwrites. At least thats how you have to do it with Swing's JFileChooser().showSaveDialog() too.

@santiwanti
Copy link
Collaborator Author

I have refactored the code so that the user can choose to do once the new file has been created.

The Web code had to be removed and now throws a NotImplemented Exception because there is no way to implement it currently.

Besides this everything else is the same.

The UX that is a bit strange is the iOS UX because the way it is implemented is that a temporary file is created and then the user can decide where to move the file. That means that the file picker action the user sees is "Move" which is a bit deceptive.

Lastly @akriese the Mac code has still not been tested because I can't run it for some reason. To test the app you have to go through the instructions laid out here: https://www.jetbrains.com/help/kotlin-multiplatform-dev/compose-multiplatform-getting-started.html and after that it's as easy as running the code.

@akriese
Copy link

akriese commented Jan 16, 2024

Hey, sorry for letting you wait. I just tried the jvm example. The example is missing the showSaveFile = false in the callback body but otherwise it seems to work fine.
I'll ask my friend now to try it on Mac and will report back hen we have a result.

@akriese
Copy link

akriese commented Jan 16, 2024

So, on his mac the jvm version works, but sometimes (we cant really tell when), picking an existing file will cut off the file extension of that file.
The macOS version does not run at all, as it was the case for you. It compiles and when running it, it says the execution is finished without a window popping up. He is using an ARM mac too though, so we cant exclude the possibility of that being a problem...

@shalva97
Copy link
Collaborator

shalva97 commented Jan 21, 2024

thanks for all the work. I think it will be okay to keep both file picker dialogs and save file dialogs in this library.

First I'd like to merge #89, it will cause conflicts, but should not be much work to fix it.

Regarding MacOS, it needs one more compilation target macosArm64 to be added. Anyways, I have MacOS with intel so I can test it on macosX64.

…pfilepicker/android/MainActivity.kt

Co-authored-by: George Shalvashvili <[email protected]>
@santiwanti
Copy link
Collaborator Author

@shalva97 I didn't want to add the macosArm64 target given that it wasn't already in the project. If you think adding the target is a good idea I would be willing to add it in another PR and test/fix all the logic. In the meantime if you can test the macos saving code that would be great. I can fix the bugs that come up and we can find a way to collaborate more synchronously if it's not working.
I will keep an eye out on #89 and update this PR once that PR is merged.

@shalva97
Copy link
Collaborator

shalva97 commented Jan 25, 2024

Hi @santiwanti. Do you want to be a maintainer? I will ask @Wavesonics, he usually responds on Kotlin's Slack

Personally I dont use this library, there was just my pet project, which used it but I have abandoned that.

@akriese
Copy link

akriese commented Jan 29, 2024

Hey @santiwanti, I just noticed that #89 was merged. Could you fix conflicts so @shalva97 can merge this at some point? :)

@santiwanti
Copy link
Collaborator Author

Sorry I was afk for a few days.

@shalva97 I would be down to being a maintainer, but lets talk about it more and with @Wavesonics to make sure we are on the same page and don't step on eachothers toes. You can message me on Slack @ swanti.

@akriese I will be back in full force tomorrow and will fix conflicts and push the changes.

@santiwanti santiwanti marked this pull request as ready for review February 2, 2024 12:27
@santiwanti
Copy link
Collaborator Author

@shalva97 @Wavesonics this still has to be tested on an Intel Mac. I haven't been able to test it myself

@santiwanti
Copy link
Collaborator Author

I will be working on #113 so that I can test the MacOS changes. If someone can test the save file picker on an intel mac before that and confirm that it works I will merge this PR. If not, I will merge it after #113 is merged and I can test the save file picker on mac myself

@shalva97
Copy link
Collaborator

shalva97 commented Feb 6, 2024

it fails to save the file, but it does return the path, so I would say It works on my intel Mac.
Screenshot 2024-02-06 at 22 27 25

The rest looks okay 🚀

santiwanti and others added 2 commits February 8, 2024 17:04
…ries/mpfilepicker/FilePicker.macos.kt

Co-authored-by: George Shalvashvili <[email protected]>
…ries/mpfilepicker/FilePicker.macos.kt

Co-authored-by: George Shalvashvili <[email protected]>
@zhelenskiy
Copy link

I'm interested in the feature, and I have x64 win 11, Linux (Kubuntu) and Mac, and ARM Mac. If you need any of the platforms, I can check it.

@shalva97
Copy link
Collaborator

can confirm, creating and writing some nice text in a file did work.
Screenshot 2024-02-23 at 23 23 41

@vinceglb vinceglb linked an issue Mar 6, 2024 that may be closed by this pull request
@zhelenskiy
Copy link

Are there any updates?

@zhelenskiy
Copy link

@santiwanti @Wavesonics are there any updates? Can I somehow help you? Is there any ETA?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offer a dialog to save files as well
4 participants