gvwilson / sdxpy

Software Design by Example: a tool-based introduction with Python
https://third-bit.com/sdxpy/
Other
353 stars 53 forks source link

/undo - several suggestions #222

Closed gvwilson closed 1 year ago

gvwilson commented 1 year ago

cc @davewsmith

davewsmith commented 1 year ago

Two passes done. Reflecting on how many of my notes make sense here (vs. writing a treatise on the fun corner-cases of undo/redo and the several ways one might model it).

gvwilson commented 1 year ago

Thanks very much - I'm trying hard to keep each chapter teachable in a one-hour lecture (with a little bit spilling into src/bonus if necessary), if that helps with scoping.

davewsmith commented 1 year ago

In 24.2, it's a shame that string.printable includes characters you're not ready to deal with.

Making movements undoable is an interesting choice that might run counter to people's experience with their current editor. I'm trying to think of an editor that behaves that way, and am drawing a blank. If the motivation is for the later exercise of combining motions, perhaps an alternate exercise of combining a sequence of inserted characters into one undoable block may make sense.

In class Move, self._new is written but never read. An artifact of some prior design?

In Undo.do, unless the reader has taken the hint and internalized change to _interact for save, action = self._app._history.pop() can be read as popping the Undo action itself, rather than the action underneath it. This might be a source of confusion to readers who skip back and forth between the code fragments. Showing a revised implementation of _interact would cure that.

An alternative to save is to leverage the fact of actions having a reference to app. The do-ing of an unsaveable action then has the action finishing to ask the app to pop the action off the history. This saves some conditional logic at the expense of adding a public method to the App for the Action to use. (In a real editor, the command history is its own object.)

In 24.4, the concept map includes a forward reference to redo. Intentional?

Implementing Redo is an interesting design challenge. How does redo differ from undoing an undo? Does it make sense to redo an action that wasn't done? Along one design path, Redo is just an Undo of a previously undone Action (which requires Actions to record which of (done, undo) was last invoked.

Possible exercise: Newline. What kind of Action is it? It's different from other character inserts given the that _display holds an array that the newline will cause a split in.

gvwilson commented 1 year ago

thanks @davewsmith - I'll try to get these changes in this week.