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

Add visualization event handlers to Connections class #933

Merged
merged 2 commits into from Jun 7, 2016

Conversation

mrcslws
Copy link
Contributor

@mrcslws mrcslws commented Apr 26, 2016

Fixes #932

@numenta-ci
Copy link

By analyzing the blame information on this pull request, we identified @chetan51, @akhilaananthram and @rcrowder to be potential reviewers

@mrcslws
Copy link
Contributor Author

mrcslws commented Apr 26, 2016

@scottpurdy

I verified that it [seems to be] performance-neutral.

@rhyolight
Copy link
Member

@mrcslws merge in master

@rhyolight
Copy link
Member

@mrcslws ping

return token;
}

void Connections::unsubscribe(UInt32 token)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier if subscribe took a char* name argument that you also pass to unsubscribe? That way the client just has to use the same string for both calls rather than keep track of the token returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. But then:

  • This class would have to store copies of strings.
  • The caller would have to be careful not to use strings that are used elsewhere. When I grab HTM traces, I do it via a set of independent classes that are unaware of each other. It'd be easy for one of these classes to accidentally stomp on another class's event handler. To avoid this, each tracing class would have to generate a string and store it.

This "token" approach mimics javascript's setTimeout and setInterval API.

@scottpurdy
Copy link
Contributor

Done with review. Looks like the SWIG directors are the right thing now that I looked into it more.

@numenta-ci
Copy link

WARNING! This Pull Request has been inactive for 25 days, and will be automatically closed in 5 days if not updated before then. This is an automated message.

@scottpurdy scottpurdy merged commit d2bc9de into numenta:master Jun 7, 2016
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.

None yet

4 participants