Merge pull request #133 from markusressel/feature/dialog-improvements

Dialog UI Sizing, UX, and Alignment Improvements

Author: markusressel

internal/ui/dialog/column_selection_dialog.go

@@ -2,6 +2,7 @@ package dialog
 
 import (
 	"slices"
+	"unicode/utf8"
 	"zfs-file-history/internal/ui/shortcut_helper"
 	"zfs-file-history/internal/ui/table"
 	"zfs-file-history/internal/ui/util"
@@ -97,7 +98,25 @@ func (d *ColumnSelectionDialog) createLayout() {
 		return action, event
 	})
 
-	d.layout = createModal(d.title, content, 70, 20)
+	maxColWidth := 0
+	for _, col := range d.allColumns {
+		if col != nil {
+			if l := utf8.RuneCountInString(col.Title); l > maxColWidth {
+				maxColWidth = l
+			}
+		}
+	}
+
+	extraWidth := 2 * (maxColWidth + 4)
+	staticHeight := 1 + len(d.allColumns) + 2
+
+	width, height := CalculateDialogSize(DialogSizeConstraints{
+		Title:             d.title,
+		ExtraContentWidth: extraWidth,
+		StaticHeight:      staticHeight,
+	})
+
+	d.layout = createModal(d.title, content, width, height)
 	d.layout.SetInputCapture(d.captureInput)
 }
 

internal/ui/dialog/column_selection_dialog_test.go

@@ -0,0 +1,92 @@
+package dialog
+
+import (
+	"testing"
+	"zfs-file-history/internal/ui/table"
+
+	"github.com/rivo/tview"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestComputeAvailableColumns(t *testing.T) {
+	all := []*table.Column{
+		{Id: table.ColumnId(1), Title: "Col 1"},
+		{Id: table.ColumnId(2), Title: "Col 2"},
+		{Id: table.ColumnId(3), Title: "Col 3"},
+	}
+	active := []*table.Column{
+		{Id: table.ColumnId(2), Title: "Col 2"},
+	}
+
+	available := computeAvailableColumns(all, active)
+	assert.Len(t, available, 2)
+	assert.Equal(t, table.ColumnId(1), available[0].Id)
+	assert.Equal(t, table.ColumnId(3), available[1].Id)
+}
+
+func TestColumnSelectionDialog(t *testing.T) {
+	app := tview.NewApplication()
+	all := []*table.Column{
+		{Id: table.ColumnId(1), Title: "Col 1"},
+		{Id: table.ColumnId(2), Title: "Col 2"},
+	}
+	active := []*table.Column{
+		{Id: table.ColumnId(1), Title: "Col 1"},
+	}
+
+	d := NewColumnSelectionDialog(app, "Column Selection", all, active, func(activeColumns []*table.Column) {})
+	assert.Equal(t, "ColumnSelectionDialog", d.GetName())
+	assert.NotNil(t, d.GetLayout())
+	assert.NotNil(t, d.GetActionChannel())
+}
+
+func TestColumnSelectionDialog_Actions(t *testing.T) {
+	app := tview.NewApplication()
+	all := []*table.Column{
+		{Id: table.ColumnId(1), Title: "Col 1"},
+		{Id: table.ColumnId(2), Title: "Col 2"},
+		{Id: table.ColumnId(3), Title: "Col 3"},
+	}
+	active := []*table.Column{
+		{Id: table.ColumnId(1), Title: "Col 1"},
+		{Id: table.ColumnId(2), Title: "Col 2"},
+	}
+
+	changeCalled := false
+	d := NewColumnSelectionDialog(app, "Column Selection", all, active, func(activeColumns []*table.Column) {
+		changeCalled = true
+	})
+
+	// Test Close
+	go func() {
+		d.Close()
+	}()
+	action := <-d.GetActionChannel()
+	assert.Equal(t, DialogCloseActionId, action)
+
+	// Test add selected available column
+	d.addSelectedAvailableColumn()
+	assert.True(t, changeCalled)
+	assert.Len(t, d.activeColumns, 3)
+
+	// Reset changeCalled
+	changeCalled = false
+
+	// Test remove selected active column
+	d.activeTable.Select(1, 0)
+	d.removeSelectedActiveColumn()
+	assert.True(t, changeCalled)
+	assert.Len(t, d.activeColumns, 2)
+
+	// Reset changeCalled
+	changeCalled = false
+
+	// Test move active column up/down
+	d.activeTable.Select(1, 0)
+	d.moveActiveColumnUp()
+	assert.True(t, changeCalled)
+
+	changeCalled = false
+	d.moveActiveColumnDown()
+	assert.True(t, changeCalled)
+}

internal/ui/dialog/delete_file_dialog.go

@@ -22,7 +22,5 @@ func NewDeleteFileDialog(application *tview.Application, file *data.FileBrowserE
 		" 🗑️ Delete File ",
 		fmt.Sprintf("Delete '%s'?", file.Name),
 		buildConfirmDialogOptions(DeleteFileDialogDeleteFileActionId, localization.LocalizationCommonDelete, file.HasReal(), DialogSeverityDanger),
-		50,
-		6,
 	)
 }

internal/ui/dialog/delete_snapshot_dialog.go

@@ -21,7 +21,5 @@ func NewDeleteSnapshotDialog(application *tview.Application, snapshot *data.Snap
 		" 💥 Destroy Snapshot ",
 		fmt.Sprintf("Destroy '%s'?", snapshot.Snapshot.Name),
 		buildConfirmDialogOptions(DeleteSnapshotDialogDeleteSnapshotActionId, "Destroy", true, DialogSeverityDanger),
-		50,
-		6,
 	)
 }

internal/ui/dialog/file_action_dialog.go

@@ -30,8 +30,6 @@ func NewFileActionDialog(application *tview.Application, file *data.FileBrowserE
 		localization.LocalizationSelectActionDialogTitle,
 		fmt.Sprintf("What do you want to do with '%s'?", file.Name),
 		dialogOptions,
-		50,
-		15,
 	)
 }
 

internal/ui/dialog/file_diff_dialog.go

@@ -82,8 +82,12 @@ func (d *FileDiffDialog) createLayout() {
 	dialogContent.AddItem(closeTextView, 1, 0, false)
 	dialogContent.SetBorderPadding(0, 0, 1, 1)
 
-	width := 80
-	height := 20
+	width, height := CalculateDialogSize(DialogSizeConstraints{
+		Title:             dialogTitle,
+		ExtraContentWidth: 74,
+		StaticHeight:      18, // Sane content height for scrollable diff text
+	})
+
 	dialog := createModal(dialogTitle, dialogContent, width, height)
 	dialog.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
 		if event.Key() == tcell.KeyEscape {

internal/ui/dialog/file_diff_dialog_test.go

@@ -0,0 +1,31 @@
+package dialog
+
+import (
+	"testing"
+	"zfs-file-history/internal/data"
+	"zfs-file-history/internal/zfs"
+
+	"github.com/rivo/tview"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewFileDiffDialog(t *testing.T) {
+	app := tview.NewApplication()
+	file := &data.FileBrowserEntry{
+		RealFile: &data.RealFile{Path: "/pool/ds1/file.txt"},
+	}
+	snapshot := &data.SnapshotBrowserEntry{
+		Snapshot: &zfs.Snapshot{
+			Name: "snap-1",
+			ParentDataset: &zfs.Dataset{
+				Path:          "/pool/ds1",
+				HiddenZfsPath: "/pool/ds1/.zfs",
+			},
+		},
+	}
+
+	d := NewFileDiffDialog(app, file, snapshot)
+	assert.Equal(t, "FileDiffDialog", d.GetName())
+	assert.NotNil(t, d.GetLayout())
+	assert.NotNil(t, d.GetActionChannel())
+}

internal/ui/dialog/help_dialog.go

@@ -2,6 +2,7 @@ package dialog
 
 import (
 	"fmt"
+	"unicode/utf8"
 	"zfs-file-history/internal/ui/theme"
 
 	"github.com/gdamore/tcell/v2"
@@ -49,7 +50,33 @@ func (p *HelpPage) createLayout() {
 		setHelpTableRow(helpTable, row, entry)
 	}
 
-	p.layout = createModal(" ℹ️ Help ", helpTable, 60, 14)
+	maxKeyWidth := 0
+	maxValueWidth := 0
+	for _, entry := range helpTableEntries {
+		if entry == emptyEntry {
+			continue
+		}
+		kw := utf8.RuneCountInString(entry.Key) + 1 // key + colon
+		if kw > maxKeyWidth {
+			maxKeyWidth = kw
+		}
+		vw := utf8.RuneCountInString(entry.Value)
+		if vw > maxValueWidth {
+			maxValueWidth = vw
+		}
+	}
+
+	// Total width is the sum of both columns plus the 1-character table column gap
+	maxHelpWidth := maxKeyWidth + 1 + maxValueWidth
+
+	title := " ℹ️ Help "
+	width, height := CalculateDialogSize(DialogSizeConstraints{
+		Title:             title,
+		ExtraContentWidth: maxHelpWidth,
+		StaticHeight:      len(helpTableEntries),
+	})
+
+	p.layout = createModal(title, helpTable, width, height)
 }
 
 func setHelpTableRow(helpTable *tview.Table, row int, entry *TableEntry) {

internal/ui/dialog/help_dialog_test.go

@@ -46,3 +46,8 @@ func TestSetHelpTableRow_EmptyEntry_LeavesKeyEmpty(t *testing.T) {
 	assert.Equal(t, tcell.ColorWhite, keyFg)
 	assert.Equal(t, "", valueCell.Text)
 }
+
+func TestNewHelpPage(t *testing.T) {
+	p := NewHelpPage()
+	assert.NotNil(t, p.GetLayout())
+}

internal/ui/dialog/restore_file_dialog.go

@@ -24,8 +24,6 @@ func NewRestoreFileDialog(application *tview.Application, file *data.FileBrowser
 		" ♻️ Restore File ",
 		fmt.Sprintf("Restore '%s'?", file.Name),
 		buildRestoreDialogOptions(file),
-		50,
-		6,
 	)
 }