Having trouble writing a simple grouped undo/redo

[Brian Theado]

I wrote some code to perform 2 node inserts. I added before/after undo commands and undo/redo of the individual operations seemed to work fine. But when I enclose the two operations in before/after changeGroup, then redo doesn't work.

Does anyone know what I'm doing wrong?

To duplicate the issue:

1. copy below xml to clipboard
2. in a new leo outline ctrl-shift-c to paste as nodes
3. highlight the "grouped insert redo test" node
4. ctrl-b to execute it...two new headlines appear as expected
5. ctrl-z to undo it...the two headlines disappear as expected
6. ctrl-shift-z to redo it. The redo doesn't happen and the console shows this message:

redoGroup oops: expecting bunch.items.  bunch.kind = insert

redoGroup g.Bunch()

    dirtyVnodeList: []

    kind: insert

    newBack: <pos 139670364254672 childIndex: 0 lvl: 0 key: 139670742747232:0 NewHeadline>

    newChanged: True

    newDirty: True

    newMarked: False

    newP: <pos 139670364258088 childIndex: 1 lvl: 0 key: 139670364257360:1 test redo>

    newParent: <pos 139670364257808 [0] None>

    oldChanged: False

    oldDirty: False

    oldMarked: False

    oldSel: (0, 0)

    p: <pos 139670364286200 childIndex: 1 lvl: 0 key: 139670364257360:1 test redo>

    pasteAsClone: False

    redoHelper: <bound method Undoer.redoInsertNode of <leo.core.leoUndo.Undoer object at 0x7f07a018dfd0>>

    undoHelper: <bound method Undoer.undoInsertNode of <leo.core.leoUndo.Undoer object at 0x7f07a018dfd0>>

    undoType: Paste Node

Here is the xml:

<?xml version="1.0" encoding="utf-8"?>
<!-- Created by Leo: http://leoeditor.com/leo_toc.html -->
<leo_file xmlns:leo="http://leoeditor.com/namespaces/leo-python-editor/1.1" >
<leo_header file_format="2"/>
<vnodes>
<v t="bpt.20190818085349.1"><vh>test redo</vh>
<v t="bpt.20190818084547.1"><vh>grouped insert redo test</vh></v>
</v>
</vnodes>
<tnodes>
<t tx="bpt.20190818084547.1">@language python

undoType = 'insert first child twice'; u = c.undoer
parent = p.parent()
u.beforeChangeGroup(p, undoType)
undoData = u.beforeInsertNode(p)
child = parent.insertAsNthChild(0)
u.afterInsertNode(child, undoType, undoData)
undoData = u.beforeInsertNode(p)
child = parent.insertAsNthChild(0)
u.afterInsertNode(child, undoType, undoData)
u.afterChangeGroup(p, undoType)
c.redraw(child)
</t>
<t tx="bpt.20190818085349.1"></t>
</tnodes>
</leo_file>

[Edward K. Ream]

On Sun, Aug 18, 2019 at 8:12 AM Brian Theado <brian....@gmail.com> wrote:

I wrote some code to perform 2 node inserts. I added before/after undo commands and undo/redo of the individual operations seemed to work fine. But when I enclose the two operations in before/after changeGroup, then redo doesn't work.

I'll take a look asap.  It may be a few hours, though.

Edward

[Edward K. Ream]

On Sun, Aug 18, 2019 at 8:15 AM Edward K. Ream <edre...@gmail.com> wrote:

I'll take a look asap.

I see the behavior you describe.  It persists if I replace p by parent in the before/after calls.

Edward

[Edward K. Ream]

On Sun, Aug 18, 2019 at 8:24 AM Edward K. Ream <edre...@gmail.com> wrote:

At least one problem is that p changes after:

    child = parent.insertAsNthChild(0)

Edward

[Edward K. Ream]

This works:

groupType = 'insert child twice'

undoType = 'insert first child twice'; u = c.undoer

# Start group.
parent = p.parent()
u.beforeChangeGroup(parent, groupType)
# 1
undoData = u.beforeInsertNode(parent)

child = parent.insertAsNthChild(0)
u.afterInsertNode(child, undoType, undoData)

# 2
undoData = u.beforeInsertNode(parent)

child = parent.insertAsNthChild(0)
u.afterInsertNode(child, undoType, undoData)

# End group.
u.afterChangeGroup(parent, groupType)
c.redraw(child)

This fails if p.parent() does not exist, but that's another matter.

Also note the two different undo types.  That's probably not necessary, but it's better style.

Edward

[Brian Theado]

Thanks a lot. I'll give it a try tonight and also see if I can get similar code working for clone and move. After that I can try to put it all together. 

--
You received this message because you are subscribed to the Google Groups "leo-editor" group.
To unsubscribe from this group and stop receiving emails from it, send an email to leo-editor+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/CAMF8tS1RaaS8qumebOtNLaT%3DLVpx_kajWcTjjQUGfnV8LuW3Ng%40mail.gmail.com.

[Edward K. Ream]

On Sun, Aug 18, 2019 at 11:35 AM Brian Theado <brian....@gmail.com> wrote:

Thanks a lot. I'll give it a try tonight and also see if I can get similar code working for clone and move. After that I can try to put it all together. 

You're welcome.  Good luck, and feel free to ask more questions.

Edward

[Brian Theado]

