Undo pieces have nr

[camsn0w]

Each piece now records it's Nr (either found or given with insert) to identify if the given piece has non-ascii characters or not. Dependent on #368

[codecov[bot]]

Codecov Report

Merging #369 (6044fb2) into master (a9b2934) will decrease coverage by 2.88%. The diff coverage is 70.58%.

:exclamation: Current head 6044fb2 differs from pull request most recent head a1d474d. Consider uploading reports for the commit a1d474d to get more accurate results

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
- Coverage   55.62%   52.74%   -2.89%     
==========================================
  Files          55       54       -1     
  Lines       10678    10781     +103     
==========================================
- Hits         5940     5686     -254     
- Misses       4297     4655     +358     
+ Partials      441      440       -1     

Impacted Files	Coverage Δ
file/undo.go	83.62% <69.69%> (-9.04%)	:arrow_down:
file/bytes.go	87.96% <100.00%> (ø)
frame/frameopts.go	0.00% <0.00%> (-92.31%)	:arrow_down:
frame/tick.go	0.00% <0.00%> (-56.53%)	:arrow_down:
frame/frame.go	0.00% <0.00%> (-35.72%)	:arrow_down:
frame/ptofchar.go	47.94% <0.00%> (-30.14%)	:arrow_down:
frame/insert.go	16.74% <0.00%> (-26.44%)	:arrow_down:
file/observable_editable_buffer.go	58.38% <0.00%> (-14.64%)	:arrow_down:
frame/box.go	54.86% <0.00%> (-14.16%)	:arrow_down:
frame/draw.go	7.74% <0.00%> (-10.57%)	:arrow_down:
... and 28 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a9b2934...a1d474d. Read the comment docs.

[camsn0w]

file/undo.go:1 we should rename undo.go to buffer.go at some point. Not in this PR.

Sounds good

file/undo.go:106 You shouldn't need to add mod or treatasclean here. New oeb.SaveableAndDirty can be implemented in terms of Buffer.Dirty(). Note that means that file/undo.go:576 can be removed.

Ok

General observation: try to keep file.Buffer's semantics as unchanged as possible. The forwarding functions should adapt file.Buffer to OEB's semantic guarantees outside the file package.

Ok

file/undo.go:110 Hm. I'm not sure about this.

Will delete it now that I know it doesn’t work.

file/undo.go:137 Move the test only code into the _test.go file.

Will do

file/undo.go:143 Document the nr argument here and elsewhere.

Will do

file/undo.go:171 off is a byte index. So: we would have had to (perhaps implicitly) compute beforeNr before calling Buffer.Insert. So you can update the split pieces's rune count without using utf8.RuneCount. At a minimum: you should only count the rune contents of the smaller slice.

Makes sense

file/undo.go:187 Test only? If so, move to undo_test.go I anticipate it will be used elsewhere eventually, right now it’s just for tests though file/undo.go:251 Same issue applies as with Insert. Having computed off and length, we'll already have computed nr implicitly.

Makes sense

General comment: it's essential to note which arguments index in runes while which index in Bytes.

I wholeheartedly agree, the ambiguity gets confusing 

nr is an int but all the original file.Buffer position types are int64. This is a nasty bug waiting to happen on 32bit systems. Rune counts and indicies need also to be int64

Sounds good

Idea: wrap the file.Buffer with rune-oriented implementation. Introduce a new type for all positional and length arguments. Something ike this:

I like the idea, Sounds good

type OffsetTuple struct { b int64 r int64 } and use them in file.Buffer Add the interface-satisfying construct for file.Buffer: it currently satisfies io.ReaderAt.

Which interface-satisfying construct? OffsetTuple?

In a different PR: make file.Buffer also implement io.RuneReader and remove ReadC. (And RuneReaderReverse and Seeker) and some more.

Etc. Ok file/undo.go:341 Please explain how ChangeInfo is meant to be used. I don't understand how you can use construct the correct observer calls in OEB from the contents of a ChangeInfo.

Following more tests I discovered it doesn’t work, thanks for pointing this out

file/undo.go:532 delete mutates the piece backing and so must update the nr value.

Thanks for catching that, that may be why my implementation doesn’t work

file/undo.go:541 why? You can implement this externally in OEB?

It was useful for both undo_test and OEB as well as implementing various interfaces.

file/undo.go:550 Why? The cachedPiece should be an internal detail of file.Buffer. Also: no concept of commit needs to exist. Let file.Buffer handle this.

text_test.go needs a GetCache

Most of these functions from 550 down seem unnecessary. We can discuss further.

They were put there to either maintain compatibility with tests or to help keep track of indexes between bytes and runes (poorly as I’ve recently discovered)

The file.Buffer action will probably need to track seq yes?

I thought we are getting rid of seq?

file/undo.go:182 The diff would have been smaller if you'd added new functions for making a buffer with a nr value. No code yet uses these entry points so they can (and should perhaps) have new names.

My bad, I’m remembering you mentioned both

file/undo_test.go:11 Needs additional cases where there are non-ASCII runes at all the boundary points.

I partially wrote the tests and discovered some issues at these edge cases, I will remove the extra junk from this PR and them up.

[rjkroege]

I landed this with some changes per email discussion. Thanks so much for the work.