[asterisk-dev] [Code Review]: Enhancements to presence in Asterisk

Mark Michelson reviewboard at asterisk.org
Thu Apr 19 14:58:15 CDT 2012



> On April 19, 2012, 12:35 p.m., Terry Wilson wrote:
> > I don't see anything obviously wrong. Was the ast_presence_state_changed_literal function a part of this review, or from something previous? I didn't see it, so I thought I'd ask just in case. In looking it up, I noticed that it is missing doxygen docs for the parameters it doesn't share with ast_presence_state_changed.

Yeah, that is my mistake. The third revision uploaded here only had changes to func_presencestate.c. The previous diffs had changes to other files as well. You'd have to look at diff revision 2 for those changes.

I'll get the doxygen fixed up when I commit.


> On April 19, 2012, 12:35 p.m., Terry Wilson wrote:
> > /team/mmichelson/trunk-digiumphones/funcs/func_presencestate.c, lines 351-354
> > <https://reviewboard.asterisk.org/r/1850/diff/3/?file=27373#file27373line351>
> >
> >     My personal preference would be inlining these like the const char * to be consistent with the const char * above. Same with the other places these exist. Completely nit-picky and preference-oriented. Feel free to have different preferences. :-)

Yep, this is a case where my preference is completely the opposite of yours. Though I will admit mixing both individual declarations with multiple declarations on a single line is inconsistent. However, if I'm going to change it at all, I'd change it to place 'dev', 'state', and 'full_dev' each on its own line.


- Mark


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


On April 17, 2012, 11:24 a.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1850/
> -----------------------------------------------------------
> 
> (Updated April 17, 2012, 11:24 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This changeset contains revisions to the presence system in Asterisk to make it more palatable for general release in Asterisk 11. While the system as it exists in the 1.8-digiumphones and 10-digiumphones works well, it was missing some functionality and was not implemented in the most optimal way in some cases. Below are a list of the changes made:
> 
> * func_presencestate has two CLI commands added. "presencestate list" lists the current states of all CustomPresence entities. "presencestate change" allows for CustomPresence entities' states to be updated. These two commands are modeled after similar ones in func_devstate.
> * func_presencestate now populates the event cache on startup to mirror what func_devstate does.
> * The functions that change presence state have been updated to behave more like the ast_devstate_changed family of functions. This includes two main changes.
>     - There are now two functions to use, either ast_presence_state_changed() or ast_presence_state_changed_literal(). The difference between these is that one takes a printf-style format string and the other takes a literal name.
>     - The state change functions now take the state (and subtype and message) as part of the function. This saves the trouble of having to look up the state from the presence state provider on every state change. In the case of CustomPresence, this means far fewer ASTDB reads.
> * A new manager action, "PresenceState", has been added. This will return the current presence state, subtype, and message for a given presence provider.
> 
> 
> Diffs
> -----
> 
>   /team/mmichelson/trunk-digiumphones/funcs/func_presencestate.c 361324 
> 
> Diff: https://reviewboard.asterisk.org/r/1850/diff
> 
> 
> Testing
> -------
> 
> I have tested the CLI commands and Manager actions. They exercise the core updates, so I can verify they work as well. I will have an external test written within the coming days and will post it when I have completed it.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120419/ebddd0ab/attachment.htm>


More information about the asterisk-dev mailing list