Open newhinton opened 1 year ago
Hi @newhinton . Thank you very much for the PR. It looks good. I left a few comments which I think should be addressed. Also I believe we should also handle States to keep the behaviour consistent with the Play Store version before we can release it otherwise the UI will be incomplete.
Regarding removing support for Google Drive and using this one instead I'm not sure it's the right approach for the Play Store release since it's a little bit more involved to configure (although more versatile).
The thing with google-drive is that we lock people to google drive on the play version.
I guess it would be possible to create a selector somewhere that switches "SaveSyncManagerImpl", but in the long term there are two implementation that both require maintenance, and the google-drive-one is a subset of SAF.
Does the google drive implementation handle deletes properly?
I left a few comments
Sorry, but what comments? I cant see any...
@newhinton No. Sadly the current Google Drive implementation doesn't handle deletes. Deleted files will basically appear again at the next sync from the other devices. Handling those is possible, but would require to add a db table to keep track of them.
Generally i think we should try to unify play and free variants. It will reduce the codebase, and since external storage also allows access to google drive, we are not artificially locking in users to just one cloud service. With this implementation, only f-droid users will get access to SAF, and google play users HAVE to use google drive.
So my suggestion to this issue is to create a "bar" that is only available to the play-variant, that will explain that in future versions the google-drive integration will be removed in favor of SAF. Then users will know whats going on, and then we promt them to update their settings when we actually switch.
Note to me:
The "computeSavesSpace" might throw an exception. Needs investigating&catching
"Complex" Cores need special handling, see citra which has folders instead of files. This does not work with my implementation.
@Swordfish90 So, states are now supported aswell! However, i can currently only do all states. So the system-chooser has no actual functionality.
There is still a bug somewhere. With each successive run of the sync, it creates a new folder, aka. "Saves (1)" so that there are many duplicate folders. I think i know why, but i need to investigate.
Edit:
~Actually, i have no clue. If someone with deep knowledge about the SAF could help out, that would be great.~
I also implemented storage calculation. I used the "remote" location, because i assumed that this would be the important location, however, it seems that the original implementation had the local storage in mind. Please advise me which location to count.
With this, i should have reached feature parity to the google implementation.
I still think we should switch the google play variant over in the long term, the benefits are that there are no two implementations that need to be maintained, since SAF already allows google drive usage. Also the google play variant severely limits users in their choice of remotes.
Is this PR still being worked on?
I just found out the hard way that non-savestate saves don't seem to work for my gba games, losing hours of play, which I believe this aims to resolve.
If this PR is not being worked on anymore, or is gonna take a long time, it would not be a bad idea to at least display a warning in the app about the lack of functional saving/the risk of losing saves.
This pull request is done, at least from my side. I have found a good way, but it needs to be reviewed and merged. Though i dont know if it solves your specific problem, and it will only be available on the f-droid-release.
I struggle to understand why such a basic feature isn't implemented already, but thanks @newhinton . I built a copy of your branch, and it works great.
Note: Lemuroid is struggling to load external changes to the save-files. Sort of understandable because Lemuroid doesn't officially support external save-files. Just a nice to know for others building this branch, or for the Lemuroid maintainers/newhinton before/if they decide to merge this
@newhinton @Swordfish90 Is anything blocking these changes getting merged? Would be great to have this feature
@johntringham Not really, at least not from my side. I've been using it. It needs a review to check for bugs, but otherwise there are no blockers
This would be a killer feature for an already excellent emulation project. I'd really like to be able to sync saves to my Nextcloud across devices.
I haven't had a chance to test this branch yet, but just adding that this sort of change would be really beneficial for my use case.
I am trying to avoid Google wherever possible, so my setup is running GrapheneOS and I use Syncthing for copying files back and forth from my computers, including ROMs. It would be really nice if my saves/savestates could just be included as part of that.
I appreciate all the work that goes into this project and even without being able to backup saves (in the way that I prefer), it's still probably the best emulation frontend I've used.
Edit: forgot to explicitly mention that I'm running the F-droid version.
There is no new review since https://github.com/Swordfish90/Lemuroid/pull/632#issuecomment-1510298379 / 2023-04-16 (1.5 year ago), normally i would say that project is unmaintained but i still see commit on the main branch (most just bump of versions) ...
hope @Swordfish90 will take here another look soon.
PS: @newhinton thanks to your work till, now. (and also to @Swordfish90 for this project).
Would you like to add #900 to your solve list?
Any maybe publish an dirty debug .apk
for testing?
@genofire Sure, i can add a dirty apk. Though its really dirty, its my local build. It basically contains all of my changes, even those from some of the other PR's. I've also added another way to access the internal storage (#936). It should be very close to the latest master branch in this repository.
https://github.com/newhinton/Lemuroid/releases/tag/demo-release-0001
@genofire Oh, and it reorders the savegames! so if you already have a savegame folder, you might need to reorder them into core-specific folders. (eg, game boy advance savegames need to be in the "mgba" folder)
@newhinton wow thanks the "cloud-sync" works fantastic.
I just tested this on my phone and it worked great with NextCloud!
The only issue I have now is my Android TV doesn't seem to have a file manager. I can't select my ROMs location let alone the save location. I think that is an unrelated issue but if anyone knows how to resolve this please let me know.
EDIT:
Sounds like the latest Android TV is missing the file select app. It can be installed manually. It's called com.android.documentsui
.
Closes #508 Closes #356 Closes #310 Closes #264 Closes #158 Closes #7 Closes #552 Closes #572
This superseeds #552. Here i use the already implemented
SaveSyncManagerImpl
to bring this feature to fdroid.Todo:
This is a replacement implementation for google drive, and therefore currently only available on the fdroid build. If we want this to be available to everyone, we need to think about migrating google drive users.
Long Term Todo: