perf(ui): sort History once per redraw, drop duplicate jobs refreshes
Stage 4 of the GUI layout plan (F11, F12). History: the cell callback copied and sorted the whole event list on every call, and a full-window Refresh issues one call per visible cell — 126 sorts of a 300-element slice per redraw, measured. The sorted snapshot now lives in `rows`, refilled by `resort()` at build time, on a sort toggle, and from `refresh()`. The length callback moves to `len(rows)` with it: cells and the row count have to read the same slice, which was only incidentally true while each cell re-derived the order for itself. The column captions become a package-level array instead of a slice reallocated per header update, and the per-cell `TextStyle`/`Refresh()` pair goes — the template already carries the zero style and `SetText` refreshes. Jobs: `refreshView()` already re-reads the service snapshot and refreshes the list, so the six `list.Refresh()` calls that preceded it, and the duplicate `syncFromService()` in the pause handler, were redundant. The folder filter's early-return path never reaches `refreshView()`, so its `list.Refresh()` moves into that branch rather than being deleted. Both changes carry regression tests, each verified to fail against the behaviour it guards: `TestHistorySortToggleKeepsRowsInSync` for the cache-versus-length hazard, `TestToolbarButtonRedrawsRowAndDetails` for the handlers that now rely on `refreshView` alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+28
-17
@@ -104,35 +104,46 @@ func (h *historyHeader) SetText(text string) {
|
||||
h.label.SetText(text)
|
||||
}
|
||||
|
||||
// historyHeaders are the History table's column captions, in column order. The
|
||||
// Time caption is built per update because it carries the sort direction arrow.
|
||||
var historyHeaders = [...]string{"Time", "Trigger", "Job", "State", "Detail", "Log"}
|
||||
|
||||
func newHistoryView(events *[]event) (*fyne.Container, func()) {
|
||||
descending := false
|
||||
headerText := func(id widget.TableCellID) string {
|
||||
headers := []string{"Time", "Trigger", "Job", "State", "Detail", "Log"}
|
||||
if id.Row < 0 && id.Col == 0 {
|
||||
if descending {
|
||||
return "Time ▼"
|
||||
}
|
||||
return "Time ▲"
|
||||
}
|
||||
if id.Row < 0 && id.Col >= 0 && id.Col < len(headers) {
|
||||
return headers[id.Col]
|
||||
if id.Row < 0 && id.Col >= 0 && id.Col < len(historyHeaders) {
|
||||
return historyHeaders[id.Col]
|
||||
}
|
||||
return ""
|
||||
}
|
||||
sortedEvents := func() []event {
|
||||
result := append([]event(nil), (*events)...)
|
||||
sort.SliceStable(result, func(left int, right int) bool {
|
||||
|
||||
// rows is the sorted snapshot every callback below reads — both the length
|
||||
// callback and the cells, which must agree on the same slice. A full redraw
|
||||
// issues one update call per visible cell, so sorting inside the cell
|
||||
// callback re-sorted the whole event list a hundred times per Refresh.
|
||||
// resort() is therefore the only place the order changes, and it runs once
|
||||
// per redraw: at build time, on a sort toggle, and from refresh().
|
||||
var rows []event
|
||||
resort := func() {
|
||||
rows = append(rows[:0], (*events)...)
|
||||
sort.SliceStable(rows, func(left int, right int) bool {
|
||||
if descending {
|
||||
return result[left].Time > result[right].Time
|
||||
return rows[left].Time > rows[right].Time
|
||||
}
|
||||
return result[left].Time < result[right].Time
|
||||
return rows[left].Time < rows[right].Time
|
||||
})
|
||||
return result
|
||||
}
|
||||
resort()
|
||||
|
||||
table := widget.NewTable(
|
||||
func() (int, int) {
|
||||
return len(*events), 6
|
||||
return len(rows), len(historyHeaders)
|
||||
},
|
||||
func() fyne.CanvasObject {
|
||||
label := widget.NewLabel("")
|
||||
@@ -140,10 +151,7 @@ func newHistoryView(events *[]event) (*fyne.Container, func()) {
|
||||
return label
|
||||
},
|
||||
func(id widget.TableCellID, item fyne.CanvasObject) {
|
||||
label := item.(*widget.Label)
|
||||
label.SetText(historyCellText(id, sortedEvents()))
|
||||
label.TextStyle = fyne.TextStyle{}
|
||||
label.Refresh()
|
||||
item.(*widget.Label).SetText(historyCellText(id, rows))
|
||||
},
|
||||
)
|
||||
table.ShowHeaderRow = true
|
||||
@@ -156,6 +164,7 @@ func newHistoryView(events *[]event) (*fyne.Container, func()) {
|
||||
if id.Row < 0 && id.Col == 0 {
|
||||
h.OnTapped = func() {
|
||||
descending = !descending
|
||||
resort()
|
||||
table.Refresh()
|
||||
}
|
||||
} else {
|
||||
@@ -173,10 +182,12 @@ func newHistoryView(events *[]event) (*fyne.Container, func()) {
|
||||
table.SetColumnWidth(4, 260)
|
||||
table.SetColumnWidth(5, logColumnWidth(*events))
|
||||
|
||||
// refresh recomputes the content-fit Log column width before redrawing, so
|
||||
// newly recorded events with longer file names widen the column instead of
|
||||
// being truncated.
|
||||
// refresh re-reads the event list into the sorted snapshot and recomputes
|
||||
// the content-fit Log column width before redrawing, so newly recorded
|
||||
// events appear in the current sort order and longer file names widen the
|
||||
// column instead of being truncated.
|
||||
refresh := func() {
|
||||
resort()
|
||||
table.SetColumnWidth(5, logColumnWidth(*events))
|
||||
table.Refresh()
|
||||
}
|
||||
|
||||
@@ -6,6 +6,8 @@ import (
|
||||
|
||||
"gitea.mixdep.ru/mix/gosentry/src/domain"
|
||||
|
||||
"fyne.io/fyne/v2"
|
||||
"fyne.io/fyne/v2/test"
|
||||
"fyne.io/fyne/v2/widget"
|
||||
)
|
||||
|
||||
@@ -127,6 +129,95 @@ func TestLogFileName(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestHistorySortToggleKeepsRowsInSync is the regression guard for F11: the
|
||||
// table now reads one cached sorted snapshot instead of re-sorting inside every
|
||||
// cell callback, so the length callback and the cells have to be refilled
|
||||
// together. If either the sort toggle or refresh stops calling resort(), the
|
||||
// row count and the cell contents disagree — which no compiler check catches.
|
||||
func TestHistorySortToggleKeepsRowsInSync(t *testing.T) {
|
||||
testApp := test.NewApp()
|
||||
defer testApp.Quit()
|
||||
|
||||
events := []event{
|
||||
{Time: "2026-06-01 10:00:00", JobName: "A"},
|
||||
{Time: "2026-06-01 11:00:00", JobName: "B"},
|
||||
{Time: "2026-06-01 12:00:00", JobName: "C"},
|
||||
}
|
||||
content, refresh := newHistoryView(&events)
|
||||
table, ok := content.Objects[0].(*widget.Table)
|
||||
if !ok {
|
||||
t.Fatal("history view does not wrap a table")
|
||||
}
|
||||
|
||||
rowCount := func() int {
|
||||
t.Helper()
|
||||
rows, cols := table.Length()
|
||||
if cols != len(historyHeaders) {
|
||||
t.Errorf("column count = %d, want %d", cols, len(historyHeaders))
|
||||
}
|
||||
return rows
|
||||
}
|
||||
// Column 2 is the Job name, the field these fixtures vary.
|
||||
jobAt := func(row int) string {
|
||||
t.Helper()
|
||||
cell := table.CreateCell()
|
||||
table.UpdateCell(widget.TableCellID{Row: row, Col: 2}, cell)
|
||||
return cell.(*widget.Label).Text
|
||||
}
|
||||
// The sort toggle lives on the Time header cell, which is only wired up
|
||||
// when UpdateHeader runs for it.
|
||||
header := table.CreateHeader()
|
||||
table.UpdateHeader(widget.TableCellID{Row: -1, Col: 0}, header)
|
||||
timeHeader, ok := header.(*historyHeader)
|
||||
if !ok {
|
||||
t.Fatal("history table header is not a historyHeader")
|
||||
}
|
||||
assertOrder := func(when string, want ...string) {
|
||||
t.Helper()
|
||||
if got := rowCount(); got != len(want) {
|
||||
t.Fatalf("%s: row count = %d, want %d", when, got, len(want))
|
||||
}
|
||||
for row, name := range want {
|
||||
if got := jobAt(row); got != name {
|
||||
t.Errorf("%s: row %d = %q, want %q", when, row, got, name)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
assertOrder("ascending", "A", "B", "C")
|
||||
|
||||
test.Tap(timeHeader)
|
||||
assertOrder("descending", "C", "B", "A")
|
||||
|
||||
// A new run arrives while the table is sorted newest-first: it must be
|
||||
// counted and placed in the order currently on screen, not the build-time one.
|
||||
events = append(events, event{Time: "2026-06-01 13:00:00", JobName: "D"})
|
||||
refresh()
|
||||
assertOrder("descending after refresh", "D", "C", "B", "A")
|
||||
|
||||
test.Tap(timeHeader)
|
||||
assertOrder("ascending after refresh", "A", "B", "C", "D")
|
||||
}
|
||||
|
||||
// TestHistoryCellTemplateIsPlainText guards the dropped per-cell TextStyle
|
||||
// assignment: the template must already carry the zero style, since nothing
|
||||
// resets it any more.
|
||||
func TestHistoryCellTemplateIsPlainText(t *testing.T) {
|
||||
testApp := test.NewApp()
|
||||
defer testApp.Quit()
|
||||
|
||||
var events []event
|
||||
content, _ := newHistoryView(&events)
|
||||
table := content.Objects[0].(*widget.Table)
|
||||
label, ok := table.CreateCell().(*widget.Label)
|
||||
if !ok {
|
||||
t.Fatal("history cell template is not a label")
|
||||
}
|
||||
if label.TextStyle != (fyne.TextStyle{}) {
|
||||
t.Errorf("cell template TextStyle = %+v, want the zero value", label.TextStyle)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewEventUsesConsistentTimestampShape(t *testing.T) {
|
||||
ev := newEvent(1, "Job", "OK", "detail")
|
||||
if _, err := time.Parse("2006-01-02 15:04:05", ev.Time); err != nil {
|
||||
|
||||
+3
-8
@@ -166,12 +166,14 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) {
|
||||
}
|
||||
selectedFolder = value
|
||||
filteredJobs = filteredJobIndexes(jobs, selectedFolder)
|
||||
list.Refresh()
|
||||
if len(filteredJobs) == 0 {
|
||||
// The "No folder" filter is intentionally allowed to be empty. It is a
|
||||
// real filter choice, not an error state, so the selection is cleared.
|
||||
// This path returns without reaching refreshView(), so it is the one
|
||||
// place the list has to be redrawn by hand.
|
||||
selected = -1
|
||||
updateDetails(-1)
|
||||
list.Refresh()
|
||||
return
|
||||
}
|
||||
selected = filteredJobs[0]
|
||||
@@ -223,7 +225,6 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) {
|
||||
}
|
||||
selected = indexOfID(jobs, created.ID)
|
||||
filteredJobs = filteredJobIndexes(jobs, selectedFolder)
|
||||
list.Refresh()
|
||||
list.Select(app.DisplayIndex(filteredJobs, selected))
|
||||
refreshView()
|
||||
})
|
||||
@@ -241,7 +242,6 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) {
|
||||
syncFromService()
|
||||
folderSelect.Options = folderOptions(jobs)
|
||||
folderSelect.Refresh()
|
||||
list.Refresh()
|
||||
refreshView()
|
||||
})
|
||||
})
|
||||
@@ -255,7 +255,6 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) {
|
||||
dialog.ShowError(err, w)
|
||||
return
|
||||
}
|
||||
list.Refresh()
|
||||
refreshView()
|
||||
})
|
||||
|
||||
@@ -287,7 +286,6 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) {
|
||||
stopAllButton.SetText("Disable auto")
|
||||
stopAllButton.SetIcon(theme.MediaPauseIcon())
|
||||
}
|
||||
list.Refresh()
|
||||
refreshView()
|
||||
}
|
||||
pauseButton := widget.NewButtonWithIcon("Pause", theme.MediaPauseIcon(), func() {
|
||||
@@ -299,8 +297,6 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) {
|
||||
dialog.ShowError(err, w)
|
||||
return
|
||||
}
|
||||
syncFromService()
|
||||
list.Refresh()
|
||||
refreshView()
|
||||
})
|
||||
deleteButton := widget.NewButtonWithIcon("Delete", theme.DeleteIcon(), func() {
|
||||
@@ -332,7 +328,6 @@ func newJobsView(w fyne.Window, svc *app.Service) (fyne.CanvasObject, func()) {
|
||||
} else {
|
||||
selected = filteredJobs[0]
|
||||
}
|
||||
list.Refresh()
|
||||
if selected >= 0 {
|
||||
list.Select(app.DisplayIndex(filteredJobs, selected))
|
||||
}
|
||||
|
||||
@@ -180,6 +180,58 @@ func jobsToolbar(t *testing.T, content fyne.CanvasObject) fyne.CanvasObject {
|
||||
return found
|
||||
}
|
||||
|
||||
// jobsToolbarButton returns the toolbar button with the given caption.
|
||||
func jobsToolbarButton(t *testing.T, content fyne.CanvasObject, text string) *widget.Button {
|
||||
t.Helper()
|
||||
found := findFirst(jobsToolbar(t, content), func(o fyne.CanvasObject) bool {
|
||||
button, ok := o.(*widget.Button)
|
||||
return ok && button.Text == text
|
||||
})
|
||||
if found == nil {
|
||||
t.Fatalf("jobs toolbar has no %q button", text)
|
||||
}
|
||||
return found.(*widget.Button)
|
||||
}
|
||||
|
||||
// jobsDetails narrows the search to the right pane. NewBorder keeps the centre
|
||||
// object first, so panel.Objects[0] is the details pane (see jobsSidebar).
|
||||
func jobsDetails(t *testing.T, content fyne.CanvasObject) fyne.CanvasObject {
|
||||
t.Helper()
|
||||
panel, ok := content.(*fyne.Container)
|
||||
if !ok || len(panel.Objects) == 0 {
|
||||
t.Fatal("jobs view is not the expected Border container")
|
||||
}
|
||||
return panel.Objects[0]
|
||||
}
|
||||
|
||||
// jobsDetailsActivity returns the "Selected job activity" list, the only
|
||||
// widget.List in the details pane.
|
||||
func jobsDetailsActivity(t *testing.T, content fyne.CanvasObject) *widget.List {
|
||||
t.Helper()
|
||||
found := findFirst(jobsDetails(t, content), func(o fyne.CanvasObject) bool {
|
||||
_, ok := o.(*widget.List)
|
||||
return ok
|
||||
})
|
||||
if found == nil {
|
||||
t.Fatal("details pane has no activity list")
|
||||
}
|
||||
return found.(*widget.List)
|
||||
}
|
||||
|
||||
// jobsDetailsTitle reads the details pane's heading, which detailsPanel builds
|
||||
// as the first bold label in the pane.
|
||||
func jobsDetailsTitle(t *testing.T, content fyne.CanvasObject) string {
|
||||
t.Helper()
|
||||
found := findFirst(jobsDetails(t, content), func(o fyne.CanvasObject) bool {
|
||||
label, ok := o.(*widget.Label)
|
||||
return ok && label.TextStyle.Bold
|
||||
})
|
||||
if found == nil {
|
||||
t.Fatal("details pane has no title label")
|
||||
}
|
||||
return found.(*widget.Label).Text
|
||||
}
|
||||
|
||||
func jobsViewToggle(t *testing.T, content fyne.CanvasObject) *widget.Button {
|
||||
t.Helper()
|
||||
found := findFirst(jobsSidebar(t, content), func(o fyne.CanvasObject) bool {
|
||||
@@ -295,6 +347,71 @@ func TestJobsSidebarWidthIsItsContent(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestToolbarButtonRedrawsRowAndDetails is the regression guard for F12: the
|
||||
// toolbar handlers no longer re-read the service or refresh the list
|
||||
// themselves, so refreshView alone has to re-snapshot the jobs and repopulate
|
||||
// the details pane. If it ever stops doing either, the row renders a stale
|
||||
// status and the details lose the selection — neither is a compile error.
|
||||
func TestToolbarButtonRedrawsRowAndDetails(t *testing.T) {
|
||||
testApp := test.NewApp()
|
||||
defer testApp.Quit()
|
||||
w := testApp.NewWindow("test")
|
||||
defer w.Close()
|
||||
|
||||
store := newTestStore(t)
|
||||
jobs := []domain.Job{
|
||||
{ID: 1, Name: "First", Schedule: "@every 1m", Command: "echo one", Enabled: true},
|
||||
{ID: 2, Name: "Second", Schedule: "@every 2m", Command: "echo two", Enabled: true},
|
||||
}
|
||||
svc := app.NewService(store, jobs)
|
||||
defer svc.Stop()
|
||||
|
||||
content, _ := newJobsView(w, svc)
|
||||
w.SetContent(content)
|
||||
|
||||
list := jobsList(t, content)
|
||||
// Row layout: VBox(nameLine, meta, status), nameLine = Border(name, inlineStatus).
|
||||
rowText := func(id int) (name string, status string) {
|
||||
t.Helper()
|
||||
row := list.CreateItem().(*fyne.Container)
|
||||
list.UpdateItem(id, row)
|
||||
nameLine := row.Objects[0].(*fyne.Container)
|
||||
return nameLine.Objects[0].(*widget.Label).Text, row.Objects[2].(*widget.Label).Text
|
||||
}
|
||||
|
||||
activity := jobsDetailsActivity(t, content)
|
||||
|
||||
list.Select(1)
|
||||
if got := jobsDetailsTitle(t, content); got != "Second" {
|
||||
t.Fatalf("details title after selecting row 1 = %q, want %q", got, "Second")
|
||||
}
|
||||
if _, status := rowText(1); status == "Paused" {
|
||||
t.Fatal("the second job should start enabled")
|
||||
}
|
||||
if got := activity.Length(); got != 0 {
|
||||
t.Fatalf("activity rows before the tap = %d, want 0", got)
|
||||
}
|
||||
|
||||
test.Tap(jobsToolbarButton(t, content, "Pause"))
|
||||
|
||||
if svc.Jobs()[1].Enabled {
|
||||
t.Fatal("tapping Pause did not reach the service")
|
||||
}
|
||||
name, status := rowText(1)
|
||||
if name != "Second" || status != "Paused" {
|
||||
t.Errorf("row 1 after Pause = (%q, %q), want (%q, %q)", name, status, "Second", "Paused")
|
||||
}
|
||||
if got := jobsDetailsTitle(t, content); got != "Second" {
|
||||
t.Errorf("details title after Pause = %q, want the selection kept at %q", got, "Second")
|
||||
}
|
||||
// The pause writes an activity record. Seeing it here is what proves
|
||||
// refreshView repopulated the details pane rather than leaving the panel on
|
||||
// the snapshot it held before the tap.
|
||||
if got := activity.Length(); got != 1 {
|
||||
t.Errorf("activity rows after the tap = %d, want the pause record", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestViewToggleTextNamesTheAction(t *testing.T) {
|
||||
cases := []struct {
|
||||
current domain.JobListView
|
||||
|
||||
Reference in New Issue
Block a user