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

Add synchronization with gPodder Nextcloud server app #5243

Merged
merged 111 commits into from Oct 6, 2021

Conversation

thrillfall
Copy link
Contributor

@thrillfall thrillfall commented Jun 27, 2021

This adds another possibility for synchronizing subscriptions and episode changes by using the Nextcloud app GPodder sync

For this to work one needs to have

  • an account on a Nextcloud server
  • where the app gpodder sync* is installed
  • and have the official Nextcloud Files app installed and connected to said account

Authorization with the Nextcloud server works via SSO (via Nextcloud Files app) in one step.

Closes #421

@thrillfall thrillfall changed the title Nextcloud gpodder sync Add synchronization with nextcloud gpodder sync Jun 27, 2021
@thrillfall thrillfall changed the title Add synchronization with nextcloud gpodder sync Add synchronization with nextcloud Jun 27, 2021
Copy link
Member

@ByteHamster ByteHamster left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I just had a quick look at the code and left some thoughts. I have not tried it yet because I currently don't have a nextcloud.

@keunes
Copy link
Member

keunes commented Jun 27, 2021

@ByteHamster I have a Nextcloud instance that you could play with to test stuff (after I install the NC gPodder app and give you an account).

Great initiative @thrillfall. Curious to see how it works & what functionality it has.

In the spirit of synergy, efficiency, and collaboration: are you aware of the development of the 'native' Nextcloud Podcast app? I've tried that before and really looks quite nice (with podcast pages, episode detail pages, category browsing).

@thrillfall
Copy link
Contributor Author

thrillfall commented Jun 28, 2021

@ByteHamster I have a Nextcloud instance that you could play with to test stuff (after I install the NC gPodder app and give you an account).

👍

Great initiative @thrillfall. Curious to see how it works & what functionality it has.

Thanks. It works exactly like the gpodder.net sync. I merely replicated the api i found in AntennaPod gpodder.net sync module.

In the spirit of synergy, efficiency, and collaboration: are you aware of the development of the 'native' Nextcloud Podcast app? I've tried that before and really looks quite nice (with podcast pages, episode detail pages, category browsing).

If its wanted by the maintainers of the podcast app the api i build can "easily" be integrated to that app. For now i just wanted to see if this is even wanted by AntennaPod maintainers

@onny
Copy link

onny commented Jun 29, 2021

@ByteHamster I have a Nextcloud instance that you could play with to test stuff (after I install the NC gPodder app and give you an account).

+1

Great initiative @thrillfall. Curious to see how it works & what functionality it has.

Thanks. It works exactly like the gpodder.net sync. I merely replicated the api i found in AntennaPod gpodder.net sync module.

In the spirit of synergy, efficiency, and collaboration: are you aware of the development of the 'native' Nextcloud Podcast app? I've tried that before and really looks quite nice (with podcast pages, episode detail pages, category browsing).

If its wanted by the maintainers of the podcast app the api i build can "easily" be integrated to that app. For now i just wanted to see if this is even wanted by AntennaPod maintainers

I'm the developer of the Podcast app and I'm interested into integration of the sync api. I'll have a closer look into this in the coming days but this sounds pretty cool :)

@keunes
Copy link
Member

keunes commented Jun 29, 2021

Great to see these developments! :)

In terms of sync APIs with AntennaPod, I just wanted to throw in a link to issue #4986, which describes what basic functionality AntennaPod would need (probably already covered by this PR, or ByteHamster would surely have mentioned it).

As a non-developer I'm wondering, @thrillfall; so you're building a replication of the gpodder.net API, but what does it connect to? Where does the data go to that gets exchanged via the API? Did you replicate also the whole gpodder.net server in the Nextcloud app (including the UI)? Or is date stored in the 'main' Nextcloud table, maybe even without UI in Nextcloud? Sorry if these questions don't make much sense.

@thrillfall
Copy link
Contributor Author

thrillfall commented Jun 30, 2021

