Implementation discussion for better "text buffers"

[rjkroege]

Some discussion about the approach for fixing #97. (@camsn0w: this is a follow up from our chat yesterday.)

Currently Edwood uses File to store an editable text buffer with undo and has-a (completely internal) RuneArray. Per @mibk's suggestion in #97, the undo package would seem reasonable. (Though it may require some modifications)

I'll call undo undo.Buffer to (maybe) be clearer.

File combines:

• buffer mutation via the has-a `RuneArray
• Undo functionality (implemented inside File)
• Text observer functionality (i.e. each Text instances observe the File that it displays) for broadcasting changes to a File to all of the interested (currently Text) instances

undo.Buffer combines:

• buffer mutation
• Undo functionality

but (obviously) no Text observer functionality.

So: at a high-level, getting Edwood to use buffer.Undo entails splitting File apart into:

• buffer mutation and Undo functionality. Maybe call this File until it can be replaced with undo.Buffer
• Text observer functionality. Maybe call this ObservableEditableBuffer (anticipating undo.Buffer)

ObservableEditableBuffer would apply edits / undos to its has-a File or undo.Buffer and notify the implementations of the (new) BufferObserver interface of these edits. Text already largely implements the forthcoming BufferObserver so it's mostly a matter of creating the interface .

Once this is split up like this, we can replace the buffer mutation and Undo functionality with undo.Buffer.

[rjkroege]

I'm thinking that BufferObserver would look like this:

type BufferObserver interface {
  // Would invoke Text.inserted.
  Inserted(p0 int, s []rune)

  // Would invoke Text.deleted.
  Deleted(p0, p1 int)
}

To move all of this code into a separate module, the File.AllText would operate over BufferObserver implementations. The functions passed into File.AllText would need to do a cast of the BufferObserver to a *Text.

[camsn0w]

Do you have any ideas for what should be done with Elog when migrating File to it's own package?

[paul-lalonde]

Is it worth thinking of multiple insertions and deletions? Multiple observations will likely lead to multiple redraws which would (um, do?) suck. Paul

…

On Thu, Jun 10, 2021, 10:03 AM Robert Kroeger ***@***.***> wrote: I'm thinking that BufferObserver would look like this: type BufferObserver interface { // Would invoke Text.inserted. Inserted(p0 int, s []rune) // Would invoke Text.deleted. Deleted(p0, p1 int) } To move all of this code into a separate module, the File.AllText would operate over BufferObserver implementations. The functions passed into File.AllText would need to do a cast of the BufferObserver to a *Text. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ6RUF6GLUQERPEBYVGRPLTSDV6FANCNFSM46O3ABQQ> .

[camsn0w]

I'll have to keep that in mind going forward. Paul, if you'd like to see how it's going I have a new pull request up by the way.

[rjkroege]

I'd like to keep the observing interface fine-grain with an invocation per mutation. Instead, what's suppose to happen is that

• all of the observing Text instances receive each mutation change
• if the change is inside the viewport, invoke Frame to update. Frame writes the necessary commands to the Display buffer
• once per user interaction, we call display.Flush() to push the ops to the screen.

This can be improved. There are bugs with this: I've noticed that Edwood sometimes leaves itself un-redrawn under certain circumstances.

[paul-lalonde]

That was an excellent nudge.

…

On Mon, Jun 14, 2021, 7:45 PM camsn0w ***@***.***> wrote: I'll have to keep that in mind going forward. Paul, if you'd like to see how it's going I have a new pull request up by the way. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#334 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ6RUCPQI23YJVPCDDD273TS25FDANCNFSM46O3ABQQ> .

[camsn0w]

For things like warn, and warnerror and elog, should separate them from the package in order to use them in file?

[paul-lalonde]

If a mutation is a single change of the buffer then this is likely to perform poorly for large search-and-replace operations. But I'm fine dealing with that if and when that day comes. Paul

…

On Fri, Jun 18, 2021 at 3:17 PM Robert Kroeger ***@***.***> wrote: I'd like to keep the observing interface fine-grain with an invocation per mutation. Instead, what's suppose to happen is that - all of the observing Text instances receive each mutation change - if the change is inside the viewport, invoke Frame to update. Frame writes the necessary commands to the Display buffer - once per user interaction, we call display.Flush() to push the ops to the screen. This can be improved. There are bugs with this: I've noticed that Edwood sometimes leaves itself un-redrawn under certain circumstances. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#334 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ6RUF5Y7VT6PREZO2PUNLTTPAWFANCNFSM46O3ABQQ> .

[camsn0w]

Just a bit of an update and explanation on my current class implementation;
text: has an observer that it calls various callback functions on
observableeditablebuffer: keeps track of the current observers and calls functions on text through the bufferobserver interface text implements
file: contains undo functionality and the struct that the observableeditablebuffer operates on

[rjkroege]

Additional thoughts:

• how to handle Elog
• what to do about warnings?

I think (albeit needs more research) that Elog could just be another observer. I'm not sure how to handle warnings. I will persue the code and update this sub-thread.

[camsn0w]

Also, how / where would I initialize elog if I am putting it in as an interface in file?

[rjkroege]

I was confused about Elog. It's state that's per File. It doesn't have to be in File proper. It needs to be moved to ObservableEditableBuffer. I am increasingly convinced that ObservableEditableBuffer should own File (and later undo.Buffer) instead of the other way around.

Also: I don't remember the issue that we discussed about warnings.

[camsn0w]

I agree with you that OEB should own file and I'll update that after making the changes specified in your comments. The issue we discussed with warnings was how to deal with the warn and warnerr functions once file is moved outside of main.

[camsn0w]

Just finished making OEB have a File. Also, I noticed that if I replaced warnError with just a fmt.Errof with the same text, it passes the tests, so wrapping it on the reciever side looks promising.

[rjkroege]

Newtype buffers are now the default. Time to take advantage of them. General order:

• less []rune to []byte conversions and copies
• cache OffsetTuple addresses and don't run file.RuneTuple() when possible.