Skip to content
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

Multiline cells #159

Merged
merged 9 commits into from
Nov 26, 2023
Merged

Multiline cells #159

merged 9 commits into from
Nov 26, 2023

Conversation

prgres
Copy link
Contributor

@prgres prgres commented Nov 23, 2023

No description provided.

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

@Evertras I did the same in the bubbletea.Table and it renders ok charmbracelet/bubbles#433

image

It looks ok hence it fails on movement (probably because I do not know how to manipulate the yoffset)

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

I tried to re-render data after the loop in renderRowData but I need access to columns style

multiline := false
	for _, column := range columnStrings {
		if lipgloss.Height(column) > 1 {
			multiline = true
			break
		}
	}

	if multiline {
		for _, column := range columnStrings {
			cellStyle := rowStyle.Copy().Inherit(column.style).Inherit(m.baseStyle)

		}
	}

I changed the plan. Now, I will try to store that info in a column row struct

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

Okay, I cannot do it that way since RowData is unaware of the column width.

What I am trying to do is to fill other cells with empty lines to align heigh of multiline

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

Some progress:
image

All we have to do is set the borderStyle.Height in renderRowData to the size of the highest cell

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

OMG I think I got this:
image

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

3 tests fails but I hope it will be straightforward to fix that

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

To test it on your own use option WithMultiline(true) while creating a new table

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

@Evertras if you will have some free time to help, I would be very grateful. The current state fails only 3 tests, but I think that not every case - something is noyes with styles for specific cells. I assume it is connected to the fact that I did not cover all logic in my loop

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

Do not bother with the look of the code - it is just POC that will need a refactor

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

For some reason, this is needed 🤔

image

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

And has to be particular Top - Botttom, Left, Center does not work

@prgres
Copy link
Contributor Author

prgres commented Nov 23, 2023

In a very dirty way, I checked that is not (probably) connected to overflow

