[asterisk-dev] [Code Review]: Update to chan_unistim functionality

[IgorG]

> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, lines 530-541
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line530>
> >
> >     As an improvement, you may want to comment these structs.  I'm not sure if the char* can be converted to length limited char[] or string fields, but that's a possible improvement as well

This structures initialized only with static values in code. No need in change it ti char[], but I'll take it in mind. Structures commented now


> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, lines 850-852
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line850>
> >
> >     This could use an error message - if a language configuration file doesn't exist, that's probably worth noting

Done


> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, line 855
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line855>
> >
> >     Clarify this error message - if one of these just appeared in the CLI, people would be fairly confused.

Done


> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, line 846
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line846>
> >
> >     Check here to see if the ao2_container_alloc fails

Done


> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, line 3346
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line3346>
> >
> >     Check for success of allocation before using it here

Done


> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, line 3368
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line3368>
> >
> >     If unistim_new fails, c would most likely be NULL here.  You probably should check if c is NULL prior to this point.

Done


> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, line 4215
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line4215>
> >
> >     Is there a reason why this is commented out?  If it should be left in the source, you may want to at least put a comment explaining why; otherwise remove it

This is meaningless code in present realization, deleted


> On March 1, 2012, 9:20 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_unistim.c, lines 897-898
> > <https://reviewboard.asterisk.org/r/1243/diff/6/?file=25088#file25088line897>
> >
> >     This needs to be fixed before the patch goes in.  The ao2_find is going to bump the ref count of lang_entry by one - since you're returning str_trans, you probably want to deref the lang_entry count before you do that, otherwise you could end up in a situation where the ref count on the lang_entry objects keeps increasing, preventing their destruction when the underlying container is disposed of

I am confused here. I need return after unref lang_entry one of its fields. Is it safe to return lang_entry->str_trans itself, or I need to duplicate it? What is the best way to return string found by ao2_find?


- IgorG


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1243/#review5694
-----------------------------------------------------------


On Feb. 28, 2012, 11:02 a.m., IgorG wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1243/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2012, 11:02 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> New unistim.conf options:
> - Added "debug" global option in unistim.conf, that enable debug when module loaded
> - Added "sharpdial" option, enable sending call whet # key pressed
> 
> New features:
> - ability for changing display language (tested on Russian language). Use .po files in encoding, able to display
>   ISO 8859-1, ISO 8859-2, ISO 8859-4, ISO 8859-5, ISO 2022-JP. For selecting language can be used option "language" in
>   unistim.conf or screen menu.
> - Support for multilines
> - Support for holding multiple lines
> - More fixes for display on i2002 phone
> - Configurable keys for sending and received history
> - Menu for selecting codec, contrast (not yet completed) or display language
> - Show clock at first line of idle phone
> - Add ability for pick up call
> - Pick up call by using on-screen soft key
> - Change displaying list of received or send calls (callerid, time and caller name on different screens, listed by lef-right keys)
> 
> Changes:
> - Changed entering on screen phone number, so any number of digits can be entered
> - rtp_port now used start rtp port
> - list of dial tone frequecies now loaded from indications.conf and not hardcoded
> - Key with globe icon how calls menu and not directly codec selection
> 
> Fixes:
> - 0017406 Correct updating LED when switching between speekerphone and handset or hanging up
> - 0017327 Multiple crashes when using phone
> - 0016867 Fixed playing dialtone in some scenarious when conversation already started
> - Fixed dispalying on-screen information when using Redial softkey (DN number and timer displayed).
> - Not sending short ring in case of call forward enabled on phone
> 
> 
> This addresses bug 18229.
>     https://issues.asterisk.org/jira/browse/18229
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/unistim.conf.sample 357253 
>   /trunk/contrib/unistimLang/en.po PRE-CREATION 
>   /trunk/contrib/unistimLang/ru.po PRE-CREATION 
>   /trunk/contrib/unistimLang/ru.po.utf8 PRE-CREATION 
>   /trunk/channels/chan_unistim.c 357253 
> 
> Diff: https://reviewboard.asterisk.org/r/1243/diff
> 
> 
> Testing
> -------
> 
> Testing done by issue tracker users: ibercom, scsiborg, idarwin, TeknoJuce, c0rnoTa. 
> Tested on production system by Jonn Taylor (jonnt) using phone models: Nortel i2004, 1120E and 1140E.
> 
> 
> Thanks,
> 
> IgorG
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120302/0e08ae1a/attachment-0001.htm>