Skip to content

fix(grid): order of adding items should not affect drawing - #991

Closed
darylhjd wants to merge 1 commit into
rivo:masterfrom
darylhjd:master
Closed

fix(grid): order of adding items should not affect drawing#991
darylhjd wants to merge 1 commit into
rivo:masterfrom
darylhjd:master

Conversation

@darylhjd

Copy link
Copy Markdown
Contributor

This PR is related to #987

It seems that the order of adding items into the grid affects the result of the drawing.

This behaviour can be seen with the following example:

package main

import (
	"github.com/rivo/tview"
)

func TestPage() {
	var dimensions []int
	for i := 0; i < 15; i++ {
		dimensions = append(dimensions, -1)
	}
	grid := tview.NewGrid().SetRows(dimensions...).SetColumns(dimensions...)
	// Set grid attributes
	grid.SetTitle("Manga Information").
		SetBorder(true)

	// Use a TextView for basic information of the manga.
	info := tview.NewTextView()
	// Set textview attributes
	info.SetWrap(true).SetWordWrap(true).
		SetTitle("About").
		SetBorder(true)

	// Use a table to show the chapters for the manga.
	table := tview.NewTable()
	// Set chapter headers
	numHeader := tview.NewTableCell("Chap").
		SetSelectable(false)
	titleHeader := tview.NewTableCell("Name").
		SetSelectable(false)
	downloadHeader := tview.NewTableCell("Download Status").
		SetSelectable(false)
	scanGroupHeader := tview.NewTableCell("ScanGroup").
		SetSelectable(false)
	readMarkerHeader := tview.NewTableCell("Read Status").
		SetSelectable(false)
	table.SetCell(0, 0, numHeader).
		SetCell(0, 1, titleHeader).
		SetCell(0, 2, downloadHeader).
		SetCell(0, 3, scanGroupHeader).
		SetCell(0, 4, readMarkerHeader).
		SetFixed(1, 0)
	// Set table attributes
	table.SetSelectable(true, false).
		SetSeparator('|').
		SetTitle("Chapters").
		SetBorder(true)

	// Add info and table to the grid. Set the focus to the chapter table.
	grid.AddItem(info, 0, 0, 5, 15, 0, 0, false).
		AddItem(table, 5, 0, 10, 15, 0, 0, true).
		AddItem(info, 0, 0, 15, 5, 0, 80, false).
		AddItem(table, 0, 5, 15, 10, 0, 80, true)

	if err := tview.NewApplication().SetRoot(grid, true).Run(); err != nil {
		panic(err)
	}
}

func main() {
	TestPage()
}

We expect the table to be shown differently based on the minimum widths and heights provided.
image
image

However, this is what we actually get - the table is shown twice in the large width view.
image
image

Strangely, if you change the order of adding of items to the grid, this unexpected behaviour disappears:

	// Change order of adding to this
	grid.AddItem(info, 0, 0, 5, 15, 0, 0, false). // Info now added together
		AddItem(info, 0, 0, 15, 5, 0, 80, false).
		AddItem(table, 5, 0, 10, 15, 0, 0, true). // Table now added together
		AddItem(table, 0, 5, 15, 10, 0, 80, true)
	
	// Previously, this was the order
	grid.AddItem(info, 0, 0, 5, 15, 0, 0, false).
		AddItem(table, 5, 0, 10, 15, 0, 0, true).
		AddItem(info, 0, 0, 15, 5, 0, 80, false).
		AddItem(table, 0, 5, 15, 10, 0, 80, true)

It seems that using a map fixes this. This was also the original data structure used before 33a1d27. No other logic is changed.

@rivo

rivo commented May 24, 2024

Copy link
Copy Markdown
Owner

Posting example code to reproduce the issue was really helpful to find the issue. Thank you for that.

As for this PR, it seems that you didn't attempt to understand what the actual problem was. Switching from a slice to a map just would have made the problem appear randomly (making it even more difficult to debug).

Anyway, the actual fix was just a one-line change.

@darylhjd

Copy link
Copy Markdown
Contributor Author

haha thanks for the real fix!

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