Skip to content
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

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

Conversation

newhinton
Copy link

@newhinton newhinton commented Apr 9, 2023

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:

  • cleanup: logging, splitting functions
  • cleanup: splitting functions
  • fault tolerance: proper handling of exceptions and nullpointers when files do not exist
  • fix "ping-pong" syncing, because syncing also updates the timestamp. Touch source file to fix.

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:

  • Migrate Google Drive integration to SAF
  • Sync states
  • Calculate state-storage usage
  • Think about deletions

@newhinton newhinton marked this pull request as draft April 9, 2023 18:50
@newhinton newhinton marked this pull request as ready for review April 10, 2023 13:46
@newhinton newhinton mentioned this pull request Apr 10, 2023
4 tasks
@Swordfish90
Copy link
Owner

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).

@newhinton
Copy link
Author

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...

Copy link
Owner

@Swordfish90 Swordfish90 left a 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.

@Swordfish90
Copy link
Owner

@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.

@newhinton
Copy link
Author

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.

@newhinton
Copy link
Author

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.

@newhinton
Copy link
Author

@Swordfish90 So, states are now supported aswell! However, i can currently only do all states. So the system-chooser has no actual functionality.

@newhinton newhinton requested a review from Swordfish90 May 7, 2023 19:06
@newhinton
Copy link
Author

newhinton commented May 16, 2023

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.

@newhinton
Copy link
Author

newhinton commented May 16, 2023

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.

@SharkWipf
Copy link

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.

@newhinton
Copy link
Author

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.

@Baardi
Copy link

Baardi commented Nov 24, 2023

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.

@Baardi
Copy link

Baardi commented Dec 28, 2023

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

@johntringham
Copy link

johntringham commented Feb 27, 2024

@newhinton @Swordfish90 Is anything blocking these changes getting merged? Would be great to have this feature

@newhinton
Copy link
Author

@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

@wallzero
Copy link

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.

@gjbianco
Copy link

gjbianco commented May 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment