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

Problem: components can't distinguish custom events #677

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yrashk
Copy link

@yrashk yrashk commented Jun 14, 2023

They all look the same, which means they would have to check or poll all potential sources of change.

Solution: make custom events take an integer tag

I've kept the backwards compatibility by using -1 for the default custom event (Event::Custom).


Discussion: #676

@ArthurSonzogni
Copy link
Owner

Hello @yrashk,
Thanks for your support!

I was thinking developers would be able to use:

Event Event::Special(std::string input)

directly. For instance: Event::Special("My event identifier").

Retrospectively, Special is not a very good name, and adding Event::Custom is probably a bit misleading here.

What do you think. Does this solve your need?

Note: To make your patch to work, Event::input_ should be populate so that the operator== works correctly.

@yrashk yrashk force-pushed the event-custom-tag branch 2 times, most recently from 6bd9ddc to 3d84e96 Compare June 15, 2023 00:06
@yrashk
Copy link
Author

yrashk commented Jun 15, 2023

Thank you! I was thinking of int to clarify that the comparisons are fast and cheap. Strings may also get abused to generate runtime identifiers.

They all look the same, which means they would have to check or poll all
potential sources of change.

Solution: make custom events take an integer tag

I've kept the backwards compatibility by using `-1` for the default custom
event (`Event::Custom`).
@yrashk
Copy link
Author

yrashk commented Jun 15, 2023

I've also updated this PR to pass the tests

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.

2 participants