-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Table column width calculations #107
Comments
Note to self to check how Drush handles this... |
Can't work out how Drush and other console table formatters do this. |
Hey, @BWPanda do you have an actual use-case for this enhancement? To be honest, the only time I see tables is when I run "b ws" and that one easily fits the console width. Here's how drush (branch 8.x) seems to do it: https://github.com/drush-ops/drush/blob/03539df0de9974f7b96a06143b54481e081621cf/includes/output.inc#L610 |
No, just been looking through the code to see how it works and to try and refactor it. Tables are used, however, everywhere that isn't just plain outputting of a line of text. E.g. messages, |
Help and messages now use text rather than tables. |
Following a bit of work on #430 and #360 , I'm interested in this. I think the drush_table_column_autowidth()/**
* Determine the best fit for column widths.
*
* @param $rows
* The rows to use for calculations.
* @param $widths
* Manually specified widths of each column (in characters) - these will be
* left as is.
*/
function drush_table_column_autowidth($rows, $widths) {
$auto_widths = $widths;
// First we determine the distribution of row lengths in each column.
// This is an array of descending character length keys (i.e. starting at
// the rightmost character column), with the value indicating the number
// of rows where that character column is present.
$col_dist = array();
foreach ($rows as $rowkey => $row) {
foreach ($row as $col_id => $cell) {
if (empty($widths[$col_id])) {
$length = strlen($cell);
if ($length == 0) {
$col_dist[$col_id][0] = 0;
}
while ($length > 0) {
if (!isset($col_dist[$col_id][$length])) {
$col_dist[$col_id][$length] = 0;
}
$col_dist[$col_id][$length]++;
$length--;
}
}
}
}
foreach ($col_dist as $col_id => $count) {
// Sort the distribution in decending key order.
krsort($col_dist[$col_id]);
// Initially we set all columns to their "ideal" longest width
// - i.e. the width of their longest column.
$auto_widths[$col_id] = max(array_keys($col_dist[$col_id]));
}
// We determine what width we have available to use, and what width the
// above "ideal" columns take up.
$available_width = drush_get_context('DRUSH_COLUMNS', 80) - (count($auto_widths) * 2);
$auto_width_current = array_sum($auto_widths);
// If we need to reduce a column so that we can fit the space we use this
// loop to figure out which column will cause the "least wrapping",
// (relative to the other columns) and reduce the width of that column.
while ($auto_width_current > $available_width) {
$count = 0;
$width = 0;
foreach ($col_dist as $col_id => $counts) {
// If we are just starting out, select the first column.
if ($count == 0 ||
// OR: if this column would cause less wrapping than the currently
// selected column, then select it.
(current($counts) < $count) ||
// OR: if this column would cause the same amount of wrapping, but is
// longer, then we choose to wrap the longer column (proportionally
// less wrapping, and helps avoid triple line wraps).
(current($counts) == $count && key($counts) > $width)) {
// Select the column number, and record the count and current width
// for later comparisons.
$column = $col_id;
$count = current($counts);
$width = key($counts);
}
}
if ($width <= 1) {
// If we have reached a width of 1 then give up, so wordwrap can still progress.
break;
}
// Reduce the width of the selected column.
$auto_widths[$column]--;
// Reduce our overall table width counter.
$auto_width_current--;
// Remove the corresponding data from the disctribution, so next time
// around we use the data for the row to the left.
unset($col_dist[$column][$width]);
}
return $auto_widths;
} A recent test setting the terminal width to 80 and running with some long project names got this result: bee projects
|
The auto column width seems to work well with a bit of tweaking but other bits of the table writing will also need doing as it currently only wraps the text from the longest column. |
I've been thinking about this again and wondering whether some of the implicit assumptions about table width are still valid.
I've been experimenting with a simpler approach and the only one I had to expand the terminal window for (and it still fitted onto a single 21" screen @ 1920 x1080 ) was I'm currently looking at doing this ticket, #108 and #185 as one refactor of the tabular output. @indigoxela - particularly interested in your views as you've participated in this discussion, but open to any other input. If I don't get any feedback in the next week or so, I'll just go ahead. |
TBH, I scratch my head to get the problem in the first place. 😉 When I use 'bee ws', the table fits nicely into the console window. Regardless of how wide or narrow the console window currently is and regardless how verbose the dblog messages are. I'm using just a Linux terminal - on native Linux. |
The current code wraps the table text and reduces the largest column's width. I'm proposing to ditch the text wrapping and just calculate column widths as the max for each column. This is what it might look like now
This is what is would look like
|
I'm also having difficulty understanding the problem that is looking for a solution. Bee seems to be displaying fine for me. Granted the terminal is not restricted to 80 cols. But whether on Linux or Windows the display looks fine apart from occasional words being chopped. |
I'm currently looking through the code that renders a table of information for display. I haven't gone through it all properly, or gotten my head around it all yet, but it looks to me like the code that prevents a table from being wider than the terminal window has some potential issues that could be improved...
Not sure if these issues exist or are worth trying to fix, but noting my thoughts down here for future just in case.
The current logic seems to go as follows:
Issues are that there's no check/support for multiline content in the header (I guess we just assume headers are shorter than the content below them), and that if you have a table with lots of wide columns, only the widest is adjusted, which may mean that the table's still too wide for the terminal (or that other columns look disproportionate).
Perhaps and example will help:
Let's work with easy numbers and say that the terminal is 12 wide, and the three columns are 2, 10 and 6 wide respectively. The total table width is 18 (too wide to fit in the terminal (a difference of 6)).
Using the existing logic, this is what'll happen: the widest column (10) will be reduced by the difference (6), so it becomes 4. The columns are now 2, 4, 6 and fit, but it seems weird that the 3rd column gets to stay at its original size.
Imagine adding a 4th column of 3. The widest column is still the 10, but it's now reduced by 9 and so has a width of 1, while all other columns stay the same.
I'm thinking something along these lines might be better logic:
Here's an example using the same numbers as the previous example (total width 12):
For the three columns (2, 10, 6): Total diff is 6. Average is 4. First column is shorter than average (by 2), so disperse 2 to the other columns. New average is now 5. Try reducing 2nd column (10) by total diff (6), becomes 4, but that's shorter than new average (5), so make it 5. 3rd column becomes 5 as well. Columns are now 2, 5, 5 (very nice!)
For the four columns (2, 10, 6, 3): Total diff is 9. Average is 3. First column is shorter than average (by 1), so disperse 1 to the other columns (can't have non-integer widths though, so round them down). New average is still 3. Try reducing 2nd column (10) by total diff (9), becomes 1, but that's shorter than new average (3), so make it 3. 3rd column becomes 3 as well. Columns are now 2, 3, 3, 3 (still nice)
New logic could do with some tweaking probably (and likely a better explanation), but at least my thoughts are here for posterity now.
The text was updated successfully, but these errors were encountered: