[GH-ISSUE #213] How to insert or reorder rows of a List or Table #168

Closed
opened 2026-03-04 01:02:33 +03:00 by kerem · 9 comments
Owner

Originally created by @ardnew on GitHub (Dec 23, 2018).
Original GitHub issue: https://github.com/rivo/tview/issues/213

I'm trying to implement a navigable sorted list of specific files that exist on a file system. Being a file system, there can be tens or thousands of these files. But the real problem arises from the fact that the files may be added or removed at runtime (external from this application) after the list has been constructed and displayed to the user.

Assuming these file system changes can be detected reliably, which widget or primitive would you suggest to handle reordering the rows of data?

Both the List and Table do not support inserting rows at arbitrary locations, nor do they support moving rows. This forces the application to basically reconstruct the table entirely if an item needs to be inserted near the beginning of the list.

For a List, inserting an item near the beginning would amount to a single AddItem() followed by a SetItemText(i+1, GetItemText(i)) for all items following the new item. Luckily I'm not using the shortcut rune feature, as that appears impossible to reassign.

I believe Table would be similar, but with the added effort of having to re-populate each column for each row being moved.

I haven't actually attempted implementing either of these approaches yet, and I was hoping you might have some insight into which method might perform better (considering how many rows there could be).

Alternatively, maybe you can suggest a better way to accomplish this?

Also, I guess I should request an insertRow() feature be added to either or both of List and Table so that client applications don't have to juggle all of the items manually.

Originally created by @ardnew on GitHub (Dec 23, 2018). Original GitHub issue: https://github.com/rivo/tview/issues/213 I'm trying to implement a navigable _sorted_ list of specific files that exist on a file system. Being a file system, there can be tens or thousands of these files. But the real problem arises from the fact that the files may be added or removed at runtime (external from this application) after the list has been constructed and displayed to the user. Assuming these file system changes can be detected reliably, which widget or primitive would you suggest to handle reordering the rows of data? Both the `List` and `Table` do not support inserting rows at arbitrary locations, nor do they support moving rows. This forces the application to basically reconstruct the table entirely if an item needs to be inserted near the beginning of the list. For a `List`, inserting an item near the beginning would amount to a single `AddItem()` followed by a `SetItemText(i+1, GetItemText(i))` for all items following the new item. Luckily I'm not using the shortcut rune feature, as that appears impossible to reassign. I believe `Table` would be similar, but with the added effort of having to re-populate each column for each row being moved. I haven't actually attempted implementing either of these approaches yet, and I was hoping you might have some insight into which method might perform better (considering how many rows there could be). Alternatively, maybe you can suggest a better way to accomplish this? Also, I guess I should request an `insertRow()` feature be added to either or both of `List` and `Table` so that client applications don't have to juggle all of the items manually.
kerem closed this issue 2026-03-04 01:02:34 +03:00
Author
Owner

@rivo commented on GitHub (Dec 26, 2018):

This requirement makes sense. I think the signatures for List and Table would be different, though. I would suggest the following:

  • List.InsertItem(row int, mainText, secondaryText string, shortcut rune, selected func()) *List where AddItem() is then simply a special case of this new function
  • Table.InsertRow(row int) *Table
  • Table.InsertColumn(column int) *Table

The Table functions would simply shift the contents. You'd still need to call SetCell() on the new row/column to define your content.

If these work for you, I can add them.

<!-- gh-comment-id:450021759 --> @rivo commented on GitHub (Dec 26, 2018): This requirement makes sense. I think the signatures for `List` and `Table` would be different, though. I would suggest the following: - `List.InsertItem(row int, mainText, secondaryText string, shortcut rune, selected func()) *List` where `AddItem()` is then simply a special case of this new function - `Table.InsertRow(row int) *Table` - `Table.InsertColumn(column int) *Table` The `Table` functions would simply shift the contents. You'd still need to call `SetCell()` on the new row/column to define your content. If these work for you, I can add them.
Author
Owner

@ardnew commented on GitHub (Dec 28, 2018):

I'm leaning towards List for my purposes, so if you could implement that, I would be forever grateful.

I did try implementing it myself (see below), but something isn't working quite right. From a goroutine, I'm calling GetItemCount() and GetItemText() to iterate over all list items to determine where to insert the new list item with my InsertItem() below. It seems to work fine up until the point it needs to insert an item outside the range of what's visible on screen, and then it starts acting funny -- i.e., it determines the wrong insertion index. I've implemented this search and insert inside of a QueueUpdate(), but I didn't implement any further resource protection. Haven't had a chance to debug any further yet.

I'm interested to see how your implementation differs from mine.

func (l *List) InsertItem(index int, mainText, secondaryText string, shortcut rune, selected func()) *List {

	// several different ways to interpret index < 0. one convenient way would
	// be to insert starting from the end of the list. the safest option, which
	// is implemented here, is to just consider it as invalid input and return
	// the original list unmodified.
	if index < 0 {
		return l
	}

	// if the index provided is greater than the number of elements in the list,
	// just treat this like an ordinary append, using the exported AddItem()
	if index >= len(l.items) {
		return l.AddItem(mainText, secondaryText, shortcut, selected)
	}

	newItem := &listItem{
		MainText:      mainText,
		SecondaryText: secondaryText,
		Shortcut:      shortcut,
		Selected:      selected,
	}

	l.items = append(l.items, nil) // add a nil item to make room in the buffer for newItem
	copy(l.items[index+1:], l.items[index:]) // shift all items right, starting from insertion index
	l.items[index] = newItem // update the nil item at the insertion index

	if l.currentItem >= len(l.items) {
		l.currentItem = len(l.items) - 1
	}
	if len(l.items) == 1 && l.changed != nil {
		item := l.items[0]
		l.changed(0, item.MainText, item.SecondaryText, item.Shortcut)
	}
	return l
}
<!-- gh-comment-id:450420221 --> @ardnew commented on GitHub (Dec 28, 2018): I'm leaning towards `List` for my purposes, so if you could implement that, I would be forever grateful. I did try implementing it myself (see below), but something isn't working quite right. From a goroutine, I'm calling `GetItemCount()` and `GetItemText()` to iterate over all list items to determine where to insert the new list item with my `InsertItem()` below. It seems to work fine up until the point it needs to insert an item outside the range of what's visible on screen, and then it starts acting funny -- i.e., it determines the wrong insertion index. I've implemented this search and insert inside of a `QueueUpdate()`, but I didn't implement any further resource protection. Haven't had a chance to debug any further yet. I'm interested to see how your implementation differs from mine. ```golang func (l *List) InsertItem(index int, mainText, secondaryText string, shortcut rune, selected func()) *List { // several different ways to interpret index < 0. one convenient way would // be to insert starting from the end of the list. the safest option, which // is implemented here, is to just consider it as invalid input and return // the original list unmodified. if index < 0 { return l } // if the index provided is greater than the number of elements in the list, // just treat this like an ordinary append, using the exported AddItem() if index >= len(l.items) { return l.AddItem(mainText, secondaryText, shortcut, selected) } newItem := &listItem{ MainText: mainText, SecondaryText: secondaryText, Shortcut: shortcut, Selected: selected, } l.items = append(l.items, nil) // add a nil item to make room in the buffer for newItem copy(l.items[index+1:], l.items[index:]) // shift all items right, starting from insertion index l.items[index] = newItem // update the nil item at the insertion index if l.currentItem >= len(l.items) { l.currentItem = len(l.items) - 1 } if len(l.items) == 1 && l.changed != nil { item := l.items[0] l.changed(0, item.MainText, item.SecondaryText, item.Shortcut) } return l } ```
Author
Owner

@rivo commented on GitHub (Jan 12, 2019):

I've added InsertItem() as well as FindItems() based on your comments and your code. My changes are more comprehensive (as is the documentation) and handle some things differently. I also modified SetCurrentItem() and RemoveItem() so negative indices can be provided like with InsertItem().

Thanks for the PR nonetheless. It was good to have a comparison. I hope you don't mind that I'm not merging your version.

If the troubles you mention in your last comment are still there, it would be good to see some example code so I can reproduce them.

I'm reopening this issue because the suggested Table functions are still open items.

<!-- gh-comment-id:453778548 --> @rivo commented on GitHub (Jan 12, 2019): I've added `InsertItem()` as well as `FindItems()` based on your comments and your code. My changes are more comprehensive (as is the documentation) and handle some things differently. I also modified `SetCurrentItem()` and `RemoveItem()` so negative indices can be provided like with `InsertItem()`. Thanks for the PR nonetheless. It was good to have a comparison. I hope you don't mind that I'm not merging your version. If the troubles you mention in your last comment are still there, it would be good to see some example code so I can reproduce them. I'm reopening this issue because the suggested `Table` functions are still open items.
Author
Owner

@ardnew commented on GitHub (Jan 12, 2019):

Of course I don't mind, I appreciate you actually implementing the request.

I do have one other suggestion based on your changes though. After seeing your FindItems() addition and wishing it had a callback, I think it would be very slick to generalize that capability with a higher-order map operation:

func (l *List) MapItems(mapi func(index int, mainText, secondaryText string) bool)

Then FindItems() would be a convenient utility function based on this. The bool return value of the callback offers the user a way to break out, halting the list traversal.

It's the next-best (and safer) interface without actually exposing List.items to the user. This is also better in the sense that the caller doesn't have to wait for the entire traversal to complete before reacting.

<!-- gh-comment-id:453787813 --> @ardnew commented on GitHub (Jan 12, 2019): Of course I don't mind, I appreciate you actually implementing the request. I do have one other suggestion based on your changes though. After seeing your `FindItems()` addition and wishing it had a callback, I think it would be very slick to generalize that capability with a higher-order map operation: ```go func (l *List) MapItems(mapi func(index int, mainText, secondaryText string) bool) ``` Then `FindItems()` would be a convenient utility function based on this. The `bool` return value of the callback offers the user a way to break out, halting the list traversal. It's the next-best (and safer) interface without actually exposing `List.items` to the user. This is also better in the sense that the caller doesn't have to wait for the _entire_ traversal to complete before reacting.
Author
Owner

@ardnew commented on GitHub (Jan 12, 2019):

And no, btw, the list troubles I originally mentioned were bugs on my side unrelated to this change -- they've been resolved.

<!-- gh-comment-id:453788001 --> @ardnew commented on GitHub (Jan 12, 2019): And no, btw, the list troubles I originally mentioned were bugs on my side unrelated to this change -- they've been resolved.
Author
Owner

@rivo commented on GitHub (Jan 23, 2019):

FindItems() does a search, though. Your suggestion for MapItems() takes no search strings. So you simply want to iterate the list items?

Since it also doesn't return a new value (except an indication that you want to stop the iteration), a better name would be Walk(), similar to TreeNode.Walk().

<!-- gh-comment-id:456690569 --> @rivo commented on GitHub (Jan 23, 2019): `FindItems()` does a search, though. Your suggestion for `MapItems()` takes no search strings. So you simply want to iterate the list items? Since it also doesn't return a new value (except an indication that you want to stop the iteration), a better name would be `Walk()`, similar to [`TreeNode.Walk()`](https://godoc.org/github.com/rivo/tview#TreeNode.Walk).
Author
Owner

@ardnew commented on GitHub (Jan 23, 2019):

No, what I mean is that FindItems() performs its own search with a built-in comparison function. I'm suggesting allowing the caller to provide its own comparison function as a callback. Consider something like the following defined in the library code:


type ListItem struct {
	id        int
	primary   string
	secondary string
}

[...]

func (l *List) FindItem(compare func(*ListItem) bool) *ListItem {
	if nil != compare {
		for _, curr := range l.item {
			if compare(curr) {
				return curr
			}
		}
	}
	return nil
}

And the caller could then provide their own search criteria:

found := list.FindItem(func(curr *ListItem) bool {
	return curr.id == 37 || curr.primary == "whoa cool"
})

if found != nil {
	fmt.Printf("found item = %+v\n", found)
}

But to further generalize this, I was suggesting a MapItems() so that the iterator wouldn't necessarily stop after finding the first element that matches the search criteria. This way, it's possible to find multiple items in the list with a single list traversal (e.g. for filtering a list), letting the caller decide when to stop iterating. Something like the following:

func (l *List) MapItems(mapi func(*ListItem) bool) {
	if nil != mapi {
		for _, curr := range l.item {
			if mapi(curr) {
				break
			}
		}
	}
}

And now that first FindItem() implementation I offered would just be a special case of this MapItems() routine:

func (l *List) FindItem(compare func(*ListItem) bool) *ListItem {
	var found *ListItem = nil
	l.MapItems(func(item *ListItem) bool {
		if compare(item) {
			found = item
			return true
		}
		return false
	})
	return found
}
<!-- gh-comment-id:456974754 --> @ardnew commented on GitHub (Jan 23, 2019): No, what I mean is that `FindItems()` performs its own search with a _built-in_ comparison function. I'm suggesting allowing the caller to _provide its own_ comparison function as a callback. Consider something like the following defined in the library code: ```go type ListItem struct { id int primary string secondary string } [...] func (l *List) FindItem(compare func(*ListItem) bool) *ListItem { if nil != compare { for _, curr := range l.item { if compare(curr) { return curr } } } return nil } ``` And the caller could then provide their own search criteria: ```go found := list.FindItem(func(curr *ListItem) bool { return curr.id == 37 || curr.primary == "whoa cool" }) if found != nil { fmt.Printf("found item = %+v\n", found) } ``` But to further generalize this, I was suggesting a `MapItems()` so that the iterator wouldn't necessarily stop after finding the first element that matches the search criteria. This way, it's possible to find multiple items in the list with a single list traversal (e.g. for filtering a list), letting the caller decide when to stop iterating. Something like the following: ```go func (l *List) MapItems(mapi func(*ListItem) bool) { if nil != mapi { for _, curr := range l.item { if mapi(curr) { break } } } } ``` And now that first `FindItem()` implementation I offered would just be a special case of this `MapItems()` routine: ```go func (l *List) FindItem(compare func(*ListItem) bool) *ListItem { var found *ListItem = nil l.MapItems(func(item *ListItem) bool { if compare(item) { found = item return true } return false }) return found } ```
Author
Owner

@rivo commented on GitHub (Jan 24, 2019):

Ok, I understand. But again, map is the wrong name for this. What you're suggesting is walk, iterate, or traverse. A mapping function would emit a new value for each list element. That's not what your example code does.

Semantics aside, everything you describe can already be achieved today with little code:

for index := 0; index < list.GetItemCount(); index++ {
    main, sec := list.GetItemText(index)
    if compare(main, sec) {
        return index
    }
}

I'd like to see real-life requirements for more functions here before I add them on a suspicion. Maybe FindItems() was even a step too far? Anyway, if there's an actual tview functionality you're missing in your application, please open a new issue.

<!-- gh-comment-id:457169191 --> @rivo commented on GitHub (Jan 24, 2019): Ok, I understand. But again, `map` is the wrong name for this. What you're suggesting is `walk`, `iterate`, or `traverse`. A mapping function would emit a new value for each list element. That's not what your example code does. Semantics aside, everything you describe can already be achieved today with little code: ```go for index := 0; index < list.GetItemCount(); index++ { main, sec := list.GetItemText(index) if compare(main, sec) { return index } } ``` I'd like to see real-life requirements for more functions here before I add them on a suspicion. Maybe `FindItems()` was even a step too far? Anyway, if there's an actual `tview` functionality you're missing in your application, please open a new issue.
Author
Owner

@ardnew commented on GitHub (Jan 24, 2019):

Ah yes, you're absolutely right with the name "map", that wasn't a distinction I was considering in my head. Although the routine I offered could be used to transform a list, it admittedly wasn't my intent.

And true, even the FindItems() you implemented could have been implemented with GetItemText(), I don't think you're taking it too far -- there is value and utility in simplifying these common operations.

Furthermore, one real benefit (which you are not taking advantage of) I think could be achieved with the library performing these list traversals is to make them thread-safe, as you've recently addressed by offering QueueUpdate(). These list traversals take time. Time that people would be inclined to offload to a goroutine, which at the moment is unsafe. It may be too much responsibility to put on the caller application to protect that internal item slice of List since they have no direct access to it anyway. Maybe FindItems() would be more valuable if the caller could safely call it from any goroutine. Just a thought

<!-- gh-comment-id:457263723 --> @ardnew commented on GitHub (Jan 24, 2019): Ah yes, you're absolutely right with the name "map", that wasn't a distinction I was considering in my head. Although the routine I offered could be used to transform a list, it admittedly wasn't my intent. And true, even the `FindItems()` you implemented could have been implemented with `GetItemText()`, I don't think you're taking it too far -- there is value and utility in simplifying these common operations. Furthermore, one real benefit (which you are not taking advantage of) I think could be achieved with _the library performing these list traversals_ is to make them thread-safe, as you've recently addressed by offering `QueueUpdate()`. These list traversals take time. Time that people would be inclined to offload to a goroutine, which at the moment is unsafe. It may be too much responsibility to put on the caller application to protect that internal item slice of `List` since they have no direct access to it anyway. Maybe `FindItems()` would be more valuable if the caller could safely call it from any goroutine. Just a thought
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/tview#168
No description provided.