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

Services integration 3 #123

Open
wants to merge 6 commits into
base: flutter
Choose a base branch
from

Conversation

Izchomatik
Copy link

No description provided.

@Izchomatik Izchomatik linked an issue Oct 25, 2024 that may be closed by this pull request
7 tasks
Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +15 to +16
required Future<void> Function() inactiveFunction,
required Future<void> Function() activeFunction,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe name like activatedFunction / deactivatedFunction would be more intuitive (it'd suggest that they are called on event).

Comment on lines +31 to +32
@override
final Event<Value<String>> stateChangeEvent = Event("stateChangeEvent");
Copy link
Member

Choose a reason for hiding this comment

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

It's preferably to use enums instead of string identifiers. If you change a string in one place, but forget to change it in another, you won't get a compiler error and the change will remain unnoticed.

We could introduce BackendEvent enum and use Event<Value<BackendEvent>>. Alternatively, we can create two separate events without values at all: senderStateChangeEvent and receiverStateChangeEvent.

Comment on lines +25 to +29
@override
bool receiverIsAlive = false;

@override
bool senderIsAlive = false;
Copy link
Member

Choose a reason for hiding this comment

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

Since receiverIsAlive/senderIsAlive values are now cached until onAnyEvents is called, it can lead to a small inconsistency: model calls startReceiver() and then reads receiverIsAlive and it can see that it's false just because event was not delivered from kotlin to dart yet.

To avoid this, we can update startReceiver/stopReceiver/startSender/stopSender so that they will update receiverIsAlive/senderIsAlive before returning.

ModelRoot(Logger logger, Backend backend) {
this.receiver = Receiver(logger, backend);
this.receiver.setDefultValues();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe inline setDefultValues logic into ctor? Isn't it an implementation detail?

Copy link
Member

Choose a reason for hiding this comment

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

It seems that we can remove setReceiverIPs() method. UI doesn't need to set write this field, only read it.

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

Successfully merging this pull request may close these issues.

Flutter migration: roc-droid integration
2 participants