Ended isolate + unit tests for it

[nt4f04uNd]

Finishes the isolate API
Also adds MockBaseAudioHandler

Fixes #733 edit: this issue was closed and PR was changed, see #733 (comment)
Related #716

[nt4f04uNd]

I'll update that PR a bit later with the latest updates from the one-isolate

[nt4f04uNd]

@ryanheise WDYT about moving onNotificationClicked into the handler? That way we could broadcast it to isolates, and honestly, I don't really see why it shouldn't be there, anything that is related to the interaction with the platform events should be abstracted into handler

If yes, I can do that in this PR

[ryanheise]

The right thing to do here is not completely obvious.Considering the Android model, the notification click launch is something different. The Handler maps to features of the service, while the notification click launch maps to features of the activity, but you could shoehorn it into the handler and it would work.

I don't have a crystal clear view on what is the right approach here, but sticking it in the handler I guess is good enough if for no other reason than that it's where we're trying to stick everything else (although there are still some exceptions. positionStream will never be in the handler.)

[nt4f04uNd]

Got it. Actually, I think it's better to move it separetly for easier review

[nt4f04uNd]

Currently if the handler was hosted with hostHandler, it won't be unhosted, unless you remove it from the IsolateNameServer. Do you think we should expose that as a method? I'm not sure about that, because if it was hosted from init call, init cannot be called again in the current API

[ryanheise]

Is there a reason for making some private APIs public? E.g. _IsolateRequest, _IsolateAudioHandler and _IsolateAudioHandler._send? That increases the surface area of the public API and potential causes of breaking API changes in the future.

[nt4f04uNd]

That allows the extension of the IsolateAudioHandler and the API it uses, and more fine-grained control of what the handler is used for, e.g. here I sync only one subject https://github.com/ryanheise/audio_service/pull/735/files#diff-d7b34b65f2ad5e5717071239418e3b80ce72eab7c2ca6469a267a92ad3e3b888R938

I actually have no objection on that and I will be fine to make it private

[ryanheise]

In that case I'd prefer to leave them as private and then wait and see what (if any) limitations people run into with the public API.

[nt4f04uNd]

Any thoughts on that? #735 (comment)

[nt4f04uNd]

I made the handler private in the recent commit

[nt4f04uNd]

This actually should be an empty array instead. I have already changed this on another branch that adds platform connection tests over this PR, so I keep it as is and will change it in my next PR

[nt4f04uNd]

There were a few places where you didn't change the nullability of the queue, I fixed them while merging

[ryanheise]

Is there a reason why the isolate unit tests couldn't use AudioService.init instead of AudioService.hostHandler?

[nt4f04uNd]

There are a lot of unrelated things in there, how do you see that?

[ryanheise]

I'm just looking at whether any of the @visibleForTesting APIs can truly be privated. Maybe hostIsolatePortName is a better example - I think having the unit tests be responsible for calling IsolateNameServer.removePortNameMapping feels wrong.

Perhaps there is an argument to break down the design of audio_service into genuine "units" so that they can be better tested in units, and maybe that is what inspired your idea to make the isolate hosting API public? If so, then I understand the benefit of that. I don't necessarily see any use case for a general purpose hosting API, but there is a benefit to splitting AudioService into separate classes. Let me think on it...

[ryanheise]

I have worked out an API I'm happy with. I want to improve the stream sync logic and improve the encapsulation before committing, but the unit tests will have to be updated afterwards.

[nt4f04uNd]

What's this improvement to the sync logic I wonder?

[ryanheise]

Several things.

  Future<void> init() async {
    if (testSyncIsolate ||
        !kDebugMode ||
        !Platform.environment.containsKey('FLUTTER_TEST')) {
      await Future.wait([
        syncSubject(playbackState, 'playbackState'),
        syncSubject(queue, 'queue'),
        syncSubject(queueTitle, 'queueTitle'),
        syncSubject(mediaItem, 'mediaItem'),
        syncSubject(androidPlaybackInfo, 'androidPlaybackInfo'),
        syncSubject(ratingStyle, 'ratingStyle'),
        syncSubject(customEvent, 'customEvent'),
        syncSubject(customState, 'customState'),
      ]);
    }
  }

That condition is overly complex, and I think any version of what it's trying to do is undesirable. Instead, syncSubject should be called in the subject's onListen event.

Aside from that, there is something wrong with the unit tests trying to add events to subjects on the client side - they should only be added on the supplier side. This should be unidirectional communication where events are only added by the supplier and consumed by the client.

The toSkip logic is a bit suspect, if we need something like that, then the approach is too complicated. All it needs to do forward stream events through the send port in one direction and let them be consumed as is.

[nt4f04uNd]

That condition is overly complex, and I think any version of what it's trying to do is undesirable. Instead, syncSubject should be called in the subject's onListen event.

That sounds fine, but if it breaks tests it might be not fine

Aside from that, there is something wrong with the unit tests trying to add events to subjects on the client side - they should only be added on the supplier side. This should be unidirectional communication where events are only added by the supplier and consumed by the client.

The toSkip logic is a bit suspect, if we need something like that, then the approach is too complicated. All it needs to do forward stream events through the send port in one direction and let them be consumed as is.

I'm not sure I understand what you mean. Subjects are both directional i.e. both host and connector isolate can be a modifier of them. If you make them only one directional, you won't be able to modfiy the subject, e.g. mediaItem, from the isolate (or isolate won't receive an update, depending which side is chosen). The toSkip thing allows that to work, and unit tests verify this is working correctly.

[nt4f04uNd]

if we need something like that, then the approach is too complicated

Can you get a little more specific what is complicated there?

[ryanheise]

Why would a FIFO need to skip anything?

[ryanheise]

I just noticed your other message in my email inbox which helps provide me with further context behind your question.

Yes, this is genuinely intended to work in one direction. The AudioHandler interface exposes only the streams to clients while it is only the implementation of the handler that should have access to the subjects. I noticed that AudioService.connectFromIsolate's return type was changed to BaseAudioHandler and I'm not sure exactly when that happened but this will need to change back to AudioHandler since the client should only have access to the public API which allows sending actions and receiving events, not the other way around.

Edit: Oh, I see that message is above, not just in email.

[nt4f04uNd]

I'd argue that's a FIFO, since we are only really interested in notifying about the most recent states, that's a tradeoff because isolate messages are not instant

EDIT: I mean, technically inside one isolate, it is a FIFO, but we need to consider these things below, so some updates should be omitted

Two main reasons the toSkip exists

1. we sync the isolates via listening to the subject, so there are two types of messages that this listener can receive:
   a. the one that has come from the other isolate
   b. the one that the isolate made itself

• In case A: Let's say there are receiver and sender. Receiver received a message and has to pipe it into its stream. But what happens if the listener here (or here) doesn't know that this message has came from the sender which already knows about this message because he sent it? It will try to send it back to the sender. Thus you will have an infinite loop because this message will be infinitely sent back and forth. ANY test with subjects would hang on forever because of this

• In case B: no other isolate knows about this message and we can freely notify all of them

2. please see this test - you may have a situation like this:

• connected isolate had an update and sends a message to the host
• just after that, before host receives an update, there's another update in the host isolate, and it sends this update back to the connected one
• host isolate receives the message it was supposed to receive and updates the stream
• in the connected isolate if we don't check the time these updates are done the message that has came from the host isolate might be older and thus not in sync with the host

[nt4f04uNd]

Yes, this is genuinely intended to work in one direction. The AudioHandler interface exposes only the streams to clients while it is only the implementation of the handler that should have access to the subjects. I noticed that AudioService.connectFromIsolate's return type was changed to BaseAudioHandler and I'm not sure exactly when that happened but this will need to change back to AudioHandler since the client should only have access to the public API which allows sending actions and receiving events, not the other way around.

I would consider this a serious limitation of the API

[ryanheise]

That would break encapsulation. When the question arises as to who should be responsible for updating, say, the playbackState, you don't want it to be global so that anyone anywhere can update it. If we consider this to be a client/server model, the client makes requests to the server, and the server is responsible for fulfilling that request and then broadcasting the updated state to all clients. Clients are not themselves responsible for updating the state, they are responsible for making requests like play and pause.

Let's say in this client/server model the server side is in another process, then for other obvious reasons, the client cannot be allowed to update playbackState directly, however, it can send a message to the server such as play and then the server's implementation of that method will be the only one that can update the state and broadcast those state changes to all clients.

It has always been this way, and I do not consider it to be a serious limitation of the API but rather safe design. Whatever your use case, I'm sure it can fit into this model.

[nt4f04uNd]

It's exactly what happens right now, the only difference is that we notify the subject about the updates that are made in them in their isolates, since there's no need to send this to the host isolate and wait before it responds. It is a server and coordinates all the updates though

Let's say in this client/server model the server side is in another process, then for other obvious reasons, the client cannot be allowed to update playbackState directly, however, it can send a message to the server such as play and then the server's implementation of that method will be the only one that can update the state and broadcast those state changes to all clients.

I'm not sure what is "another process", but if you can't reach out to the main isolate, the connectFromIsolate with throw in the first place

It has always been this way, and I do not consider it to be a serious limitation of the API but rather safe design

So, it is by design that you can't call for example setMediaItem from the connected isolate? It seems wrong to me. AudioHandler is not meant to be subclassed, it's just an interop between the platform callbacks and the handler implementations, which are subclassed from the BaseAudioHandler

[nt4f04uNd]

When the question arises as to who should be responsible for updating, say, the playbackState, you don't want it to be global so that anyone anywhere can update it.

I don't see any problem with that, is there?

[ryanheise]

AudioHandler is not meant to be subclassed, it's just an interop between the platform callbacks and the handler implementations, which are subclassed from the BaseAudioHandler

Let's step back a bit, I will just give my perspective on what I meant AudioService to do since I designed it :-)

AudioHandler was meant as the interface by which any client, including the Flutter UI can interact with the app's audio logic. The AudioHandler was intended as a single source of truth for audio state and managing audio logic in a way that maintains consistency across all clients. I think I've been clear about that in the README, the tutorial, the examples, and the first comment in the pinned issue. That is not to say you couldn't use AudioHandler for your own purposes and suit it to whatever other architecture your app has, and believe me, others have done that, but when it comes to what I meant, this is what I meant. I have completely no objection to people using AudioHandler as a medium for interacting with the platform, though, but even then this does not negate my point about the choice of using encapsulation.

I'm not sure what is "another process", but if you can't reach out to the main isolate, the connectFromIsolate with throw in the first place

Eventually, I would like this client/server model to allow connecting to the media session of another Android process. The client/server model is a general concept that applies in a number of different scenarios: 1) within the same isolate, 2) between isolates, 3) between processes, 4) between devices:

