Skip to content

Conversation

@bugnano
Copy link
Collaborator

@bugnano bugnano commented Mar 10, 2022

@bugnano bugnano requested a review from vickz84259 March 10, 2022 14:43
@bugnano bugnano force-pushed the fetaure/new-js-event-handlers branch from ad9d617 to dd421d1 Compare March 16, 2022 07:35
@bugnano bugnano force-pushed the fetaure/new-js-event-handlers branch from dd421d1 to de483aa Compare March 16, 2022 07:36
Copy link
Collaborator

@vickz84259 vickz84259 left a comment

Choose a reason for hiding this comment

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

How is someone meant to unsubscribe from an event?

if (widget.onCustomMessageAction != null) {
_oldCustomActions = Set<String>.of(widget.onCustomMessageAction!.keys);
for (var action in _oldCustomActions) {
execute('chatBox.onCustomMessageAction("$action", (event) => JSCCustomMessageAction.postMessage(JSON.stringify(["$action", event])));');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why send an array with the action string and event object? The event object by itself already contains a property called action. So the message sent to Dart could just be: JSON.stringify(event)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did that basically to be independent from the event data structure.

But, as I already need to know the event data structure, in order to convert it to Dart, I agree that using event.action works as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

anyway, now I'm using only one handler for all the custom message actions, and using event.action to know the triggered action

@bugnano
Copy link
Collaborator Author

bugnano commented Mar 21, 2022

How is someone meant to unsubscribe from an event?

This actually is a known memory leak, but I don't think it will be that serious.

The way I envisioned it is this:

You put the actions that you want to subscribe to as keys of the mapping.

If you want to subscribe to a new custom action, you add the key to the mapping, and internally, I handle the JS to handle the subscription.

If you don't want to be subscribed anymore, you remove the key from the mapping, and it will work.

Internally the JS will still be subscribed, but no handler will be called from the Dart side.

I know it's not optimal, but I don't think it's a problem.

@eteeselink what do you think?

@eteeselink
Copy link
Contributor

My Flutter-fu fails me, but I'd like to understand this.

With this code, how would someone subscribe to a custom action?

@bugnano bugnano requested a review from vickz84259 March 21, 2022 14:28
@bugnano bugnano force-pushed the fetaure/new-js-event-handlers branch from a56a9b3 to 271efb8 Compare March 21, 2022 14:46
@bugnano
Copy link
Collaborator Author

bugnano commented Mar 21, 2022

My Flutter-fu fails me, but I'd like to understand this.

With this code, how would someone subscribe to a custom action?

With a mapping, as per the (already approved) docs:

onCustomMessageAction: {
  'favourite': (event) {
    print('Favourited message with id: ${event.message.id}');
  },
  'report': (event) {
    print('Reported message with id: ${event.message.id}');
  },
},

@eteeselink
Copy link
Contributor

ok clear. that's delightful btw :) not sure if i already said that.

@bugnano bugnano merged commit ee57d03 into main Mar 23, 2022
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.

4 participants