wit-ai / wit-ios

Wit.ai iOS client
Other
222 stars 71 forks source link

Fix of regressions introduced by PR #70 + Converse Endpoint #74

Closed hactar closed 7 years ago

hactar commented 8 years ago

PR #70 did some clean-up, but introduced a few regressions:

This PR fixes these bugs by reverting them while keeping most of PR #70 intact.

Please ignore the -ios 10 changes commit message, that was an internal change, this pr does nothing for iOS 10.

hactar commented 7 years ago

This now includes #78 plus fixes for it so we now support the converse endpoint. Adds tests for this endpoint too. Currently the support is text only, but using SFSpeech, support for the speech recognition part could be added too (needs to pass along the WitSession in a clean way). This pull request therefore now also closes #64 .

klintan commented 7 years ago

I cloned this and it seems the imports for util.h (foundation.h) and util.m (util.h) were missing. But I saw that you didn't change them in this pull-request so not sure when that happened or if it is something I'm missing. (Perhaps this is why witprefix is there, but I don't think that would explain util.h missing?)

Another small issue that I got (but I didn't want to add it if it was only me) WITCvad.m:123:30: Implicit declaration of function 'MAX' is invalid in C99

So had to manually add these macros to WITCvad.m (although SO seems to suggest that they should be default in the compiler)

#define MIN(a,b)    ((a) < (b) ? (a) : (b))
#define MAX(a,b)    ((a) > (b) ? (a) : (b))

Noticed another thing as well: It seems the error delegate is

- (void)error:(NSError*)e customData:(id)customData; {
    [self.delegate witDidGraspIntent:nil messageId:nil customData:customData error:e];
}

which means we have to implement witDidGraspIntent even though it is not used for anything else, not sure what the best practice is in iOS, but perhaps call a general error-delegate or implement the errors to call each respective delegate (stops, message, action)

hactar commented 7 years ago

@klintan I can't reproduce the missing files or the implicit declaration, tested on multiple machines, and added travis support to ensure everything builds correctly: https://travis-ci.org/hactar/wit-ios-sdk

As for the call to WitDidGraspIntent in case of an error - good catch. The legacy API uses that, but for the new converse api we should implement a better callback that includes the WitSession.