func (m Model) renderRowData(row Row, rowStyle lipgloss.Style, last bool) string {
	numColumns := len(m.columns)

	columnStrings := []string{}
	totalRenderedWidth := 0
	tmpTotalRenderedWidth := 0
	rawColumnsStrings := make([]string, 3*len(m.columns))

	stylesInner, stylesLast := m.styleRows()

	maxCellHeight := 0
	columnIndex := 0
	for rawColumnIndex := 1; rawColumnIndex/3 < len(m.columns); rawColumnIndex += 3 {
		if rawColumnIndex != 1 {
			columnIndex = (rawColumnIndex - 1) / 3
			// 0 1
			// 1 4
			// 2 7
		}
		column := m.columns[columnIndex]
		emptyStyle := lipgloss.NewStyle()

		if m.horizontalScrollOffsetCol > 0 && columnIndex == m.horizontalScrollFreezeColumnsCount {
			rendered := m.renderRowColumnData(row, genOverflowColumnLeft(1), rowStyle, emptyStyle)

			rawColumnsStrings[rawColumnIndex-1] = rendered
		}

		if columnIndex >= m.horizontalScrollFreezeColumnsCount &&
			columnIndex < m.horizontalScrollOffsetCol+m.horizontalScrollFreezeColumnsCount {
			continue
		}

		//--
		cellStr := m.renderRowColumnData(row, column, rowStyle, emptyStyle)
		maxCellHeight = max(maxCellHeight, lipgloss.Height(cellStr))
		rawColumnsStrings[rawColumnIndex] = cellStr
		//--
		if m.maxTotalWidth != 0 {
			renderedWidth := lipgloss.Width(cellStr)

			const (
				borderAdjustment = 1
				overflowColWidth = 2
			)

			targetWidth := m.maxTotalWidth - overflowColWidth

			if columnIndex == len(m.columns)-1 {
				// If this is the last header, we don't need to account for the
				// overflow arrow column
				targetWidth = m.maxTotalWidth
			}

			if tmpTotalRenderedWidth+renderedWidth > targetWidth {
				overflowWidth := m.maxTotalWidth - tmpTotalRenderedWidth - borderAdjustment
				overflowColumn := genOverflowColumnRight(overflowWidth)
				overflowStr := m.renderRowColumnData(row, overflowColumn, rowStyle, emptyStyle)

				rawColumnsStrings[rawColumnIndex+1] = overflowStr

				break
			}

			tmpTotalRenderedWidth += renderedWidth
		}
	}
	// for i, column := range rawColumnsStrings {
	// 	fmt.Println(i, column)
	// }

	// fmt.Println("---")

	for columnIndex, column := range m.columns {
		var borderStyle lipgloss.Style
		var rowStyles borderStyleRow

		if !last {
			rowStyles = stylesInner
		} else {
			rowStyles = stylesLast
		}

		if m.horizontalScrollOffsetCol > 0 && columnIndex == m.horizontalScrollFreezeColumnsCount {
			var borderStyle lipgloss.Style

			if columnIndex == 0 {
				borderStyle = rowStyles.left.Copy()
			} else {
				borderStyle = rowStyles.inner.Copy()
			}

			cellStyle := rowStyle.Copy().Inherit(genOverflowColumnLeft(1).style).
				Inherit(m.baseStyle).Height(maxCellHeight)
			cellStyle = cellStyle.Inherit(borderStyle)
			idx := columnIndex*3 + 1 - 1
			if idx <= 0 {
				idx = 1
			}
			rendered := cellStyle.Render(rawColumnsStrings[idx])
			// rendered := cellStyle.Render(rawColumnsStrings[columnIndex*3-1])

			totalRenderedWidth += lipgloss.Width(rendered)

			columnStrings = append(columnStrings, rendered)
		}

		if columnIndex >= m.horizontalScrollFreezeColumnsCount &&
			columnIndex < m.horizontalScrollOffsetCol+m.horizontalScrollFreezeColumnsCount {
			continue
		}

		if len(columnStrings) == 0 {
			borderStyle = rowStyles.left
		} else if columnIndex < numColumns-1 {
			borderStyle = rowStyles.inner
		} else {
			borderStyle = rowStyles.right
		}

		cellStyle := rowStyle.Copy().Inherit(column.style).
			Inherit(m.baseStyle).Height(maxCellHeight)
		cellStyle = cellStyle.Inherit(borderStyle)
		idx := columnIndex*3 + 1
		// if idx <= 0 {
		// 	idx = 1
		// }
		// fmt.Println(idx, rawColumnsStrings[idx])
		cellStr := cellStyle.Render(rawColumnsStrings[idx])
		// cellStr := cellStyle.Render(rawColumnsStrings[columnIndex*3])

		if m.maxTotalWidth != 0 {
			renderedWidth := lipgloss.Width(cellStr)

			const (
				borderAdjustment = 1
				overflowColWidth = 2
			)

			targetWidth := m.maxTotalWidth - overflowColWidth

			if columnIndex == len(m.columns)-1 {
				// If this is the last header, we don't need to account for the
				// overflow arrow column
				targetWidth = m.maxTotalWidth
			}

			if totalRenderedWidth+renderedWidth > targetWidth {
				overflowWidth := m.maxTotalWidth - totalRenderedWidth - borderAdjustment
				cellStyle := rowStyle.Copy().Inherit(genOverflowStyle(rowStyles.right, overflowWidth)).
					Inherit(m.baseStyle).Height(maxCellHeight)
				cellStyle = cellStyle.Inherit(borderStyle)
				rendered := cellStyle.Render(rawColumnsStrings[columnIndex*3+1+1])

				totalRenderedWidth += lipgloss.Width(rendered)

				columnStrings = append(columnStrings, rendered)
				break
			}

			totalRenderedWidth += renderedWidth
		}

		columnStrings = append(columnStrings, cellStr)
	}

	return lipgloss.JoinHorizontal(lipgloss.Bottom, columnStrings...)
}

@prgres
Copy link
Contributor Author

prgres commented Nov 24, 2023

I managed to achieve multiline feature with clean tests
image

The only drawback is that it renders cell content twice if the multiline option is enabled. But I do not have any other idea to handle it without refactoring the current setup.

@Evertras
Copy link
Owner

I'm okay with offering an additional feature at a small expense to performance if that feature is entirely opt-in, which it seems to be here. We should add a small note about potential performance issues in the WithMultiline func comment for docs.

Also seems I need to update the linter settings, haven't looked at that in a while...

@Evertras
Copy link
Owner

Looking at this a bit more, I feel like the perf shouldn't really be an issue. So maybe we don't need the comment. 🤔

@Evertras
Copy link
Owner

We'll also definitely want to add some tests around what multiline tables should look like with expected inputs, which should also bump our coverage up to where it needs to be. Can you add some tests around expected outputs to https://github.com/Evertras/bubble-table/blob/main/table/view_test.go ?

Sorry for missing messages before, was away for a bit.

@Evertras
Copy link
Owner

And as a final note, feel free to make the features example permanently show multiline as well as in your screenshot.

@prgres
Copy link
Contributor Author

prgres commented Nov 25, 2023

@Evertras I have been waiting to add tests and examples for your approval. As you agreed on that solution, I will add them soon :)

@prgres
Copy link
Contributor Author

prgres commented Nov 25, 2023

@Evertras done

No worries about the missed messages. I treated this PR as a dev log to maintain a history and share hints about the implementation for future contributors

Copy link
Owner

@Evertras Evertras left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this and for adding the example, LGTM!

@Evertras Evertras merged commit a875259 into Evertras:main Nov 26, 2023
7 checks passed
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.

None yet

2 participants