[asterisk-dev] [Code Review]: Prevent manager show commands cutting off the command name in favor of other, less important information.

[jrose]

> On Oct. 4, 2012, 11:37 a.m., David Lee wrote:
> > /trunk/main/manager.c, line 1818
> > <https://reviewboard.asterisk.org/r/2143/diff/2/?file=31722#file31722line1818>
> >
> >     I hate magic numbers. Shouldn't we define a MAX_LINE_LENGTH constant, or something like that? And 85 seems rather arbitrary. A more typical value would be more like 80, 100 or 120.
> 
> jrose wrote:
>     I explained the choice of 85 in the description, and yeah, it's definitely a little arbitrary (because it already was arbitrary). I'm reluctant to add a MAX_LINE_LENGTH definition since I would think it would have been in cli.h a long time ago if there weren't some compelling reason against it.
>     
>     It would be nicer if there was a way to get the width of the terminal, but I don't know for sure if that can be done reliably without relying on libraries like ncurses, which is probably unacceptable.
> 
> Matt Jordan wrote:
>     You can #define the maximum display length in manager.c.  It doesn't have to be in a header file.  The point of not having magic numbers is that no one has to guess what the intent of the number is, not that its globally defined for consumption by every unit that prints to a terminal (although that consistency would be a nice thing to have).
> 
> Tilghman Lesher wrote:
>     Both curses and ncurses define the integer variable COLS, and curses is part of the POSIX specification SUSv2 (1997), which we've previously said is a reasonable requirement to compile Asterisk.
>     
>     Once you've included <curses.h> or <ncurses.h>, you can simply reference the integer COLS (it's an int variable).  The variable is additionally updated when the process receives a SIGWINCH, so you should continue to use the variable in calculations, rather than attempting to cache it from startup.
>     
>     Source:  http://pubs.opengroup.org/onlinepubs/7908799/xcurses/curses.h.html

Thanks Tilghman, that sounds like the way to go then.


- jrose


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


On Oct. 4, 2012, 11:23 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2143/
> -----------------------------------------------------------
> 
> (Updated Oct. 4, 2012, 11:23 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> First, this patch cuts out information about the required authorization level.  There is really no need for it here and this information is freely avaialble with the 'manager show command [command]' information.
> 
> Second, the size field for the name is now picked based on the size of the largest command name. Any remaining space (out of 85, based on the sum of all previous fields, though I could add 2 since I removed some spaces as well and still be length neutral) will be given to the description.
> 
> 
> This addresses bug ASTERISK-20396.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20396
> 
> 
> Diffs
> -----
> 
>   /trunk/main/manager.c 374334 
> 
> Diff: https://reviewboard.asterisk.org/r/2143/diff
> 
> 
> Testing
> -------
> 
> I used the command. It did what I thought it would. Seems pretty simplish.
> 
> Old output:
>   Action           Privilege        Synopsis                                               
>   ------           ---------        --------                                               
>   [...]           
>   StopMixMonitor   <none>           Stop recording a call through MixMonitor, and free the 
>   MixMonitor       <none>           Record a call and mix the audio during the recording.  
>   MixMonitorMute   <none>           Mute / unMute a Mixmonitor recording.                  
>   ConfbridgeSetSi  call,all         Set a conference user as the single video source distri
>   [...]
> 
> New output:
> 
> *CLI> manager show commands
>   Action                       Synopsis                                                  
>   ------                       --------                                                  
>   [...]
>   StopMixMonitor               Stop recording a call through MixMonitor, and free the rec
>   MixMonitor                   Record a call and mix the audio during the recording.  Use
>   MixMonitorMute               Mute / unMute a Mixmonitor recording.                     
>   ConfbridgeSetSingleVideoSrc  Set a conference user as the single video source distribut
>   [...]
> 
> 
> Thanks,
> 
> jrose
> 
>

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