1. within the same isolate. e.g. The Flutter app will often employ some layered architecture with the business logic and state management pushed downward. The upper layers will interact with the AudioHandler interface in order to issue commands, and then listen for state changes. E.g. like BLoC or MVVM (or --insert your architecture here). This happens within the same isolate and while technically it would be possible to make everything global, we still prefer from an architectural perspective to hide things from the upper layers so that the lower layers can guarantee constraints such as state consistency.
2. between isolates: e.g. An app wakes up from the background due to an alarm or geolocation event in another isolate and wants to issue commands to the handler and read its state. Again, this use case fits exactly the same AudioHandler interface. The client is connecting to the handler SO THAT the handler can "handler" it.
3. between processes: e.g. The app wants to connect to the media session of another Android process in order to send it playback commands and listen to the state changes. Again, exactly the same purpose of the AudioHandler interface.
4. between devices: e.g. A peripheral device such as headphones or Android Auto wants to connect to YOUR process and send media commands to it, and then listen to state changes in order to display them through its own UI. Again, Android Auto should not be responsible for effecting the state changes, only for making requests to our handler so that it can "handle" it.

I don't see any problem with that, is there?

Oh dear. "Encapsulation"? I thought we were on the same page.

https://www.topperskills.com/tutorials/oop/object-oriented-programming-encapsulation-concepts.html

Hiding internal state and requiring all interaction to be performed through an object's methods is known as data encapsulation — a fundamental principle of object-oriented programming.

I have no objection to implementing this same concept slightly differently by replacing the methods by an "input" stream (sort of like the BLoC architecture works), but still that is logically equivalent to the same thing, and I would never want to break encapsulation of the state by allowing anyone outside the capsule to directly write to the state.

I don't think we should continue this discussion though. If we're going to debate the value of encapsulation, I'll just have to make a final decision on the matter and say I prefer to encapsulate the state rather than make it globally writable.

[nt4f04uNd]

Can you explain what you mean by encapsulation here? I don't understand, since if I extend from the BaseAudioHandler, everything is encapsulated. I just don't get why you want it to be AudioHandler necessarily.

If you reject this design though I don't feel I want to work on this PR, sorry. You didn't have any real argument against it, you started talking about connection from other processes, which is totally unrelated to this PR. The only argument I heard is that you just don't prefer this style, I totally agree I don't want to discuss stylistic preferences here.

[nt4f04uNd]

I would never want to break encapsulation of the state by allowing anyone outside the capsule to directly write to the state

I read it again and now I understand a bit what you mean by encapsulation, but that's the way you designed BaseAudioHandler, so probably you want to redesign it to avoid any "direct" writes to the subjects (I personally would consider that as pain in the back)

[ryanheise]

I read it again and now I understand a bit what you mean by encapsulation

Encapsulation is part of the standard vocabulary of a software engineer, and that enables productive and compact design discussions. The word itself shouldn't need an explanation among qualified engineers, rather, the word is itself the compact explanation used to justify other higher level design decisions.

So all we need for the scope of this discussion is that, due to encapsulation, this is undesirable:

handler.queue.add(newQueue);

while this is good:

handler.updateQueue(newQueue);

with updateQueue being implemented thusly:

Future<void> updateQueue(List<MediaItem> queue) async {
  this.queue.add(queue);
}

You may not see the benefit, but, as a shorthand, I'll say the benefit of the above is "all of the benefits of encapsulation", which you can look up elsewhere to save time here.

I personally would consider that as a pain in the back

Everyone goes through that phase, I suppose, until they have one or two experiences where rejecting encapsulation caused an even bigger pain in the back.

Anyway, I want to say that regarding this pull request, I am happy with it and I have already merged it into my local branch and started making the improvements mentioned above to encapsulation and stream syncing. Although the encapsulation issue needs to be fixed, that does not negate the value of this contribution - but if you would prefer me not to merge it, let me know and I'll respect that.

[ryanheise]

I've made my API changes to the one-isolate branch. I'm sorry that you have no interest in continuing this pull request if I require encapsulation, although I respect your decision. I will create some simple unit tests later tonight that work to the API changes.

[nt4f04uNd]

I saw you decided to write unit tests from scratch. FYI I wouldn't recommend not testing with which parameters methods were called since while I was making another PR I discovered that a few methods were missing extras in platform interop.