-
Notifications
You must be signed in to change notification settings - Fork 263
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
fix(list): fix list display area overflow. #406
base: master
Are you sure you want to change the base?
Conversation
I found a corner-case. I dont have good solution now. |
06b9653
to
ea7d4f4
Compare
Support for page increases/decreases. |
ea7d4f4
to
4b48a85
Compare
Add comments |
The number of Items per a page depend on pagination height. However, pagination height may changes if it enabled or disabled.
4b48a85
to
0d23bcf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for having a go at this.
@@ -728,6 +728,7 @@ func (m *Model) updateKeybindings() { | |||
func (m *Model) updatePagination() { | |||
index := m.Index() | |||
availHeight := m.height | |||
const paginationDefaultHeight = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the values hardcoded leaks paginationView()
implementation details and is brittle in at least two ways:
- It may break if
paginationView()
is changed in the future. - It may break if the user changes the
PaginationStyle
orArabicPagination
styles (e.g. extra top or bottom margin is added).
Calculating the available height here seems to be quite the chicken and egg conundrum and I do not have better suggestions ATM 🤯 🤷
FWIW, it might be helpful to add some tests while fixing this problem to have more certainty about all possible cases in the future. I've used something like this to test locally, maybe it will be useful:
func prepareListModel(m *Model) {
m.SetShowFilter(false)
m.SetShowStatusBar(false)
m.SetShowTitle(false)
m.SetShowHelp(false)
m.SetShowPagination(true)
// Remove the padding on the left.
m.Styles.TitleBar.UnsetPadding()
// Use arabic pagination for simpler output.
m.Paginator.Type = paginator.Arabic
}
func handleCmd(m Model, cmd tea.Cmd) Model {
for cmd != nil {
m, cmd = m.Update(cmd)
}
return m
}
func expectView(t *testing.T, m Model, expected string) {
t.Helper()
actual := m.View()
if expected != actual {
t.Fatalf("Error: expected %q, got %q", expected, actual)
}
}
func TestUpdatePagination(t *testing.T) {
list := New([]Item{item("a"), item("b"), item("c")}, itemDelegate{}, 10, 4)
prepareListModel(&list)
expectView(t, list, "TODO")
cmd := list.InsertItem(len(list.Items()), item("d"))
list = handleCmd(list, cmd)
expectView(t, list, "TODO")
}
There was a problem hiding this 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 review!
Indeed, I assumed that paginationView()
always returns ""
in the case of pagination disabled for the future.
Yes, I'm struggling with that very point.
Calculating the available height here seems to be quite the chicken and egg conundrum
Now I'm thinking that resetting paginator property for each time runs of updatePagination()
is not bad idea.
This culculates paginationDefaultHeight
each time.
Performance will not be that different.
func (m *Model) updatePagination() {
index := m.Index()
availHeight := m.height
// Reset paginator
m.Paginator.Page = 0
m.Paginator.PerPage = len(m.VisibleItems())
m.Paginator.SetTotalPages(len(m.VisibleItems()))
paginationDefaultHeight := lipgloss.Height(m.paginationView())
if m.showTitle || (m.showFilter && m.filteringEnabled) {
availHeight -= lipgloss.Height(m.titleView())
}
if m.showStatusBar {
availHeight -= lipgloss.Height(m.statusView())
}
if m.showPagination {
availHeight -= paginationDefaultHeight
}
if m.showHelp {
availHeight -= lipgloss.Height(m.helpView())
}
updatePages := func(availHeight int) {
m.Paginator.PerPage = max(1, availHeight/(m.delegate.Height()+m.delegate.Spacing()))
if pages := len(m.VisibleItems()); pages < 1 {
m.Paginator.SetTotalPages(1)
} else {
m.Paginator.SetTotalPages(pages)
}
}
updatePages(availHeight)
// If pagination is needed, recompute items alignment
paginationHeight := lipgloss.Height(m.paginationView())
if m.showPagination && paginationDefaultHeight != paginationHeight {
availHeight += paginationDefaultHeight
availHeight -= paginationHeight
updatePages(availHeight)
}
// Restore index
m.Paginator.Page = index / m.Paginator.PerPage
m.cursor = index % m.Paginator.PerPage
// Make sure the page stays in bounds
if m.Paginator.Page >= m.Paginator.TotalPages-1 {
m.Paginator.Page = max(0, m.Paginator.TotalPages-1)
}
}
The number of Items per a page depend on pagination height. However, pagination height may changes if it enabled or disabled. Re-runing updatePagination() resolves this if needed.
Fix #405