As a non-developer I'm wondering, @thrillfall; so you're building a replication of the gpodder.net API, but what does it connect to? Where does the data go to that gets exchanged via the API? Did you replicate also the whole gpodder.net server in the Nextcloud app (including the UI)? Or is date stored in the 'main' Nextcloud table, maybe even without UI in Nextcloud? Sorry if these questions don't make much sense.

These are all very valid questions. You guessed right:
Data is stored in app specific tables (in nextcloud db). There is no UI (besides the API)
There is no connection to gpodder.net

@thrillfall thrillfall force-pushed the nextcloud-gpodder-sync branch 3 times, most recently from 0011604 to 6850790 Compare July 2, 2021 11:58
@thrillfall
Copy link
Contributor Author

@ByteHamster i fixed all issues raised to my best efforts. If you have any further recommendations please let me know

@thrillfall thrillfall force-pushed the nextcloud-gpodder-sync branch 4 times, most recently from 76d20d6 to cc9e09a Compare July 4, 2021 18:25
@keunes
Copy link
Member

keunes commented Jul 5, 2021

As noted before I'm not a developer so maybe this question makes no sense, but I was wondering about this change:

remove 'GPodder' references in class names and ui. Use 'Nextcloud' only

Doesn't this make things potentially clash/more confusing if/when we get integration with the native Nextcloud Podcast app?

@thrillfall
Copy link
Contributor Author

As noted before I'm not a developer so maybe this question makes no sense, but I was wondering about this change:

remove 'GPodder' references in class names and ui. Use 'Nextcloud' only

Doesn't this make things potentially clash/more confusing if/when we get integration with the native Nextcloud Podcast app?

i would not think so. its a nextcloud app either way. thus the naming

@ByteHamster
Copy link
Member

Doesn't this make things potentially clash/more confusing if/when we get integration with the native Nextcloud Podcast app?

To be honest, I was hoping that both apps will get the same API so we only need one service and settings screen.

@thrillfall
Copy link
Contributor Author

thrillfall commented Jul 7, 2021

nextcloud app has been released: https://apps.nextcloud.com/apps/gpoddersync @keunes

@thrillfall
Copy link
Contributor Author

@ByteHamster do you want me to resolve the issues once i deem them fixed or do you want to do that once you check the changes?

@keunes
Copy link
Member

keunes commented Jul 7, 2021

@thrillfall I have installed the Nextcloud gPodder app and gave ByteHamster access (for testing purposes). Will download a debug build myself and give it a shot as well.

(One thing that struck me, though not on AntennaPod's side, is that there's a button in Nextcloud app bar that does nothing. If it could be removed, I reckon that'd be great.)

(Not sure if you were asking about a general issue, or an issue created in the context of the code review. My guess is that it was about the code review, but if not, the following documentation might be useful:
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue )

@thrillfall
Copy link
Contributor Author

@thrillfall I have installed the Nextcloud gPodder app and gave ByteHamster access (for testing purposes). Will download a debug build myself and give it a shot as well.

(One thing that struck me, though not on AntennaPod's side, is that there's a button in Nextcloud app bar that does nothing. If it could be removed, I reckon that'd be great.)

That should be gone in latest release 1.0.1

(Not sure if you were asking about a general issue, or an issue created in the context of the code review. My guess is that it was about the code review, but if not, the following documentation might be useful:
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue )

No. I was asking about the review comments/issues by @ByteHamster which are all still unresolved (though fixed in code)

@ByteHamster
Copy link
Member

No. I was asking about the review comments/issues by @ByteHamster which are all still unresolved (though fixed in code)

Feel free to mark things as resolved as you resolve them :)

Sorry for the long silence. I hope I will find the time to look at it this weekend.

@ByteHamster
Copy link
Member

I just played around with it and have some feedback :) I didn't look at the code, just played with it.

  • When clicking at the setting for the first time, it was a bit unexpected to me to see the popup dialog. I somehow expected to see the settings page and only get to choose a service when pressing the login button. Aren't the actions pretty much the same for all sync services, like login/logout/force-sync? I think it would be a bit nicer to be asked which sync service to use only when pressing the login button.
    • Related but will probably be fixed together: After selecting gpodder in that popup dialog, I can no longer switch to nextcloud. The popup is not shown again.
  • The description of the "sync" settings item still talks about gpodder. It should be generic now.
  • I think the service select dialog should display 1-2 sentences about each service. Maybe simply the message on top of the sync screen - only for each service on one screen, so users know what they are choosing from. An icon would be nice there, too :)
  • After granting access, the settings screen did not change. I still only had the option to connect. Going back and pressing the sync screen again fixed it.
  • When trying to sync, I get a HTTP 500 error. Not sure if this is caused by AntennaPod, the server configuration or the server app (I have never used nextcloud before). Retrying does not help. Logcat:
2021-07-09 22:50:02.966 17550-19848/de.danoeh.antennapod.debug E/SyncService: de.danoeh.antennapod.net.sync.model.SyncServiceException: com.nextcloud.android.sso.exceptions.NextcloudHttpRequestFailedException: HTTP-Anfrage ist fehlgeschlagen mit HTTP-Statuscode: 500
        at de.danoeh.antennapod.net.sync.nextcloud.NextcloudSyncService.getSubscriptionChanges(NextcloudSyncService.java:114)
        at de.danoeh.antennapod.core.sync.SyncService.syncSubscriptions(SyncService.java:372)
        at de.danoeh.antennapod.core.sync.SyncService.getResultForService(SyncService.java:111)
        at de.danoeh.antennapod.core.sync.SyncService.doWork(SyncService.java:101)
        at androidx.work.Worker$1.run(Worker.java:85)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:923)
     Caused by: com.nextcloud.android.sso.exceptions.NextcloudHttpRequestFailedException: HTTP-Anfrage ist fehlgeschlagen mit HTTP-Statuscode: 500
        at com.nextcloud.android.sso.api.AidlNetworkRequest.performNetworkRequest(AidlNetworkRequest.java:202)
        at com.nextcloud.android.sso.api.NextcloudAPI.performNetworkRequest(NextcloudAPI.java:119)
        at de.danoeh.antennapod.net.sync.nextcloud.NextcloudSyncService.getSubscriptionChanges(NextcloudSyncService.java:101)
        at de.danoeh.antennapod.core.sync.SyncService.syncSubscriptions(SyncService.java:372) 
        at de.danoeh.antennapod.core.sync.SyncService.getResultForService(SyncService.java:111) 
        at de.danoeh.antennapod.core.sync.SyncService.doWork(SyncService.java:101) 
        at androidx.work.Worker$1.run(Worker.java:85) 
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) 
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) 
        at java.lang.Thread.run(Thread.java:923) 
     Caused by: java.lang.IllegalStateException: <!DOCTYPE html>
    <html class="ng-csp" data-placeholder-focus="false" lang="en" data-locale="en" >
[...]
    					<div class="error error-wide">
    	<h2>Internal Server Error</h2>
    	<p>The server was unable to complete your request.</p>
    	<p>If this happens again, please send the technical details below to the server administrator.</p>
    	<p>More details can be found in the server log.</p>
    
    	<h3>Technical details</h3>
    	<ul>
    		<li>Remote Address: xxx.xxx.145.53</li>
    		<li>Request ID: skxxxxx</li>
    			</ul>
[...]
    	</body>
    </html>
    
        at com.nextcloud.android.sso.InputStreamBinder.processRequest(InputStreamBinder.java:366)
        at com.nextcloud.android.sso.InputStreamBinder.performNextcloudRequestAndBodyStream(InputStreamBinder.java:169)
        at com.nextcloud.android.sso.InputStreamBinder.performNextcloudRequest(InputStreamBinder.java:146)
        at com.nextcloud.android.sso.aidl.IInputStreamService$Stub.onTransact(IInputStreamService.java:109)
        at android.os.Binder.execTransactInternal(Binder.java:1154)
        at android.os.Binder.execTransact(Binder.java:1123)

