RE: [Wengophone-devel] branches and wifo-refactoring (Resent for clearity)

[Quang Minh Phan]

(I resend this msg because the previous message was not very easy to read due to some font issue)   Hi Vadim,     >Great goal,   >But i don't really understand the new system of error codes and new API methods.... >I'm affraid with your current approach you are missing this goal       >Well, let's talk about the error codes. In the old phapi, some function >return an int, some function return nothing (void), some other function >returns char*. This makes error handling quite difficult and incoherent.   >This is inexact.... >ALL functions returns int except 3 which are void... (phTerminate, phSetDebugLevel, phRefresh - which are actually internal function). >All these ints are error codes from the same enum except phGetVersion which return phApi version >The error codes are arranged so that 0 means success >I don't really think there is much incoherency and that this particular aspect needs further simplification There were some more functions that return void that were added recently. What we want to have after the process of refactoring is that anyone who wants to add a function to phAPI will know exactly what return type he/she should use. This should jump right into the eyes when he/she reads phAPI.h. We also define some most common use constant like OWPL_RESULT_FAILURE, OWPL_RESULT_NOT_IMPLEMENTED, OWPL_RESULT_OUT_OF_MEMORY, OWPL_RESULT_INVALID_ARGS  to make error checking easier. Before, some functions returned only -1 to indicate an error so we couldn’t know what error was. Some other functions returned multiple values (-1, -2, -3 …) to indicate different errors but these returned values are different among functions, we needed to read the source code of each function to know what a particular value meant.     Now, every function in the new API will return OWPL_RESULT_SUCCESS in caseof success, or another value of type OWPL_RESULT in case of failure, thatmakes error checking more coherent. Hence, the code would appear clearer toreaders.   I completley disagree with the idea that        if (call() == OWPL_RESULT_SUCCESS)           {              bla; bla; bla;            } is more readable than     if (!call())        {           bla;bla;bla;         } in the second  case signal to noice ratio is much better than in the first case   I think that it depends. We have our own coding preferences that may be different from others’. If I code the same function, I’d rather write like this:   OWPL ret; … ret = call(param1, param2, param3, param4, param5 …); if (ret != OWPL_RESULT_SUCCESS) {             if (ret  == OWPL_RESULT_OUT_OF_MEMORY)  {                         … } else {                         Error handling here } } bla; bla; bla;   Still, it is just my personal preference. Anyway, OWPL_RESULT_SUCCESS is defined to be 0, so you can still use your second option above.     For the new API set, some missing functions are be added and some are simplyrenamed to describe better what it does. For example, in the old phapi, there was no easy way to just create a callhandle and set properties to the call before actually dial. The onlyfunctions intended for the purpose of making a call were phPlaceCallx, thatdial and returned a call handle. With these function, we could not ask phapito encrypt the call before dialing. This is now possible with the new APIset: OWPL_LINE hLineOWPL_CALL hCall;.... owplCallCreate(hLine, &hCall); // Create a new call without dialingowplCallSetEncrypt(hCall, 1);  // Set the call to crypted mode...                                   //Set other properties for the callhereowplCallConnect(hLine, "sip:[EMAIL PROTECTED]"); // Dial the call   This kind of problem can be solved by adding ENCRYPTION bit to streams parameter in phLinePlaceCall2 IMO owplXXXX  approach is more complicated to use  (but  one  can argue that  it is matter of taste) It is definetevly more consice to write         phLinePlaceCall2(vl,  uri, 0, 0,  PH_STREAM_ENCRYPTION|PH_STREAM_ALL) than: owplCallCreate(hLine, &hCall); // Create a new call without dialingowplCallSetEncrypt(hCall, 1);  // Set the call to crypted mode...                                   //Set other properties for the callowplCallSetStreams(hCall, PH_STREAM_ALL)owplCallConnect(hLine, "sip:[EMAIL PROTECTED]"); // Dial the call I understand that this owplXXXX stuff was needed in order to implement non A/V media types but as i argue IMO there is NO actual need to implement non A/V media types in phApi     How will you do to add option of type “char*”. Will you add the fourth param to the function that is of type char* like this phLinePlaceCall3(vl, uri, 0,0, PH_STREAM_ENCRYPTION |  PH_STREAM_ALL, some_more_param) ? You may ask me what kind of  “some_more_param” do I want? Why do I need this some_more_param? Well, I do have some examples but I don’t want to cite them here just to avoid a new discussion about the necessity of the params that I would use in my example. What I want to say here is that the new way is much more extendable. The day you want to add 10 mores properties to the call before dialing it, you don’t need to change any existing function, instead, you just need to add the new functions and the new phAPI.h will be 100% backward compatible.      Event there is no RFC about File transfer or VNC with SIP, We prefer to useINVIVE/OK/ACK sequence because it was made to create a session. MESSAGE wasnot invented for this purpose.At least, one advantage that I can mention right away is that if the INVITEwas sent and 3 minutes latter, the invited person hasn't accepted theINVITATION, the session is automatically closed thanks to SIP proxy (a 4xxtimeout response will be sent by the proxy). The fact that the session wasclosed due to a timeout is handled by the SIP stack and an event will besent to the Plugin (File transfer or VNC). The plugin itself doesn't need tolaunch a timer to handle this kind of issue (which is required if MESSAGE isused instead of INVITE).   You mean you added 5th weel to the phApi only to handle timeouts? wengophone already handle various timeouts, it is pretty easy given usage of boost to add another timeout to handle this specific problem. I REALLY don't understand the reasoning, it was SO MUCH SIMPLER to do it using SIP MESSAGE request and HTTP transfer, than to develop SDP plugin framework for phApi The price/performance ratio with respect of developpement effort of  your approach is abysmall... Timeout is one thing we can take advantage of, but not ALL (we can also think of PAUSE and RESUME function). Anyway, SIP is Session Initiation Protocol, not Voice Initiation Procotol nor Video Initiation Protocol. The INVITE/OK/ACK sequence was meant to create any kind of session. I believe that if there are any new standard to establish VNC session or File Transfer Sessions, they will use INVITE instead of MESSAGE. Like in the case of MSRP (http://www.ietf.org/internet-drafts/draft-ietf-simple-message-sessions-15.txt)     Well i'm actually already maintaining a branch which is independent of  Wengophone.... so if you wish you simply need to take it Thanks Vadim, we’ll check if it fits our needs.   Regards,   Minh

_______________________________________________
Wengophone-devel mailing list
Wengophone-devel@lists.openwengo.com
http://dev.openwengo.com/mailman/listinfo/wengophone-devel