<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2143/">https://reviewboard.asterisk.org/r/2143/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 4th, 2012, 11:37 a.m., <b>David Lee</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2143/diff/2/?file=31722#file31722line1818" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/manager.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static char *handle_showmanagers(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1815</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">space_remaining</span> <span class="o">=</span> <span class="mi">85</span> <span class="o">-</span> <span class="n">name_len</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On October 4th, 2012, 11:54 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On October 6th, 2012, 11:48 a.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</blockquote>
<p>On October 7th, 2012, 4:16 p.m., <b>Tilghman Lesher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
<p>On October 8th, 2012, 12:16 p.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks Tilghman, that sounds like the way to go then.</pre>
</blockquote>
<p>On October 9th, 2012, 10 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've started checking on including curses, and some of the variables in term.h have conflicts with stuff from curses unfortunately. Also after talking with Matt, we might not be so keen on requiring curses for compilation. It's something worth looking into later, but for now it would require more sweeping changes across Asterisk and should probably be included as part of a larger effort to get our CLI commands to follow have a method of checking terminal width while printing output. I'm planning to file an issue for that shortly.
It's still necessary to fix the name problem here in the meantime, so I'll follow Matt's advice of using a #define within manager.c.
One thing I'm not particularly fond of right now is that if the synopsis exceeds the allowable space it just cuts off abruptly without any indication that the synopsis isn't complete. I've thought a little about copying the synopsis string into a buffer to do handle this, but without a standard way to indicate that text is trunctated, I don't really know what to do about that. It's probably not worth bothering with at this time.</pre>
</blockquote>
<p>On October 9th, 2012, 10:12 a.m., <b>Tilghman Lesher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I just copied the following 6 lines from main/editline/term.c into main/manager.c:
#ifdef HAVE_CURSES_H
#include <curses.h>
#endif
#ifdef HAVE_NCURSES_H
#include <ncurses.h>
#endif
Compiled fine, no conflicts. It's worth noting that we already do this check in configure.ac and we already use the capability, if available.</pre>
</blockquote>
<p>On October 9th, 2012, 10:30 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Oh, I see now. I made the mistake of putting my inclusion of curses before term.h and it caused the color definitions in terminal to conflict.
Alright, I'll finish this up. I'll still go ahead and file an issue to apply this concept in a more sweeping way to CLI output though.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">>jrose said:
> It's probably not worth bothering with at this time.
Agreed.</pre>
<br />
<p>- David</p>
<br />
<p>On October 9th, 2012, 11:06 a.m., jrose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Oct. 9, 2012, 11:06 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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
[...]</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-20396">ASTERISK-20396</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/CHANGES <span style="color: grey">(374334)</span></li>
<li>/trunk/main/manager.c <span style="color: grey">(374334)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2143/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>