Hmm, it didn't work for me. Using your code and following the same steps as my original email, I get the same error message.

After that if I start from step #4 (the ctrl-b step) again, then the undo and the redo work. But then if I start again at step #4, I get the original error message again.

Could you confirm it is working for you even starting from scratch?

--

[Brian Theado]

I think the issue is in u.redoGroup (leo/core/LeoPyRef.leo#Code-->Core classes-->@file leoUndo.py-->class Undoer-->u.redo helpers-->u.redoGroup)

The line:

    bunch = u.beads[u.bead]; count = 0

Should be:

    bunch = u.beads[u.bead+1]; count = 0

That line is in both u.undoGroup and u.redoGroup. For undoGroup u.bead is valid, but for redo it should be operating with u.bead+1.

Brian

[Brian Theado]

I found a core function which uses undo groups and used this code to call it:

c.createNodeHierarchy(['a', 'b', 'c'], parent=p)
c.redraw(p)

Redoing this operation gives the same error as the test code and the bead+1 fixes it also. I think this bug is affecting all grouped redo.

[Edward K. Ream]

On Mon, Aug 19, 2019 at 7:04 PM Brian Theado <brian....@gmail.com> wrote:

I found a core function which uses undo groups and used this code to call it:

c.createNodeHierarchy(['a', 'b', 'c'], parent=p)
c.redraw(p)

Redoing this operation gives the same error as the test code and the bead+1 fixes it also. I think this bug is affecting all grouped redo.

Thanks for this work.  Does the proposed change pass all unit tests?

Edward

[Brian Theado]

Unfortunately, I'm not able to run unit tests without getting a segfault. It appears to be related to loading the readline library similar to what I reported last year: https://groups.google.com/d/msg/leo-editor/ghiIN7irzY0/2wreFgcYDQAJ. Though now I'm running Ubuntu 18.04. Something in the unit test code causes pdb to be loaded which loads readline and causes the crash.

I faced the same issue when trying to set breakpoints in code...invoking pdb crashed. I solved that by launching leo with 'python -m pdb'. Somehow when doing it that way there was no crash. However, the same workaround didn't work for running the unit tests. I have some ideas on tracking it down further, but I haven't gotten to it yet.

--

You received this message because you are subscribed to the Google Groups "leo-editor" group.
To unsubscribe from this group and stop receiving emails from it, send an email to leo-editor+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/CAMF8tS20sv7uEzmpFH8Sf65h5JRDF_kX_rvDyggbqK8%2BhJKi5g%40mail.gmail.com.

[Edward K. Ream]

On Tue, Aug 20, 2019 at 6:22 AM Brian Theado <brian....@gmail.com> wrote:

Unfortunately, I'm not able to run unit tests without getting a segfault.

Alright.  Create a new branch, and I'll run the unit tests there.

Edward

[Brian Theado]

Sure, I'll do that or figure out the segfault issue sometime this week.

[Brian Theado]

It turns out the '-m pdb' workaround for the readline crash does work for me running unit tests.

I decided I wanted to follow the good practice of adding a failing unit test which exposes the bug and then show after the fix that same test passes. I discovered there are already unit tests for other group undo commands like convertAllBlanks and convertAllTabs and I wondered how those tests were passing. It turns out all the undo/redo tests start with a fresh undo stack. The off-by-one error of this bug causes the bead index to be -1 instead of 0. But for a single element list array, indices -1 and 0 point to the same list element. So the group redo will always function properly when it is the only item on the list.

In order to duplicate the bug, I will need to write a test containing multiple commands. I might be out of time for this for a few days, but I plan to tackle it when I get a chance.

[Edward K. Ream]

On Sat, Aug 24, 2019 at 7:56 PM Brian Theado <brian....@gmail.com> wrote:

It turns out the '-m pdb' workaround for the readline crash does work for me running unit tests.

Two tips.  You can add a call to g.pdb() at the start of any unit test.  Also, I run unitTest.leo with the --fail-fast command-line option.

I decided I wanted to follow the good practice of adding a failing unit test which exposes the bug and then show after the fix that same test passes. I discovered there are already unit tests for other group undo commands like convertAllBlanks and convertAllTabs and I wondered how those tests were passing. It turns out all the undo/redo tests start with a fresh undo stack. The off-by-one error of this bug causes the bead index to be -1 instead of 0. But for a single element list array, indices -1 and 0 point to the same list element. So the group redo will always function properly when it is the only item on the list.

Excellent work.

Btw, rev e4d720b appears to fix #460 with a trivial change.

Edward

[Brian Theado]

I submitted pull request https://github.com/leo-editor/leo-editor/pull/1302. No new failed test cases and the failing test case I added passes.

--

You received this message because you are subscribed to the Google Groups "leo-editor" group.
To unsubscribe from this group and stop receiving emails from it, send an email to leo-editor+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/CAMF8tS1A0Raz9w4TTMmstTG%2BPeRU7hUnbSyFWhsPUszPZubAqQ%40mail.gmail.com.

[Edward K. Ream]

On Sun, Aug 25, 2019 at 3:36 PM Brian Theado <brian....@gmail.com> wrote:

I submitted pull request https://github.com/leo-editor/leo-editor/pull/1302. No new failed test cases and the failing test case I added passes.

Thanks for the new unit test.  I merged the code and reran all unit tests in devel.  All seems well.

Edward