diff --git a/exec_test.go b/exec_test.go index 63d076ce..ea37b514 100644 --- a/exec_test.go +++ b/exec_test.go @@ -323,6 +323,9 @@ func mutateBothOtherUndo(t *testing.T, g *globals) { firstwin := g.row.col[0].w[0] secondwin := g.row.col[0].w[1] + t.Logf("firstwin, %q", firstwin.body.file.String()) + t.Logf("secondwin, %q", secondwin.body.file.String()) + // Modify the firstwin. firstwin.body.q0 = 3 firstwin.body.q1 = 10 @@ -330,9 +333,14 @@ func mutateBothOtherUndo(t *testing.T, g *globals) { firstwin.body.file.Mark(global.seq) cut(&firstwin.tag, &firstwin.body, nil, false, true, "") + t.Logf("after cut firstwin, %q", firstwin.body.file.String()) + // Run undo from one of the windows. (i.e. same as clicking on the Undo action.) // Cut should remain, original global edit should get Undone only in secondwin. undo(&secondwin.tag, nil, nil, true /* this is an undo */, false /* ignored */, "") + + t.Logf("after undo firstwin, %q", firstwin.body.file.String()) + t.Logf("after undo secondwin, %q", secondwin.body.file.String()) } func mutateBranchedAndRejoined(t *testing.T, g *globals) { diff --git a/file/buffer_adapter.go b/file/buffer_adapter.go index b65ed881..a6889da1 100644 --- a/file/buffer_adapter.go +++ b/file/buffer_adapter.go @@ -3,7 +3,6 @@ package file import ( "bufio" "bytes" - "flag" "io" "strings" ) @@ -73,17 +72,9 @@ type BufferAdapter interface { // Enforce that *file.File implements BufferAdapter. var ( _ BufferAdapter = (*File)(nil) - - // TODO(rjk): Make this compile. :-) _ BufferAdapter = (*Buffer)(nil) - - newTypeBuffer bool ) -func init() { - flag.BoolVar(&newTypeBuffer, "newtypebuffer", false, "turn on the file.Buffer new Buffer implementation") -} - func NewTypeBuffer(inputrunes []rune, oeb *ObservableEditableBuffer) BufferAdapter { // TODO(rjk): Figure out how to plumb in the oeb object to setup Undo // observer callbacks. diff --git a/file/file_file_test.go b/file/file_file_test.go index 32538d7f..04765cd2 100644 --- a/file/file_file_test.go +++ b/file/file_file_test.go @@ -36,7 +36,7 @@ func TestFileHandlesNilEpsilonDelta(t *testing.T) { {"undo (nil delta)", true, 14, 17, 14, 17, nil, nil}, {"redo (nil epsilon)", false, 14, 17, 14, 17, nil, nil}, } { - oeb := MakeObservableEditableBuffer("", []rune("This is an example sentence.\n")) + oeb := _makeObservableEditableBuffer("", []rune("This is an example sentence.\n"), false) fimpl := oeb.f.(*File) fimpl.delta = tc.delta diff --git a/file/file_test.go b/file/file_test.go index 0a7b327d..ec26a73a 100644 --- a/file/file_test.go +++ b/file/file_test.go @@ -137,7 +137,11 @@ func TestFileUndoRedo(t *testing.T) { // Because of how seq managed the number of Undo actions, this // corresponds to the case of not incrementing seq and undoes every // action in the log. - f.Undo(true) + f.checkedUndo(true, t, undoexpectation{ + ok: true, + q0: 0, + q1: 0, + }) t.Log(f.f.String()) t.Log(f.seq, f.f.HasUncommitedChanges(), f.Dirty(), f.putseq) @@ -145,7 +149,21 @@ func TestFileUndoRedo(t *testing.T) { &stateSummary{false, false, true, false, ""}) // Redo - f.Undo(false) + // This is actually wrong with legacy file. It will leave a portion of the selection + // set. + if nt == "newtype" { + f.checkedUndo(false, t, undoexpectation{ + ok: true, + q0: 0, + q1: 9, + }) + } else { + f.checkedUndo(false, t, undoexpectation{ + ok: true, + q0: 6, + q1: 9, + }) + } t.Log(f.f.String()) t.Log(f.seq, f.f.HasUncommitedChanges(), f.Dirty(), f.putseq) @@ -175,13 +193,21 @@ func TestFileUndoRedoWithMark(t *testing.T) { check(t, "TestFileUndoRedoWithMark after 2 inserts", f, &stateSummary{false, true, false, true, s1 + s2}) - f.Undo(true) + f.checkedUndo(true, t, undoexpectation{ + ok: true, + q0: 6, + q1: 6, + }) check(t, "TestFileUndoRedoWithMark after 1 undo", f, &stateSummary{false, true, true, true, s1}) // Redo - f.Undo(false) + f.checkedUndo(false, t, undoexpectation{ + ok: true, + q0: 6, + q1: 9, + }) check(t, "TestFileUndoRedoWithMark after 1 redo", f, &stateSummary{false, true, false, true, s1 + s2}) @@ -274,7 +300,9 @@ func TestFileLoadUndoHash(t *testing.T) { } // Undo renmaing the file. - f.Undo(true) + f.checkedUndo(true, t, undoexpectation{ + ok: false, + }) check(t, "TestFileLoadUndoHash after Undo", f, &stateSummary{false, false, true, false, s2 + s2}) if got, want := f.Name(), "edwood"; got != want { @@ -328,7 +356,11 @@ func TestFileInsertDeleteUndo(t *testing.T) { &stateSummary{false, true, false, true, "yi 海老hi 海老麺麺"}) t.Logf("after setup seq %d, putseq %d", f.seq, f.putseq) - f.Undo(true) + f.checkedUndo(true, t, undoexpectation{ + ok: true, + q0: 5, + q1: 5, + }) check(t, "TestFileInsertDeleteUndo after 1 Undo", f, &stateSummary{false, true, true, true, "yi 海老麺"}) t.Logf("after 1 Undo seq %d, putseq %d", f.seq, f.putseq) @@ -365,12 +397,21 @@ func TestFileInsertDeleteUndo(t *testing.T) { }, }) - f.Undo(true) // 2 deletes should get removed because they have the same sequence. + // 2 deletes should get removed because they have the same sequence. + f.checkedUndo(true, t, undoexpectation{ + ok: true, + q0: 0, + q1: 1, + }) check(t, "TestFileInsertDeleteUndo after 2 Undo", f, &stateSummary{false, true, true, true, "byehi 海老麺"}) t.Logf("after 2 Undo seq %d, putseq %d", f.seq, f.putseq) - f.Undo(false) // 2 deletes should be put back. + f.checkedUndo(false, t, undoexpectation{ // 2 deletes should be put back. + ok: true, + q0: 1, + q1: 1, + }) check(t, "TestFileInsertDeleteUndo after 1 Undo", f, &stateSummary{false, true, true, true, "yi 海老麺"}) t.Logf("after 1 Redo seq %d, putseq %d", f.seq, f.putseq) @@ -421,7 +462,11 @@ func TestFileRedoSeq(t *testing.T) { t.Errorf("TestFileRedoSeq no redo. got %#v want %#v", got, want) } - f.Undo(true) + f.checkedUndo(true, t, undoexpectation{ + ok: true, + q0: 0, + q1: 0, + }) check(t, "TestFileRedoSeq after Undo", f, &stateSummary{false, false, true, false, ""}) @@ -535,7 +580,9 @@ func TestFileNameSettingWithScratch(t *testing.T) { t.Errorf("TestFileNameSettingWithScratch failed to init isscratch. got %v want %v", got, want) } - f.Undo(true) + f.checkedUndo(true, t, undoexpectation{ + ok: false, + }) if got, want := f.Name(), "/guide"; got != want { t.Errorf("TestFileNameSettingWithScratch failed to init name. got %v want %v", got, want) @@ -544,7 +591,9 @@ func TestFileNameSettingWithScratch(t *testing.T) { t.Errorf("TestFileNameSettingWithScratch failed to init isscratch. got %v want %v", got, want) } - f.Undo(true) + f.checkedUndo(true, t, undoexpectation{ + ok: false, + }) if got, want := f.Name(), "edwood"; got != want { t.Errorf("TestFileNameSettingWithScratch failed to init name. got %v want %v", got, want) } @@ -617,12 +666,20 @@ func TestTagObserversFireCorrectly(t *testing.T) { t.Errorf("got %+v, want %+v", got, want) } - oeb.Undo(true) + oeb.checkedUndo(true, t, undoexpectation{ + ok: true, + q0: 0, + q1: 0, + }) if got, want := *counts, (observercount{0, 5}); got != want { t.Errorf("got %+v, want %+v", got, want) } - oeb.Undo(false) + oeb.checkedUndo(false, t, undoexpectation{ + ok: true, + q0: 0, + q1: 2, + }) if got, want := *counts, (observercount{0, 6}); got != want { t.Errorf("got %+v, want %+v", got, want) } diff --git a/file/helpers_test.go b/file/helpers_test.go index 6b784a13..f271bc18 100644 --- a/file/helpers_test.go +++ b/file/helpers_test.go @@ -145,3 +145,31 @@ func (s *span) String() string { return buffy.String() } + +type undoexpectation struct { + q0 int + q1 int + ok bool +} + +func (e *ObservableEditableBuffer) checkedUndo(isundo bool, t *testing.T, u undoexpectation) { + t.Helper() + + q0, q1, ok := e.Undo(isundo) + + if got, want := ok, u.ok; got != want { + t.Errorf("Undo wrong ok: got %v, want %v", got, want) + } + + if !ok { + // Values of q0, q1 don't matter if ok is false + return + } + + if got, want := q0, u.q0; got != want { + t.Errorf("Undo wrong q0: got %d, want %d", got, want) + } + if got, want := q1, u.q1; got != want { + t.Errorf("Undo wrong q1: got %d, want %d", got, want) + } +} diff --git a/file/observable_editable_buffer.go b/file/observable_editable_buffer.go index f90bda6c..81d42db9 100644 --- a/file/observable_editable_buffer.go +++ b/file/observable_editable_buffer.go @@ -131,7 +131,7 @@ func (e *ObservableEditableBuffer) HasMultipleObservers() bool { // MakeObservableEditableBuffer is a constructor wrapper for NewFile() to abstract File from the main program. func MakeObservableEditableBuffer(filename string, b []rune) *ObservableEditableBuffer { - return _makeObservableEditableBuffer(filename, b, newTypeBuffer) + return _makeObservableEditableBuffer(filename, b, true) } func _makeObservableEditableBuffer(filename string, b []rune, newtype bool) *ObservableEditableBuffer { diff --git a/file/undo.go b/file/undo.go index 5572f0d6..bb10f88a 100644 --- a/file/undo.go +++ b/file/undo.go @@ -150,6 +150,8 @@ func (b *Buffer) FlattenHistory() { // Insert inserts the data at the given offset in the buffer. An error is return when the // given offset is invalid. func (b *Buffer) Insert(start OffSetTuple, data []byte, nr, seq int) error { + // log.Println("Insert start") + // defer log.Println("Insert end") off := start.b if len(data) == 0 { return nil @@ -173,7 +175,7 @@ func (b *Buffer) Insert(start OffSetTuple, data []byte, nr, seq int) error { return nil } - c := b.newChange(off, start.r, len(data), nr, seq) + c := b.newChange(off, start.r, seq) var pnew *piece if offset == p.len() { // Insert between two existing pieces, hence there is nothing to @@ -294,7 +296,7 @@ func (b *Buffer) Delete(startOff, endOff OffSetTuple, seq int) error { } b.cachedPiece = newStart - c := b.newChange(off, startOff.r, startOff.b-endOff.b, startOff.r-endOff.r, seq) + c := b.newChange(off, startOff.r, seq) c.new = newSpan(newStart, newEnd) c.old = newSpan(start, end) swapSpans(c.old, c.new) @@ -313,7 +315,7 @@ func (b *Buffer) newAction(seq int) *action { // newChange is associated with the current action or a newly allocated one if // none exists. -func (b *Buffer) newChange(off, roff, nb, nr, seq int) *change { +func (b *Buffer) newChange(off, roff, seq int) *change { a := b.currentAction if a == nil { a = b.newAction(seq) @@ -323,8 +325,6 @@ func (b *Buffer) newChange(off, roff, nb, nr, seq int) *change { c := &change{ off: off, roff: roff, - nb: nb, - nr: nr, } a.changes = append(a.changes, c) return c @@ -379,57 +379,75 @@ func (b *Buffer) findPiece(off OffSetTuple) (*piece, int, int) { // TODO(rjk): Rationalize the returned values. I'm not sure that they're // useful for correct. func (b *Buffer) Undo(_ int) (int, int, bool, int) { + // log.Println("Undo start") + // defer log.Println("Undo end") + b.validateInvariant() b.SetUndoPoint() a := b.unshiftAction() if a == nil { return -1, 0, false, 0 } + // TODO(rjk): This is wrong if a filename change and edits are part of + // the same action? if a.kind == sam.Filename { return b.filenameChangeAction(a) } - var off, size int + var roff, nr int for i := len(a.changes) - 1; i >= 0; i-- { c := a.changes[i] swapSpans(c.new, c.old) - // TODO(rjk): c.off is in runes? I think it's in bytes. So this is non-sensical? - // off = b.RuneTuple(c.off).b - off = c.off - size = c.old.len - c.new.len + roff = c.roff // Must happen after the swapSpans. - b.undone(c, true) + nr = b.undone(c, true) } if b.head == 0 { - return off, size, false, 0 + return roff, roff - nr, true, 0 } - return off, size, false, b.actions[b.head-1].seq + b.validateInvariant() + // TODO(rjk): Conceivably, I need better tests for the return values. + return roff, roff - nr, true, b.actions[b.head-1].seq } -// undone is an Undo helper to implement Edwood specific semantics. +// undone is an Undo helper to implement Edwood specific callback +// semantics. It returns the number of affected runes. There are 4 cases: +// undo insertion, undo deletion, redo insertion, redo deletion: +// +// - undo-insertion: dispatch a deletion operation, return 0 +// - undo-deletion: dispatch an insert, return size of inserted text +// - redo-insertion: dispatch an insert, return size of inserted +// - redo-deletion: dispatch a deletion, return 0 +// // TODO(rjk): I want a zero-copy API all the way into frame. -func (b *Buffer) undone(c *change, undo bool) { - if b.oeb == nil { - return - } - +func (b *Buffer) undone(c *change, undo bool) int { var size, rsize int + newnb, newnr := c.new.nbr() + oldnb, oldnr := c.old.nbr() + if undo { - size = c.nb - rsize = c.nr + size = newnb - oldnb + rsize = newnr - oldnr } else { // Redo case. - size = -c.nb - rsize = -c.nr + size = oldnb - newnb + rsize = oldnr - newnr + } + + // log.Println("undone", undo, size, rsize) + if b.oeb == nil { + // TODO(rjk): Exiting here provides visibility into the computed size. + return rsize } off := c.off // in bytes. The original location where a change started. if size > 0 { // TODO(rjk): API should be in terms of OffsetTuple and eventually bytes. b.oeb.deleted(c.roff, c.roff+rsize) + rsize = 0 } else { // size is smaller. So we're undoing a deletion buffy := make([]byte, -size) @@ -440,8 +458,10 @@ func (b *Buffer) undone(c *change, undo bool) { // TODO(rjk): ick. Pass byte buffers around. Or references. Or something. rb := []rune(string(buffy)) + // log.Println("undone", undo, c.roff, len(rb), string(buffy)) b.oeb.inserted(c.roff, rb) } + return rsize } func (b *Buffer) unshiftAction() *action { @@ -465,6 +485,9 @@ func (b *Buffer) filenameChangeAction(a *action) (int, int, bool, int) { // the change added at off. If there is no action to redo, Redo returns -1 // as the offset. func (b *Buffer) Redo(_ int) (int, int, bool, int) { + // log.Println("Redo start") + // defer log.Println("Redo end") + b.validateInvariant() b.SetUndoPoint() a := b.shiftAction() if a == nil { @@ -475,18 +498,18 @@ func (b *Buffer) Redo(_ int) (int, int, bool, int) { return b.filenameChangeAction(a) } - var off, size int - + var roff, nr int for _, c := range a.changes { swapSpans(c.old, c.new) - off = c.off - size = c.new.len - c.old.len + roff = c.roff // Must happen after swapSpans - b.undone(c, false) + nr = b.undone(c, false) } - return off, size, false, a.seq + // log.Println("redo", roff, roff+nr, true, len(a.changes)) + b.validateInvariant() + return roff, roff - nr, true, b.actions[b.head-1].seq } // RedoSeq finds the seq of the last redo record. TODO(rjk): This has no @@ -618,13 +641,17 @@ type change struct { new span // all pieces which are introduced/swapped in by the change off int // absolute offset at which the change occurred roff int // absolute offset in runes at which the change occurred. - nb int // number of bytes in this change, negative on deletion. - nr int // number of runes in this change, negative on deletion. + + // TODO(rjk): These don't get updated. + // It's length of new - length of old? + // nb int // number of bytes in this change, negative on deletion. + // nr int // number of runes in this change, negative on deletion. } // span holds a certain range of pieces. Changes to the document are // always performed by swapping out an existing span with a new one. len -// is required to control swapSpans operation. +// is required to control swapSpans operation. len might not be updated +// when a cached piece is modified. type span struct { start, end *piece // start/end of the span len int // the sum of the lengths of the pieces which form this span. @@ -633,7 +660,7 @@ type span struct { func newSpan(start, end *piece) span { s := span{start: start, end: end} for p := start; p != nil; p = p.next { - s.len += (p.len()) + s.len += p.len() if p == end { break } @@ -641,6 +668,20 @@ func newSpan(start, end *piece) span { return s } +// nbr returns the number of bytes and runes in a span. span.len is not +// updated for cached piece modification. nbr returns up to date values. +func (s span) nbr() (int, int) { + nr, nb := 0, 0 + for p := s.start; p != nil; p = p.next { + nb += p.len() + nr += p.nr + if p == s.end { + break + } + } + return nb, nr +} + // swapSpans swaps out an old span and replace it with a new one. // - If old is an empty span do not remove anything, just insert the new one. // - If new is an empty span do not insert anything, just remove the old one. diff --git a/file/undo_test.go b/file/undo_test.go index f71c3242..51de8017 100644 --- a/file/undo_test.go +++ b/file/undo_test.go @@ -277,17 +277,17 @@ func TestUndoRedoReturnedOffsets(t *testing.T) { }{ 0: {redo, -1, 0}, 1: {undo, 0, 20}, - 2: {undo, 3, -19}, - 3: {undo, 8, 8}, - 4: {undo, 12, -9}, - 5: {undo, 7, -5}, + 2: {undo, 3, -16}, + 3: {undo, 8, 16}, + 4: {undo, 12, 3}, + 5: {undo, 7, 2}, 6: {undo, 0, -7}, 7: {undo, -1, 0}, 8: {redo, 0, 7}, - 9: {redo, 7, 5}, - 10: {redo, 12, 9}, - 11: {redo, 8, -8}, - 12: {redo, 3, 19}, + 9: {redo, 7, 12}, + 10: {redo, 12, 21}, + 11: {redo, 8, 0}, + 12: {redo, 3, 22}, 13: {redo, 0, -20}, 14: {redo, -1, 0}, } diff --git a/guide b/guide index e150d7f2..d9c9f40f 100644 --- a/guide +++ b/guide @@ -3,13 +3,12 @@ Edit X:edwood/\+Errors: 1,$d X:edwood/.*\.go: w go build -tags debug -./testedwood.sh --debug=localhost:8000 +./testedwood.sh --newtypebuffer go test --run 'TestLargeEditTargets' -covermode=count -coverprofile=count.out go test --run XXX -bench 'BenchmarkLargeEditTargets' -cpuprofile cpu.prof -go test +go test -covermode=count -coverprofile=count.out go tool cover -html=count.out -go test // Edit commands for converting C to Go