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

Updates should be handled only once #84

Closed
im-trisha opened this issue May 18, 2023 · 5 comments · Fixed by #88
Closed

Updates should be handled only once #84

im-trisha opened this issue May 18, 2023 · 5 comments · Fixed by #88

Comments

@im-trisha
Copy link
Collaborator

im-trisha commented May 18, 2023

At the moment, in a situation like this:

bot.command('help', help);
bot.onMessage.listen(onMessage);

The update will be handled both by onMessage and help, and this probably shouldn't be the case (But as the updates are listened from an "update stream" this behavior maybe is wanted), but could at least be deactivated

IF THE BEHAVIOR ISN'T WANTED, you should refactor the code, by removing the updates stream, and instead, in the "start" function, you should listen to the long polling, and if you receive the updates, you should iter in each handler, and as soon one handler's filters match, you should run it and then break the for loop

PLUS, i suggest, if you do those modifications, you should make the "start" function blocking by using an await for while waiting for updates, as a long polling function should be blocking.

@HeySreelal
Copy link
Owner

Hello again! I'm aware of this issue, but I think it's okay to stream every message into the onMessage stream than pause it because the command or any other method is streaming the same update.

Maybe I need to brainstorm a little more.

Thanks again for sharing the idea.

@im-trisha
Copy link
Collaborator Author

Sure, the blocking bot.start isn't the real issue
The issue i'm talking about is the fact that, even if one handler handles the update, every other handler with a filter that matches that update, will handle that update.

I think that probably what should be done is handling just the first one, by the order in which they are inserted. For example:

bot.command('help', help);
bot.onMessage.listen(onMessage);

if the text is "/help", only the help handler should handle the update.

bot.onMessage.listen(onMessage);
bot.command('help', help);

in this case, if the text is "/help", only the onMessage handler should handle the update (as it is the first one that matches)

I don't know if this is what you want, but i think that this is probably the behavior that should happen. Let me know please.

@HeySreelal
Copy link
Owner

Yeah yeah, I understand. I'm aware of this issue for a while now. But, in my opinion, the different stream getters are only to be used when users intentionally want to listen to all the updates.

If they want to add more specificity to the receiving updates, Televerse provides a ton of methods, such as on, command, hears, text, filter, and so on.

On another note, users can simply use the Televerse.filter method to overcome this behavior.

That being said, I'd say the getters such as onMessage, onEditedMessage, onChannelPost etc should still listen to all the possible events happening of their types. I'm aware that this may cause handling the same update multiple times on different streams. But, I guess, this is kind of intended behavior.

I will surely look for ways to eliminate this in future releases. Thank you so much for reporting this.

Also, I look forward to talking with you in our group chat over Telegram if you're interested. You can join the group here: https://t.me/TeleverseDart.

@HeySreelal
Copy link
Owner

Hey, just wanted to update this behavior is implemented to Televerse v1.9.0.

Televerse now only relies on the onUpdate method on the Fetcher method to listen to updates. Televerse parallelly keeps track of all the handlers set up in an internal list.

As part of this change, I had to completely remove the Event class from the library. Since we're only relying on Fetcher, there's no need to create an extra complexity with the Event class. All the getters in the Event class have now been moved to Televerse class as methods.

Thanks again for suggesting this :)

@HeySreelal
Copy link
Owner

Well, I'm not sure of the bugs lying inside the current structure. If you're able to catch some, please do shoot it :)

Thanks! Happy Televersing <3

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 a pull request may close this issue.

2 participants