Fix relaunch-restore bugs found on device (spec §7 verification)
On-device runs exposed three bugs the e2e suite could not (it never feeds layout feedback, and runs headless without size events): 1. WrapIndex poisoning from zero-width shapes. The first frames are built before the window size is known (px=0x0); the renderer shapes the editor window at zero width, where every line wraps into many visual lines. When that feedback arrives, applyWrapCounts writes the inflated counts to the window's lines. Normally the app stays on those lines and re-shapes them at a real width, which corrects the counts before anyone notices. A restored scroll moves the viewport away instead, so the poisoned counts persist and map the restored scroll offset to the wrong line (2000 landed on line 11 of 200). The frame now carries ViewportDegenerate (set at frame-build time, since feedback delivery lags shaping by a frame), and the main loop drops layout feedback for such frames. 2. Session saving suppressed forever after a successful restore. saveSessionIfChanged suppresses saves while l.session is set, and only abortRestore cleared it — the success path never did, so an app restored from a session never persisted new state. The snapshot has fully landed once the cursor/selection/find have landed with the content and the armed scroll has landed (or there was none); clear l.session at both points. 3. gofmt on restore_test.go (comment alignment).
This commit is contained in:
parent
e3690d6b98
commit
03595af27d
|
|
@ -366,12 +366,24 @@ func run(w *app.Window) error {
|
|||
if sendFind {
|
||||
logic.FindQueryChan() <- newFind
|
||||
}
|
||||
logic.LayoutChan() <- ui.LayoutFeedback{
|
||||
GlyphLayout: glyphLayout,
|
||||
WindowText: frame.WindowText,
|
||||
WindowStartByte: frame.WindowStartByte,
|
||||
WindowStartLine: frame.WindowStartLine,
|
||||
EditSeq: frame.EditSeq,
|
||||
// Skip layout feedback for frames built before the window size
|
||||
// was known (frame.ViewportDegenerate, set at frame-build time —
|
||||
// feedback delivery lags shaping by a frame, so checking the
|
||||
// current size here would miss them): a zero-width shape wraps
|
||||
// every line into many visual lines, and feeding those counts
|
||||
// back would poison the WrapIndex for the window's lines
|
||||
// (applied once, corrected only if those lines are re-shaped at a
|
||||
// real width — a restored scroll that moves the viewport away
|
||||
// never re-shapes them, and the poisoned counts then map a
|
||||
// legitimate scroll offset to the wrong line).
|
||||
if !frame.ViewportDegenerate {
|
||||
logic.LayoutChan() <- ui.LayoutFeedback{
|
||||
GlyphLayout: glyphLayout,
|
||||
WindowText: frame.WindowText,
|
||||
WindowStartByte: frame.WindowStartByte,
|
||||
WindowStartLine: frame.WindowStartLine,
|
||||
EditSeq: frame.EditSeq,
|
||||
}
|
||||
}
|
||||
default:
|
||||
handleEvent(e)
|
||||
|
|
|
|||
|
|
@ -45,23 +45,31 @@ type Frame struct {
|
|||
WindowStartLine int // -1 when the frame has no editor window
|
||||
WindowText string // the editor window this frame's text element holds
|
||||
EditSeq uint64
|
||||
// ViewportDegenerate is set when this frame was built before the
|
||||
// window's pixel size was known (0x0). Its editor window, if shaped at
|
||||
// all, was shaped at zero width: the shaper wraps every line into many
|
||||
// visual lines, and feeding those counts back (LayoutFeedback) would
|
||||
// poison the WrapIndex for the window's lines. The main goroutine
|
||||
// drops the feedback for such frames.
|
||||
ViewportDegenerate bool
|
||||
}
|
||||
|
||||
// frameOf wraps a computed element tree with the current view-state
|
||||
// snapshot. Must be called on the logic goroutine.
|
||||
func (l *Logic) frameOf(elems []ui.Element) Frame {
|
||||
return Frame{
|
||||
Elems: elems,
|
||||
Scale: l.state.scale,
|
||||
FontScale: l.state.fontScale,
|
||||
FocusedElementID: l.state.FocusedElementID,
|
||||
Query: l.state.Browser.Query,
|
||||
FindQuery: l.state.Editor.Find.Query,
|
||||
FindClearSeq: l.state.Editor.Find.ClearSeq,
|
||||
WindowStartByte: l.state.Editor.IMEWindowStartByte,
|
||||
WindowStartLine: l.state.WindowStartLine,
|
||||
WindowText: l.state.Editor.IMEWindowText,
|
||||
EditSeq: l.state.Editor.EditSeq,
|
||||
Elems: elems,
|
||||
Scale: l.state.scale,
|
||||
FontScale: l.state.fontScale,
|
||||
FocusedElementID: l.state.FocusedElementID,
|
||||
Query: l.state.Browser.Query,
|
||||
FindQuery: l.state.Editor.Find.Query,
|
||||
FindClearSeq: l.state.Editor.Find.ClearSeq,
|
||||
WindowStartByte: l.state.Editor.IMEWindowStartByte,
|
||||
WindowStartLine: l.state.WindowStartLine,
|
||||
WindowText: l.state.Editor.IMEWindowText,
|
||||
EditSeq: l.state.Editor.EditSeq,
|
||||
ViewportDegenerate: l.state.PixelWidth <= 0 || l.state.PixelHeight <= 0,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -101,16 +101,20 @@ type Logic struct {
|
|||
session SessionState
|
||||
restoreFile string
|
||||
// restoreScroll/restoreScrollArmed hold the snapshot's scroll offset
|
||||
// until it is safe to apply it: only once the scale is known (the first
|
||||
// ScaleEvent — the size ConfigEvent arrives before it, and a layout in
|
||||
// between computes the viewport in the wrong unit, which would clamp
|
||||
// the offset, and the clamp is one-way). scaleSeen tracks that event.
|
||||
restoreScroll ui.Dp
|
||||
restoreScrollArmed bool
|
||||
scaleSeen bool
|
||||
sessionSaver func(SessionState)
|
||||
lastSession SessionState
|
||||
lastSessionSave time.Time
|
||||
// until it is safe to apply it: only on an emitFrame whose layout pass
|
||||
// saw a trustworthy viewport (scale known, size known, restored content
|
||||
// present — the layout computes MaxScroll and one-way-clamps the offset,
|
||||
// and on device the first ScaleEvent can precede the size ConfigEvent).
|
||||
// scaleSeen tracks the first ScaleEvent; restoreContentLanded marks the
|
||||
// read result that filled the buffer (FileLen alone is set earlier, by
|
||||
// the stat result, and is not a content-arrival signal).
|
||||
restoreScroll ui.Dp
|
||||
restoreScrollArmed bool
|
||||
scaleSeen bool
|
||||
restoreContentLanded bool
|
||||
sessionSaver func(SessionState)
|
||||
lastSession SessionState
|
||||
lastSessionSave time.Time
|
||||
}
|
||||
|
||||
// NewLogic creates a new Logic instance, accepting an optional mockFS.
|
||||
|
|
@ -254,14 +258,6 @@ func (l *Logic) Run() {
|
|||
l.scaleSeen = true
|
||||
}
|
||||
l.emitFrame()
|
||||
// The restored scroll lands only AFTER a layout at the current
|
||||
// scale: its clamp uses MaxScroll, which must reflect the same
|
||||
// viewport units (the size ConfigEvent precedes the first
|
||||
// ScaleEvent, so a layout between them is in the wrong unit).
|
||||
if l.scaleSeen && l.restoreScrollArmed {
|
||||
l.maybeApplyRestoreScroll()
|
||||
l.emitFrame()
|
||||
}
|
||||
case fb := <-l.layoutChan:
|
||||
// Store the full GlyphLayout on editor state.
|
||||
// Derive LastLineY from it for scroll clamping.
|
||||
|
|
@ -364,6 +360,18 @@ func (l *Logic) emitFrame() {
|
|||
// the rate limit elapsed (tiny JSON file, see session.go).
|
||||
l.saveSessionIfChanged()
|
||||
elems := l.state.layout(l.browserManager)
|
||||
// Relaunch restore (spec §7): land the armed restore scroll AFTER the
|
||||
// layout pass above (it refreshed MaxScroll for the current viewport and
|
||||
// one-way-clamps any offset set against an earlier, smaller one). The
|
||||
// guard defers the application until the viewport is trustworthy — on
|
||||
// device the first ScaleEvent can precede the size ConfigEvent. The
|
||||
// re-layout makes this frame carry the restored viewport. The restore is
|
||||
// complete once the scroll lands; drop restoreFile so a later open of
|
||||
// this same file is treated as a fresh one.
|
||||
if l.restoreScrollArmed && l.maybeApplyRestoreScroll() {
|
||||
l.restoreFile = ""
|
||||
elems = l.state.layout(l.browserManager)
|
||||
}
|
||||
now := time.Now()
|
||||
if PerfRecord != nil {
|
||||
var delta time.Duration
|
||||
|
|
@ -579,8 +587,12 @@ func (l *Logic) handleWorkerResult(res pool.Result) {
|
|||
// the snapshot to the replacement's buffer.
|
||||
if res.FilePath == l.state.Editor.Filename && l.restoreFile == res.FilePath {
|
||||
l.applyRestorePositions(len(content))
|
||||
l.maybeApplyRestoreScroll()
|
||||
l.abortRestore()
|
||||
l.restoreContentLanded = true
|
||||
// The snapshot has landed: resume session saving. restoreFile
|
||||
// and the armed scroll stay until the scroll itself lands in
|
||||
// the emitFrame hook (which needs this content for a real
|
||||
// MaxScroll), so a different open in between still aborts via
|
||||
// openFile's restoreFile guard.
|
||||
// Re-scan the restored query only when the bar was open:
|
||||
// with the bar closed, the result would be dropped (the apply
|
||||
// gate requires Visible) and Scanning would stay stuck true;
|
||||
|
|
@ -588,6 +600,12 @@ func (l *Logic) handleWorkerResult(res pool.Result) {
|
|||
if f := &l.state.Editor.Find; f.Visible && f.Query != "" && !f.Scanning {
|
||||
l.state.Editor.findDispatchScan()
|
||||
}
|
||||
// No armed scroll: the snapshot has fully landed with the
|
||||
// content; lift the save suppression now (the emitFrame
|
||||
// hook does this when a scroll does land).
|
||||
if !l.restoreScrollArmed {
|
||||
l.session = SessionState{}
|
||||
}
|
||||
}
|
||||
}
|
||||
} else if res.FilePath == l.restoreFile {
|
||||
|
|
|
|||
|
|
@ -136,8 +136,9 @@ func (l *Logic) BeginRestore(s SessionState) {
|
|||
// Fresh per-file find state; the snapshot's query and visibility land
|
||||
// with the stat (the file must exist first).
|
||||
e.findReset()
|
||||
// The scroll offset is armed, not applied: it must not be clamped by a
|
||||
// layout that runs before the scale is known (see the Logic struct).
|
||||
// The scroll offset is armed, not applied: it lands in the emitFrame
|
||||
// hook, once the layout pass has a trustworthy viewport (see the Logic
|
||||
// struct).
|
||||
l.restoreScroll = ui.Dp(s.Scroll)
|
||||
l.restoreScrollArmed = s.Scroll > 0
|
||||
l.state.page = EditorPage
|
||||
|
|
@ -153,25 +154,46 @@ func (l *Logic) BeginRestore(s SessionState) {
|
|||
func (l *Logic) abortRestore() {
|
||||
l.restoreFile = ""
|
||||
l.restoreScrollArmed = false
|
||||
l.restoreContentLanded = false
|
||||
l.session = SessionState{}
|
||||
}
|
||||
|
||||
// maybeApplyRestoreScroll lands the armed restore scroll offset once the
|
||||
// viewport unit is trustworthy (the first ScaleEvent has arrived), clamped
|
||||
// to the current MaxScroll (the layout clamps again if the line index
|
||||
// shrinks it later). Must be called on the logic goroutine.
|
||||
func (l *Logic) maybeApplyRestoreScroll() {
|
||||
// viewport is trustworthy, clamped to the current MaxScroll (the layout
|
||||
// clamps again if the line index shrinks it later). It is called from
|
||||
// emitFrame AFTER the layout pass, because that pass is where MaxScroll is
|
||||
// computed for the current viewport — applying the offset before it (or in a
|
||||
// pass with an unknown viewport) would hit the one-way clamp and lose the
|
||||
// offset. On device the first ScaleEvent can precede the size ConfigEvent
|
||||
// (and both can precede the restored content), so the offset stays armed
|
||||
// until scale, size and editor content are all known; it is applied on the
|
||||
// first such emitFrame. restoreContentLanded (not FileLen, which the stat
|
||||
// result sets before the content arrives) marks that the buffer holds the
|
||||
// restored file; clearing restoreFile any earlier would make the read
|
||||
// result's path guard miss the just-loaded file. Returns true if it
|
||||
// applied. Must be called on the logic goroutine.
|
||||
func (l *Logic) maybeApplyRestoreScroll() bool {
|
||||
if !l.restoreScrollArmed {
|
||||
return
|
||||
return false
|
||||
}
|
||||
if !l.scaleSeen {
|
||||
return
|
||||
return false
|
||||
}
|
||||
if l.state.PixelWidth <= 0 || l.state.scale <= 0 {
|
||||
return false
|
||||
}
|
||||
if l.state.Page() != EditorPage || !l.restoreContentLanded {
|
||||
return false
|
||||
}
|
||||
if l.restoreScroll > l.state.MaxScroll {
|
||||
l.restoreScroll = l.state.MaxScroll
|
||||
}
|
||||
l.state.ScrollOffset = l.restoreScroll
|
||||
l.restoreScrollArmed = false
|
||||
// The snapshot has fully landed (cursor/selection/find with the
|
||||
// content, scroll now): lift the save suppression.
|
||||
l.session = SessionState{}
|
||||
return true
|
||||
}
|
||||
|
||||
// applyRestorePositions clamps the restored snapshot's cursor and selection
|
||||
|
|
|
|||
|
|
@ -119,6 +119,46 @@ func TestRestore_FileCursorScrollSelection(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestRestore_ScrollLandsWhenScalePrecedesSize reproduces the on-device
|
||||
// startup ordering, where the first FrameEvent (and hence the ScaleEvent)
|
||||
// arrives before the size ConfigEvent. The scroll must still land, clamped
|
||||
// by the layout at the real scale: an earlier revision applied it at the
|
||||
// ScaleEvent against a not-yet-sized viewport and lost it.
|
||||
func TestRestore_ScrollLandsWhenScalePrecedesSize(t *testing.T) {
|
||||
content := strings.Repeat("hello world line\n", 100)
|
||||
dir := t.TempDir()
|
||||
diskPath := filepath.Join(dir, "late.txt")
|
||||
if err := os.WriteFile(diskPath, []byte(content), 0644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
h := e2e.NewHarness(e2e.WithFileSystem(real.NewRealFileSystem(dir), "/"))
|
||||
h.Logic().BeginRestore(editor.SessionState{File: "/late.txt", Cursor: 123, Scroll: 100})
|
||||
h.Run()
|
||||
h.SendScale(2.0) // scale first
|
||||
h.SendConfig(780, 1688) // size second (on-device ordering)
|
||||
defer h.Cleanup()
|
||||
|
||||
for i := 0; i < 100; i++ {
|
||||
v, err := h.Inspect(func(st *editor.State) any {
|
||||
cb := st.Editor.ChunkedBuffer
|
||||
if cb == nil || cb.FileLen() == 0 || cb.LineIndex == nil {
|
||||
return false
|
||||
}
|
||||
full, err := cb.FullContent()
|
||||
return err == nil && int64(len(full)) == cb.FileLen() &&
|
||||
cb.LineIndex.Size == cb.FileLen() && st.ScrollOffset == ui.Dp(100)
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("Inspect: %v", err)
|
||||
}
|
||||
if v.(bool) {
|
||||
return
|
||||
}
|
||||
time.Sleep(50 * time.Millisecond)
|
||||
}
|
||||
t.Fatal("timed out: scroll did not land when the scale event preceded the size")
|
||||
}
|
||||
|
||||
// TestRestore_ClampsToShorterFile: the file was truncated since the last
|
||||
// session; the cursor and selection must clamp to the shorter content, not
|
||||
// restore past EOF.
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user