Skip to content

Call OnUnselected and OnSelected when removing a selected tab item #5986

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.

Sign up for GitHub

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

Draft
redawl wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Call OnUnselected and OnSelected when removing a selected tab item #5986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8157aab
Call OnUnselected and OnSelected when removing a tab item
redawl Oct 21, 2025
7501d01
Fix linting error
redawl Oct 21, 2025
29d41fc
Don't call unselect if removed item wasn't selected
redawl Oct 22, 2025
8836a5e
Fix a couple more tabs problems
redawl Oct 23, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions container/tabs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,22 @@ func removeIndex(t baseTabs, index int) {
if index < 0 || index >= len(items) {
return
}
selectedItem := selected(t)
existingItem := items[index]
setItems(t, append(items[:index], items[index+1:]...))
if s := t.selected(); index < s {
t.setSelected(s - 1)
}
if selectedItem == existingItem {
if f := t.onUnselected(); f != nil {
f(existingItem)
}
if f := t.onSelected(); f != nil {
if s := selected(t); s != nil {
f(s)
}
}
}
}

func removeItem(t baseTabs, item *TabItem) {
Expand Down Expand Up @@ -238,8 +250,14 @@ func setItems(t baseTabs, items []*TabItem) {
// Current is first tab item
selectIndex(t, 0)
case selected >= count:
// Current doesn't exist, select last tab
selectIndex(t, count-1)
if selected == count {
// If selected is exactly equal to count, it still exists,
Copy link
Copy Markdown
Member

@andydotxyz andydotxyz Oct 25, 2025

Choose a reason for hiding this comment

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

I don't understand this sorry. if we are replacing 2 items, where the second (index 1) is selected and then replace with 1 we get:

count = 1
selected = 1

Your comment seems to say that item 1 still exists (which it doesn't) but then sets selected to count - 1 (which is 0). The comments here do not seem to be consistent with the code.

Also should it not just be another case in the select rather than an if/else?
Copy link
Copy Markdown
Contributor Author

@redawl redawl Oct 25, 2025

Choose a reason for hiding this comment

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

selected is the index of the selected item, it does not refer to a specific tabitem. State before and after setItems:
Before: [ item1, item2]
After: [item2]

From the tabs perspective, selection is tracked via index, so selected index needs to be moved down by 1 so that item2 still appears to be selected in the ui. However we do not want to call OnSelected since the selected tabItem didn't change (just its index).

Agreed about the if statement can just be two switch cases.
Copy link
Copy Markdown
Contributor Author

@redawl redawl Oct 25, 2025

Choose a reason for hiding this comment

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

Hmm, I think I found another existing bug...

If you have 4 doctabs with the last one selected, and you remove tab 2, then tab 3 is selected instead of tab 4
Copy link
Copy Markdown
Member

@andydotxyz andydotxyz Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I may be reading it wrong, but this appears to be inside setItems - so the assumption that it's a removal of any particular item doesn't hold...
Copy link
Copy Markdown
Contributor Author

@redawl redawl Oct 25, 2025

Choose a reason for hiding this comment

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

Correct. However, if selected >= count we know an item was removed. There is no other way for the new list to be shorter.
// it's just shifted to the left. We don't want to call onSelected in this case,
// so we call setSelected directly
t.setSelected(count - 1)
} else {
selectIndex(t, count-1)
}
}
}

Expand Down
71 changes: 71 additions & 0 deletions container/tabs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,74 @@ func TestTab_ThemeChange(t *testing.T) {
tabs.SelectIndex(0)
assert.Equal(t, initial, w.Canvas().Capture())
}

func TestTab_RemoveCallsHandlers(t *testing.T) {
tabs := NewDocTabs(
NewTabItem("a", widget.NewLabel("a")),
NewTabItem("b", widget.NewLabel("b")),
)

selectedText := ""
unSelectedText := ""
tabs.OnSelected = func(ti *TabItem) {
selectedText = ti.Text
}
tabs.OnUnselected = func(ti *TabItem) {
unSelectedText = ti.Text
}

tabs.RemoveIndex(0)

assert.Equal(t, "b", selectedText)
assert.Equal(t, "a", unSelectedText)
}

func TestTab_RemoveNonSelectedTab(t *testing.T) {
tabs := NewDocTabs(
NewTabItem("a", widget.NewLabel("a")),
NewTabItem("b", widget.NewLabel("b")),
)

selectedText := ""
unSelectedText := ""
tabs.OnSelected = func(ti *TabItem) {
selectedText = ti.Text
}
tabs.OnUnselected = func(ti *TabItem) {
unSelectedText = ti.Text
}

tabs.RemoveIndex(1)

assert.Equal(t, "", selectedText)
assert.Equal(t, "", unSelectedText)
}

func TestTab_RemoveLastTab(t *testing.T) {
tabs := NewDocTabs(
NewTabItem("a", widget.NewLabel("a")),
)
selectedText := ""
tabs.OnUnselected = func(ti *TabItem) {
selectedText = ti.Text
}

tabs.RemoveIndex(0)

assert.Equal(t, "a", selectedText)
}

func TestTab_RemoveTabWhenLastIsSelected(t *testing.T) {
tabs := NewDocTabs(
NewTabItem("a", widget.NewLabel("a")),
NewTabItem("b", widget.NewLabel("b")),
)
tabs.SelectIndex(1)
selectedText := ""
tabs.OnSelected = func(ti *TabItem) {
selectedText = ti.Text
}
tabs.RemoveIndex(0)

assert.Equal(t, "", selectedText)
}
Loading