Skip to content

[Console] Add non-auto column width functionality #17761

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

Closed
wants to merge 7 commits into from
Closed

[Console] Add non-auto column width functionality #17761

wants to merge 7 commits into from

Conversation

akeeman
Copy link
Contributor

@akeeman akeeman commented Feb 11, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR #6296

Be able to fix a columns width in a console table (i.e. set a columns width beforehand).
When a column's contents exceed the given column width, it will stretch.

Very useful, for instance, when one wants to display multiple tables that are separated from each other, but still want to align their columns.

@javiereguiluz
Copy link
Member

I'm 👍 for the idea of this pull request, but I have some comments and questions.

  1. I don't like the use of the "Fixed" word. In the Console component, column widths are always "fixed". The only difference is whether they are auto or, if this PR is merged, set explicitly:
-->setColumnFixedWidth(0, 15)
+->setColumnWidth(0, 15);
  1. Maybe we could add support for 'auto' as the value of the column width, for those developers who prefer to define all widths explicitly:
->setColumnWidth(0, 15)
->setColumnWidth(1, 'auto')
->setColumnWidth(2, 25);
->setColumnWidth(3, 'auto')
  1. Maybe we could also add a shortcut method to define all the column widths at once:
$table
    ->setHeaders(array('ISBN', 'Title', 'Author', 'Price'))
    ->setColumnWidths(array('13', 'auto', 'auto', '5'))
    ->setRows(array(
        // ...
    ))

@akeeman
Copy link
Contributor Author

akeeman commented Feb 13, 2016

Thanks for your input.

  1. I used the word 'fixed' to distinguish this feature's config from the already internally used 'columnWidths' (which is used to cache the calculated (effective) column widths). So maybe I can do some renaming there, because the setters (and thus getters) you proposed do sound more straight forward. I'm thinking of renaming that cache to 'effectiveColumnWidth', so I can use 'columnWidth', 'setColumnWidth', 'getColumnWidth', 'setColumnWidths', and 'getColumnWidths'. The property I'm then going to rename is private, so it shouldn't have too much impact on other stuff.

  2. The 'auto' thing sounds like a good plan, as it's more clear than just using 0. Internally it will be converted to 0, as it allows easy calculating. Especially while the set width kind of represents the minimum width and will stretch when the content of a row demands more space. If someone thinks it's needed I can, of course, always look into other modes, like trimming and/or wrapping.

  3. Sounds good.

…xedWidth to columnWidth; implemented setColumnWidths; implemented 'auto' as valid value
@akeeman akeeman changed the title [Console] Add fixed column width functionality [Console] Add non-auto column width functionality Feb 18, 2016
@fabpot
Copy link
Member

fabpot commented Feb 26, 2016

@akeeman Can you submit a documentation pull request (on symfony/symfony-docs) to document this new feature? Can you also add a small note about this in the component CHANGELOG? Thanks.

@fabpot fabpot closed this Feb 26, 2016
@xabbuh
Copy link
Member

xabbuh commented Feb 27, 2016

@fabpot Was it intended to close here?

@akeeman
Copy link
Contributor Author

akeeman commented Feb 28, 2016

I did already provide the documentation, but as this PR is closed cannot provide the change log line along with this PR.

diff --git a/src/Symfony/Component/Console/CHANGELOG.md b/src/Symfony/Component/Console/CHANGELOG.md
index 6d4e0f9..df37640 100644
--- a/src/Symfony/Component/Console/CHANGELOG.md
+++ b/src/Symfony/Component/Console/CHANGELOG.md
@@ -5,6 +5,7 @@ CHANGELOG
 -----

  * added truncate method to FormatterHelper
+ * added setColumnWidth(s) method to Table 

 2.8.3
 -----

@fabpot fabpot reopened this Feb 28, 2016
@akeeman
Copy link
Contributor Author

akeeman commented Feb 28, 2016

The code and change log line are now given in this PR, where some documentation is provided here. Let me know if there is any further feedback. If not, I consider this PR finished.

@akeeman akeeman closed this Feb 28, 2016
@akeeman akeeman reopened this Feb 28, 2016
@akeeman
Copy link
Contributor Author

akeeman commented Feb 28, 2016

(Woops, wrong button)

'Width "%d" is not a valid column width for column %d. Expected width > 0 or \'auto\'.',
$width,
$columnIndex
));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be on one line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing " around the decons %d
Can you also change 'auto' by "auto"?

@akeeman
Copy link
Contributor Author

akeeman commented Mar 1, 2016

Given that the column width is the maximum value over the cell's content width and the int casted column width that is given to the setColumnWidth or setColumnWidths method, the values -1, 0 or "auto" will result in the same behavior.
I think the support for 'auto' as a valid value makes these methods easier to use. One does not have to think about the way a column's width is calculated to have an automatically set width.
As it doen't matter that much, I've added the support for all three values: 0, -1, and "auto". (These are documented too.)

@akeeman akeeman closed this Mar 1, 2016
@fabpot
Copy link
Member

fabpot commented Mar 1, 2016

I still think that "auto" support should be removed. Mixing strings with integer is not a good idea IMO.

@fabpot fabpot reopened this Mar 1, 2016
@HeahDude
Copy link
Contributor

HeahDude commented Mar 1, 2016

And it's ugly when using tools such as CoreSphere/console-bundle or IDE.

@akeeman
Copy link
Contributor Author

akeeman commented Mar 1, 2016

It was a wish of your fellow member. However, it has been removed as it keeps the whole thing simpler anyway.

}

/**
* Set the minimum width of all columns.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sets

@fabpot
Copy link
Member

fabpot commented Mar 1, 2016

Thank you @akeeman.

@fabpot fabpot closed this in 7351168 Mar 1, 2016
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Mar 6, 2016
This PR was merged into the master branch.

Discussion
----------

[Console] Add columns width setter documentation

Documentation for symfony/symfony#17761

Commits
-------

097fb65 remove support for "auto"
50a74fb Add columns width setter documentation
@fabpot fabpot mentioned this pull request May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants