search

jellyfin-archive / jellyfin-android-original

Android Client for Jellyfin
https://jellyfin.org
GNU General Public License v2.0
272 stars 65 forks source link

Media notification improvements #319

Closed Maxr1998 closed 4 years ago

Maxr1998 commented 4 years ago
Maxr1998 commented 4 years ago

Regarding the necessary changes to the web client for to support seeking - I have already implemented those, and will issue a separate PR to that project very soon. Note that this PR also works without these changes, clicking the seekbar in the notification just won't do anything then.

EDIT: I had some issues adapting my changes to the latest jellyfin-web master. For some reason, even after a successful build, I couldn't get the app to load, so I couldn't test the changes.. I attached the (confirmed working) diffs to apply on the jellyfin-android repo for now.

inputManager.diff playbackmanager.diff

rexbron commented 4 years ago

Would the change to ongoing notifications close #305 and #235?

dkanada commented 4 years ago

The features look good to me, we just have to get the build working and this should be fine. If you'd like to make other improvements we have a huge list of things the Android client needs :laughing: it's a bit of a beast.

Maxr1998 commented 4 years ago

@rexbron #235 should be closed through this, but unfortunately the first issue still stands - the web client sometimes doesn't properly propagate updates to the notification, especially if the app is in background. This is, however, unrelated to this PR. Might be related to jellyfin/web, or the Cordova bindings, I'm not sure.

Maxr1998 commented 4 years ago

@dkanada I'm not entirely sure why the build fails, I don't think it's related to my PR. Obligatory "it built fine on my machine", lol.

dkanada commented 4 years ago

Hmmmm I don't know who ran it again but it's passing now.

Maxr1998 commented 4 years ago

Nice! So, should be good to merge now, right?

Maxr1998 commented 4 years ago

Just rebased onto the latest master, and tested my changes again. Would appreciate if they could be merged in soon :P

hamburglar2160 commented 4 years ago

I just ran this on Android 11. As you said, the notification seek bar thing is still a problem until a JF-web merge is done. Is there a web PR that you can link here? With this merged, it's still a significant improvement over what's there but we'll have the seekbar issue.

What will it take to get a stop button there to force the notification away? As best I can tell, I need to return to the app and pause/stop from there.

Maxr1998 commented 4 years ago

I just ran this on Android 11. As you said, the notification seek bar thing is still a problem until a JF-web merge is done. Is there a web PR that you can link here?

Unfortunately not. I plan to rework my above patches that I attached above once the merge is through, though, and the build issues are fixed. Without a way to test, I don't want to open a PR for that change yet. Still, the seekbar already "works", just the seeking is a no-op that's reverted on the next notification update. So, I think it's not really necessary to have this fixed before merging.

What will it take to get a stop button there to force the notification away? As best I can tell, I need to return to the app and pause/stop from there.

Good question. Shouldn't normally be too hard, but imo since it doesn't solve the underlying issue, it's not worth the time. The "stuck" notification is likely a web/Cordova issue, the update callback simply doesn't get called. For now, I always force-close Jellyfin when the issue arises.

hamburglar2160 commented 4 years ago

Ah. Well if it's helpful, here's the APK built out with your PR. Choose the debug build and you probably need to uninstall your previous APK.

Regarding my previous second point, one could just pause THEN swipe away the notification. The obvious benefit to having the persistent notification is that swiping it away will not end playback. Ideally though there'd be a "stop" or "X" button to just outright end playback. Here's an example. image

The other thing I notice is that you can't collapse the notification. It is stuck at "full, open" position. That, to me, is a blocker.

dkanada commented 4 years ago

@Maxr1998 we do have a wrapper function to call a native preference screen, so if you want to add preferences like when swipe to close is available or download options let me know. I'm not a fan of the close button, I'd rather have a preference for something like that.

Maxr1998 commented 4 years ago

@dkanda sorry, was a bit busy and didn't manage to reply. Only being able to dismiss when the playback is paused is the best option imo, and doesn't require any preference. Still, the underlying issue that the notification doesn't get updated with the latest state still needs fixing. Afaik there were plans anyway to move away from Cordova, which might fix that as well.

Anyway, thanks for the merge!

dkanada commented 4 years ago

The migration from Cordova is still some time away and hasn't been finalized, so we'll be keeping this code for a while I think. We have a few possible avenues for the move, from native clients that wrap a WebView like this one, or a single codebase shared by Android and Android TV devices.