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

request-specific events #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

markkorput
Copy link
Contributor

Hey Oriol, how's it going? Hope all is well in NYC. I'm still enjoying your addons a lot, but this issue with http clients not providing request-specific callbacks puzzles me. All the http addons I find require the same workflow; register one general response handler for all requests. Is it just me or is that a very inconvenient way to process the results? (that's an honest, non-rhetoric question by the way).

Two years ago I struggled with the same thing, I think I then send a pull request with the clumsy "customField" solution, but I just realised how easy it would be to implement this based on Node/JS Promises for async operations.

Anyway, I prefer this approach because;

  • it avoids that one big response callback with a lot of if statements to figure out what kind of response I'm processing (that is assuming I can even figure that out with the data I get).

  • you can assign the listener at the same places (in the code) as the where the request is made, making the code easier to read.

One risk with the current implementation is that the caller might keep the response pointer around which becomes invalid when the response is fully processed. This could be solved by returning a shared_ptr instead, but this would require some more elaborate internal rewrites and I didn't want to go messing with your code like that.

@armadillu
Copy link
Owner

hey man really busy these days, I will have a look after Wednesday? If I do forget, please keep bugging me!

@armadillu
Copy link
Owner

Hey, looking at this I can see where you are going, but I do feel uneasy about sharing pointers to the internal data. If you are only sharing them so that the user can subscribe to the different events, maybe you can return some compound object that sits inside ofxSimpleHttpResponse; like:

struct ofxSimpleHttpResponse{
	struct Events{
		ofEvent<ofxSimpleHttpResponse> responseEvent, successEvent, failureEvent;
		ofxSimpleHttp * who;
	}
}

And then the api only returns the "Events" object ptr... There's too much sensitive stuff in ofxSimpleHttpResponse.

The other thing that comes to mind and may be cleaner, is maybe you can add a new api method that takes a bunch of lambdas, so then there's no need for the awkward ofEvent add listener & stuff; just provide your code and you are done.

@markkorput
Copy link
Contributor Author

Good points! I'll try some things when I have time and create a new pull request if I have something, so you can close this pull request.

@markkorput markkorput closed this Jan 29, 2017
@markkorput markkorput reopened this Jan 29, 2017
@markkorput
Copy link
Contributor Author

Added three new commits to the branch (do those automatically get added to this pull request?).

Anyway, I refactored the events into a separate notifier class and also added support for lambdas. I never used lambdas in C++ before, but I think they work really well here. I did add them to the notifier class, not as argument to the main API methods, which seemed cleaner as this is all optional behavior.

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