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

Make slots called in the order of they are connected #12248

Closed
wants to merge 1 commit into from
Closed

Make slots called in the order of they are connected #12248

wants to merge 1 commit into from

Conversation

Geequlim
Copy link
Contributor

@Geequlim Geequlim commented Oct 20, 2017

Store slots of signals in OrderedHashMap instead the VMap. So the connected methods can be called with the order they connected.
Fix #4898

@neikeq
Copy link
Contributor

neikeq commented Oct 20, 2017

order in which objects are called is not warranted, enforcing an order
would decrease performance in connecting and disconnecting considerably.

@reduz Does this statement still apply?

@Zylann
Copy link
Contributor

Zylann commented Oct 20, 2017

It also looks like the container for connection will be heavier than VMap, which was basically a vector

VMap<Signal::Target, Signal::Slot> slot_map = s->slot_map;

int ssize = slot_map.size();
OrderedHashMap<Signal::Target, Signal::Slot, Signal::TargetHasher> slot_map = s->slot_map;
Copy link
Member

Choose a reason for hiding this comment

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

I wrote the comment above for a reason, and you are ignoring it. This will break.

@reduz
Copy link
Member

reduz commented Oct 20, 2017

Apologies, but I don't want to merge this. It makes signals more complex and breaks the use case described in my comment. You are making a really big change for what I believe is probably a corner case that can probably be handled in a different way.

@akien-mga
Copy link
Member

Closing as per the above.

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.

5 participants