rivo / tview

Terminal UI library with rich, interactive widgets — written in Golang
MIT License
11.13k stars 575 forks source link

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

Closed darylhjd closed 6 months ago

darylhjd commented 6 months ago

This PR is related to https://github.com/rivo/tview/issues/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 https://github.com/rivo/tview/commit/33a1d271f2b6bae5ef63606df05275c2c91b2f86. No other logic is changed.

rivo commented 6 months ago

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 commented 6 months ago

haha thanks for the real fix!