@ByteHamster
Copy link
Member

Yeah, that was the same with the location permission. It was only shown when navigating to the settings screen manually (or when using F-Droid/Google Play to show permissions) and users still complained.

I just successfully authenticated and loaded subscriptions using curl (like in the documentation). So it's pretty easy (but obviously needs some UI).

@thrillfall
Copy link
Contributor Author

Login Flow V2 seems to be rather easy to implement but it will need another round of testing and someone who has the time/motivation to do this :/

But wouldn't that mean that we loose the easy setup where we can choose from our configured nextcloud accounts (via nextcloud files app) and instead have to log in with username and password? That would be a real drag. Literately every nextcloud related app has the github issue to implement SSO.

@ByteHamster
Copy link
Member

It opens the web browser, so if you already are signed in there, you do not need the password again. You do need to type in the host name, though. Given that it is a one-time thing only and the permission would scare everyone forever (even average users who don't host their own nextcloud), I think it is okay to have this little inconvenience when logging in.

@keunes
Copy link
Member

keunes commented Sep 28, 2021

the permission would scare everyone forever (even average users who don't host their own nextcloud)

The permission is only actively asked when accessing this sync setting, right? Wouldn't we be able to display a modal explaining the reason for needing this permission, before Android asks it?

For the Play store we could add a link to permissions documentation (currently in the privacy policy).

Login Flow V2 seems to be rather easy to implement

Would it also get rid of the requirement to have the Nextcloud Android app installed?

@JonOfUs
Copy link
Contributor

JonOfUs commented Sep 28, 2021

It opens the web browser, so if you already are signed in there, you do not need the password again.

I don't know if this only happens on my setup, but in most of the cases I have to log in again, even when I am logged in on my web browser. Maybe because the apps use a built-in browser. So it is possible that with Login Flow V2 one would have to do the whole login process in some/many cases.

The permission is only actively asked when accessing this sync setting, right?

Interestingly, on my phone (Android 10) not even then either. I can sync via Nextcloud without this permission and the app never asks for it. But I don't know if this is the case for every Android version. I don't even know why this permission is ever needed (I have no idea of Android app development).

Would it also get rid of the requirement to have the Nextcloud Android app installed?

Yes, that wouldn't be necessary anymore.

@ByteHamster
Copy link
Member

Maybe because the apps use a built-in browser.

We can use the device browser then.

I can sync via Nextcloud without this permission and the app never asks for it.

Yeah, I think this is about the permission to get the accounts registered with the device. Android grants it automatically but because the accounts could be related to email addresses, it displays them as the contacts permission. AntennaPod does not actually have the "real" contacts permission. Still, users will be confused by this.

I have a working prototype for browser-based login. Will clean it up and post it in the next few days.

@ByteHamster
Copy link
Member

So, I added some UI to the browser-based login. While doing this, I noticed that you do not actually use the gpodder format when uploading episode changes. You use AntennaPod's internal string representation of the Java objects - that's not really nice, as that representation can change at any time. Also, it's not compatible with other clients.

@thrillfall If you are no longer interested in this PR, I can do AntennaPod's part but I cannot do the server part.

This is what you currently send. Notice that everything is wrapped into a single string.

{"data": "[EpisodeAction{podcast='https://feeds.metaebene.me/freakshow/m4a', episode='http://freakshow.fm/podlove/file/7181/s/feed/c/m4a/xy.m4a', guid='podlove-2021-06-25t15:38:53+00:00-3e4c7e5d1913f06', action=PLAY, timestamp=Fri Oct 01 21:04:00 GMT+02:00 2021, started=13663, position=13663, total=13663}

This is the format that should be sent:

[
  {
   "podcast": "http://example.com/feed.rss",
   "episode": "http://example.com/files/s01e20.mp3",
   "device": "gpodder_abcdef123",
   "action": "download",
   "timestamp": "2009-12-12T09:00:00"
  },

I constantly kept tapping the wrong settings item when I wanted to log
in.
@ByteHamster
Copy link
Member

I saw that you, @JonOfUs, worked on EpisodeAction parsing on the server side. Would you be interested in doing the necessary changes there? It should actually be a lot easier to parse when using json for the actions instead of squashing everything together into one string and using regex.

@JonOfUs
Copy link
Contributor

JonOfUs commented Oct 2, 2021

Yes, I thought about this myself. I will look into it in the next days.

@JonOfUs
Copy link
Contributor

JonOfUs commented Oct 5, 2021

@ByteHamster the mentioned changes are on the way. If you have the time, you could start implementing these into AntennaPod.

Minor changes to e.g. the DateTime format could still happen, the PR is yet to be merged. But the EpisodeAction upload should look pretty much like this, now very similar to gpodder.net (DateTime in UTC):

[
  {
   "podcast": "http://example.com/feed.rss",
   "episode": "http://example.com/files/s01e20.mp3",
   "guid": "s01e20-example-org",
   "action": "PLAY",
   "timestamp": "2009-12-12T09:00:00",
   "started": 15,
   "position": 120,
   "total":  500
  },
  {
   "podcast": "http://example.org/podcast.php",
   "episode": "http://ftp.example.org/foo.ogg",
   "guid": "foo-bar-123",
   "action": "DOWNLOAD",
   "timestamp": "2009-12-12T09:05:21",
   "started": -1,
   "position": -1,
   "total":  -1
  }
]

Edit: You can test the new upload format on my Nextcloud, there it already works (only JSON - String doesn't work anymore).

@ByteHamster
Copy link
Member

ByteHamster commented Oct 5, 2021

I changed the payload to be the exactly same that we send to gpodder.net, so it is slightly different to what you posted:

  • The action is lower case
  • When the action is "download", started/position/total are not sent (instead of sending -1)

Unfortunately, I cannot test the most recent commit. @JonOfUs did you reset your nextcloud? When logging in trying to log in, it shows the following message:

The server and with it all passwords got reset on 2021-10-04, so you probably have to set up a new one.

@JonOfUs
Copy link
Contributor

JonOfUs commented Oct 5, 2021

Nice, okay. I will look into these changes, but it sounds like it won't be much work to adapt on server-side.

About the account: yeah, I had to wipe my server and haven't had a backup job for the testing instance's DB running, so the gpoddersync data and your accounts' data is gone. I created an account for you and just sent you a password reset email. Podcast data is gone, but the rest should be as before.

@ByteHamster
Copy link
Member

Hmm, clicking that link and entering a new password only shows the following error message:

Request failed with status code 412

@JonOfUs
Copy link
Contributor

JonOfUs commented Oct 6, 2021

Curious.
I sent you another link, now it should work (works for me now). If it doesn't, let me know, then I'll come up with something else.

@ByteHamster
Copy link
Member

Logging in works now, thanks. Initially, I got the same 412 error again but just before giving up I reloaded the website and that suddenly fixed it. No idea what happened there. Maybe something related to caching.

Looks like syncing also already works with your testing instance :)

@ByteHamster ByteHamster merged commit bc85ebc into AntennaPod:develop Oct 6, 2021
@ByteHamster
Copy link
Member

Finally merged :) Thank you very much @thrillfall @JonOfUs and everyone involved 🎉

Will be released in AntennaPod 2.5.0

@antennapod-bot
Copy link

This pull request has been mentioned on AntennaPod Forum. There might be relevant details there:

https://forum.antennapod.org/t/antennapod-2-5-release-notes/1636/1

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.

Add Nextcloud/OwnCloud support
10 participants