781flyingdutchman / background_downloader

Flutter plugin for file downloads and uploads
Other
158 stars 73 forks source link

Receiving updates question #48

Closed slaci closed 1 year ago

slaci commented 1 year ago

Hello,

I've tried to listen to the updates stream, but as it is a normal stream, I can listen it to only once. I mean, if I called listen() once and after cancelled the subscribtion, then I can't call listen again on it, cause dart throws bad state error (streams can be only listened once). The use case is this: there is a screen where I start to listen to the stream, then navigate away, then back and I receive the error, because it was listened to on the first navigation. If the updates stream was a broadcast stream, that would solve this problem, but then it would lose the buffering feature (by default) which I don't know how important here. (Edit: Just to make sure: I don't want/trying to listen to the stream multiple times at once).

The second option is registerCallbacks() which would be a good alternative (I prefer stream though), but how do I unregister them? Only destroy clears them in base_downloader.dart as far as I see 🤔 I would like to unregister them when I navigate away.

Edit2: Maybe calling destroy would be the proper way when navigating away? As I see now it recreates the stream controller. Will try it later.

781flyingdutchman commented 1 year ago

Hi, thanks for your feedback - this really does make the plugin better, so keep it coming!

I suggest adding a BroadcastStream, so in addition to updates there would be updatesBroadcast and we add the update to both streams if they have listeners. That way you can choose, and understand the risk of missing updates if you're not listening. Changing the type of the updates field would be a breaking change and for most users is unlikely to be an improvement. What do you think?

I'll add unregisterCallback.

slaci commented 1 year ago

Well broadcast streams don't buffer the events, so it would not degrade performance too much if its not used. In contrast the updates stream would buffer (if someone just uses the broadcast).

Make it possible to close the updates stream controller?

An easier solution would be to allow to close the updates stream controller. Currently destroy does this. That does other things too, I haven't checked what exactly, but it is possible its the right way for me currently (as I want to use the downloader on specific screens only). If those other cleanups don't make sense, then a dispose() method may be a good candidate which closes the streamcontroller and creates a new one). That may cleanup callbacks too. My usecase does not need a broadcast tbh.

If you would go with the broadcast:

Honestly I would deprecate the updates getter and replace it by an updates() method (or something), which can take params. The updates getter could call this method in a backward compatible way. The method could have an asBroadcast bool named parameter.

That would help with other things, that maybe also problematic (maybe not). When I was experimenting with resumeFromBackground() I couldn't find a good spot to call it when using the updates stream and bloc. Well, I'm still not sure what that method does exactly, but it dispatches updates, so you need to listen for those updates beforehand (as stated in the docs). Eg:

await resumeFromBackground();
await emit.forEach(updates, (update) {
  // too late (or maybe not because of buffering; but if you use broadcast, then too late for sure)
});
await resumeFromBackground();
// this won't run as `await emit.forEach` will hold the execution until the stream or bloc is closed 

For this the updates() method could take an onListen callback, or just something specific param which calls resumeFromBackground in the onListen method of the stream controller and it wouldn't depend on the buffering.

Just writing down my ideas, maybe they don't make sense, but these are the things I was struggling a little bit.