Skip to content

table: skip stacking vertically-merged rows that would misplace values - #436

Open
nk-batman21 wants to merge 1 commit into
jedib0t:mainfrom
nk-batman21:fix-automerge-wrap-stacking
Open

nk-batman21 wants to merge 1 commit into
jedib0t:mainfrom
nk-batman21:fix-automerge-wrap-stacking

Conversation

@nk-batman21

Copy link
Copy Markdown

Proposed Changes

  • Since table: stack vertically-merged rows into wrapped cell height; fixes #261 #411 (v6.8.3), rows of a vertical merge are stacked into one block, and that block then goes through the column's WidthMaxEnforcer as a whole. text.WrapSoft and text.WrapHard re-flow the stacked values into each other and text.Trim cuts them off, so once the values add up to more than WidthMax they land on the wrong row, go missing, or push past the column border.
  • A block that starts inside a merge renders its merged column blank as a whole, so a new value further down that column is hidden. This also happens with the default enforcer.
  • Rows with RowConfig{AutoMerge: true} are stacked too, but the block's horizontal merges come from its first row: {"k", "x", "x"} followed by {"k", "y", "z"} shows y across both columns and z is lost.
  • combineVerticalMergeGroup now reports the first two cases, and also a stacked column with VAlignMiddle or VAlignBottom, which moves its values onto other rows' lines. verticalMergeGroupEnd no longer groups rows with RowConfig.AutoMerge. The rows of such a block are rendered one by one, as they were before table: stack vertically-merged rows into wrapped cell height; fixes #261 #411. Other blocks are still stacked for Vertical AutoMerge keeps unnecessary whitespace when a merged row value is wrapped by WidthMax #261, but a WrapSoft or WrapHard column is now stacked only when its stacked values fit within WidthMax together.

For example:

tw := table.NewWriter()
tw.SetColumnConfigs([]table.ColumnConfig{
	{Number: 1, AutoMerge: true},
	{Number: 2, WidthMax: 10, WidthMaxEnforcer: text.WrapSoft},
})
tw.AppendHeader(table.Row{"Team", "Member"})
tw.AppendRow(table.Row{"Eng", "Alice"})
tw.AppendRow(table.Row{"Eng", "Bob"})
tw.AppendRow(table.Row{"Ops", "Carol"})
fmt.Println(tw.Render())

renders on v6.8.3:

+------+--------+
| TEAM | MEMBER |
+------+--------+
| Eng  | Alice Bob |
|      | Carol  |
| Ops  |        |
+------+--------+

and with this change, as on v6.8.2:

+------+--------+
| TEAM | MEMBER |
+------+--------+
| Eng  | Alice  |
|      | Bob    |
| Ops  | Carol  |
+------+--------+

Ran: make test-no-lint steps (table stays at 100% coverage), golangci-lint run ./... (v2.7.2), and the table tests on Go 1.18.
Checked: in 10000 random tables with merged columns, some using RowConfig.AutoMerge, VAlign or RowVAlign, with each value tagged by its row: no value is lost, no border breaks and every value stays on its own row or in its merged cell; 8000 merged rows with WrapSoft or Trim render about as fast as on main.

Rows of a vertical merge are stacked into one block since jedib0t#411, but the
block is still passed through the column's WidthMaxEnforcer as a whole.
Enforcers like text.WrapSoft, text.WrapHard and text.Trim treat the
stacked values as one text, so values end up on the wrong row or are cut
off. A block that starts inside a merge also renders its merged column
blank as a whole, hiding the values that follow. With RowConfig.AutoMerge,
the horizontal merges of the block's first row are applied to every line,
hiding the values of the other rows. A stacked column with VAlignMiddle
or VAlignBottom moves its values onto other rows' lines.

Render the rows of such a block one by one instead, as before jedib0t#411. A
column using WrapSoft, WrapHard or Trim is now stacked only when its
stacked values fit within WidthMax together.
@sonarqubecloud

Copy link
Copy Markdown

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 39644424062

Coverage remained the same at 100.0%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 22 of 22 lines across 2 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 5179
Covered Lines: 5179
Line Coverage: 100.0%
Coverage Strength: 1.21 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants