-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
Implement Sync via SyncManager #632
base: master
Are you sure you want to change the base?
Implement Sync via SyncManager #632
Conversation
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?
Sorry, but what comments? I cant see any... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I forgot to push "Request changes" to the review.
...pp-ext-free/src/main/java/com/swordfish/lemuroid/ext/feature/savesync/ActivateSAFActivity.kt
Show resolved
Hide resolved
@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:
|
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? |
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 . |
Note: Lemuroid is struggling to load external changes to the save-files. |
@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 #632 (comment) / 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). |
@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 |
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: