Add pagination on TUI #157
Conversation
ilmanzo
force-pushed
the
enable_pagination
branch
from
66a9108 to
1947b62
Compare
cmd/openqa-mon/tui.go
Outdated
| tui.totalPages++ | ||
| } | ||
| // safety check | ||
| if startIdx >= len(tui.Model.jobs) || endIdx > len(tui.Model.jobs) { |
There was a problem hiding this comment.
Instead of returning, I would set startIdx and endIdx to the maximum values:
| if startIdx >= len(tui.Model.jobs) || endIdx > len(tui.Model.jobs) { | |
| startIdx = min(startIdx, len(tui.Model.jobs)-1) | |
| endIdx = min(endIds, len(tui.Model.jobs)-1) |
Like this the safety check ensure that the program still displays the jobs, otherwise we have a safety check that will prevent the program from crashing, but not displaying anything useful.
There was a problem hiding this comment.
There is still a glitch, when you have a small terminal (let's say 10 lines) and you enlarge it, the page recalc logic doesn't trigger immediately. To have a full responsive application we'd need to intercept the terminal resize event and act accordingly, but it would be easier to just use a proper terminal library as stated above :)
There was a problem hiding this comment.
Can you please add a comment or even better file an issue for this?
There was a problem hiding this comment.
I think the if () statement can be omitted.
There was a problem hiding this comment.
Also, why not apply them directly in line 348 and 349? 🤔
startIdx := min(tui.currentPage * pageHeight, len(tui.Model.jobs)-1)
endIdx := min(startIdx+pageHeight, len(tui.Model.jobs)-1)
cmd/openqa-mon/tui.go
Outdated
| tui.totalPages++ | ||
| } | ||
| // safety check | ||
| if startIdx >= len(tui.Model.jobs) || endIdx > len(tui.Model.jobs) { |
There was a problem hiding this comment.
Can you please add a comment or even better file an issue for this?
ilmanzo
force-pushed
the
enable_pagination
branch
from
aca9532 to
91e3e02
Compare
ilmanzo
force-pushed
the
enable_pagination
branch
from
91e3e02 to
8012c39
Compare
|
There are 2 visual issues that still need to be resolved:
|
ilmanzo
force-pushed
the
enable_pagination
branch
from
8012c39 to
e4cabe6
Compare
|
should be better now: Screencast.from.2024-08-19.15-24-42.mp4 |
I still don't see any differences, but the last state is from an hour ago. Perhaps the last changes haven't been pushed yet? |
ilmanzo
force-pushed
the
enable_pagination
branch
from
e4cabe6 to
fd4fa70
Compare
sorry, you're right.. Forgot to push :) |
cmd/openqa-mon/tui.go
Outdated
| tui.totalPages++ | ||
| } | ||
| // safety check | ||
| if startIdx >= len(tui.Model.jobs) || endIdx > len(tui.Model.jobs) { |
There was a problem hiding this comment.
I think the if () statement can be omitted.
cmd/openqa-mon/tui.go
Outdated
| tui.totalPages++ | ||
| } | ||
| // safety check | ||
| if startIdx >= len(tui.Model.jobs) || endIdx > len(tui.Model.jobs) { |
There was a problem hiding this comment.
Also, why not apply them directly in line 348 and 349? 🤔
startIdx := min(tui.currentPage * pageHeight, len(tui.Model.jobs)-1)
endIdx := min(startIdx+pageHeight, len(tui.Model.jobs)-1)
ilmanzo
force-pushed
the
enable_pagination
branch
from
fd4fa70 to
77f50ef
Compare
Actually your suggestion is a very good idea; I also got rid of the now useless offset :) |
ilmanzo
force-pushed
the
enable_pagination
branch
from
77f50ef to
6ddcae5
Compare
|
how about a minor version bump ? I like |
Yes indeed. But I do version bumps in dedicated merge requests before a new release. This change would motivate one though. |
ilmanzo
force-pushed
the
enable_pagination
branch
from
6ddcae5 to
32e5244
Compare
|
Thank you for adding pagination support in |
<>for previous page / next page,wrapping around on first/last.minfunction), can be changed if 1.20 is a strict requirementcleanMakefile target, and so onFixes #106