Invalid position error on redo position move

[Brian Theado]

In the code I've been writing, there is one particular case which gives an "invalid position" error when I redo it. I've written some simplified code which replicates the scenario (even removing the grouped undo allows the issue to duplicate).

The code performs 3 individually undoable operations: insert, clone, then move. The error comes after undoing the move and then redoing that same move.

Just sharing in case someone sees something obviously wrong. Otherwise I'll be digging deeper sometime this week.

Procedure:

1. Copy the below xml
2. open a new leo outline
3. Ctrl-shift-v to paste the nodes
4. Ctrl-b to execute

Error looks like this:

setCurrentPosition Invalid position: 'newHeadline', root: 'NewHeadline' select,selectHelper,select_new_node,set_body_text_after_select
setCurrentPosition Invalid position: 'newHeadline', root: 'NewHeadline' new_cmd_wrapper,redo,redoMove,selectPosition

<?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.20190820213410.1"><vh>redo bug (use ctrl-b)</vh>
<v t="bpt.20190820213730.1"><vh>one</vh></v>
<v t="bpt.20190820213735.1"><vh>two</vh>
<v t="bpt.20190820213740.1"><vh>three</vh></v>
</v>
</v>
</vnodes>
<tnodes>
<t tx="bpt.20190820213410.1">@language python

# Code written to assume this descendent structure
# one
# two
#   three
parent = p; u = c.undoer

# Will the issue still reproduce without the insert?
if 1:
    undoData = u.beforeInsertNode(parent)
    child =parent.insertAsNthChild(0)
    u.afterInsertNode(child, "insert as first child", undoData)
    sibling =  child.copy().moveToNext().moveToNext()
else:
    # Nope. The issue doesn't duplicate with this
    child = parent.copy().moveToFirstChild()
    sibling = child.copy().moveToNext()

undoData = u.beforeCloneNode(child)
clone = child.clone()
u.afterCloneNode(clone, "clone node", undoData)

undoData = u.beforeMoveNode(clone)
clone.moveToFirstChildOf(sibling)
u.afterMoveNode(clone, "move node to top", undoData)

# Maybe the next step is to set a breakpoint in the "invalid position"
# and trace it from there.
c.executeMinibufferCommand('undo')
c.executeMinibufferCommand('redo')</t>
<t tx="bpt.20190820213730.1"></t>
<t tx="bpt.20190820213735.1"></t>
<t tx="bpt.20190820213740.1"></t>
</tnodes>
</leo_file>

[vitalije]

In the first case after `if 1:`

you have called two times child.copy().moveToNext().moveToNext(), while in the else case you have called it only once.

I have tried with one call in both cases and it works without error.

Vitalije

[Brian Theado]

Thanks, vitalije. The else part of the 'if 1:' was my failed attempt to further simplify the failing case.

The error is happening when the code does: insert, clone, move the clone to the first child of node="two" (which already has one child named "three"), undo, redo.

I though I might also duplicate it by skipping the insert and just cloning the already existing node="one". But still I want to move the clone to the first child of "two". Since the insert was skipped, one fewer moveToNext() was needed in order to get the to new parent node="two".

The error given by the code as currently written is what I need to solve. You can ignore the else clause it is just noise :-).

Actually, inspired by your response, I found a way to duplicate the error even without the insert. So now it is just clone, move, undo, redo. Here is the new subtree 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.20190821091623.1"><vh>redo bug (use ctrl-b)</vh>
<v t="bpt.20190821091623.2"><vh>one</vh></v>
<v t="bpt.20190821091623.3"><vh>two</vh></v>
<v t="bpt.20190821091623.4"><vh>three</vh>
<v t="bpt.20190821091623.5"><vh>four</vh></v>
</v>
</v>
</vnodes>
<tnodes>
<t tx="bpt.20190821091623.1">@language python

# Code written to assume this descendent structure
# one
# two
# three

#   four

parent = p; u = c.undoer

one = parent.copy().moveToFirstChild()
three = one.copy().moveToNext().moveToNext()

undoData = u.beforeCloneNode(one)
one_clone = one.clone()
u.afterCloneNode(one_clone, "clone node", undoData)

undoData = u.beforeMoveNode(one_clone)
one_clone.moveToFirstChildOf(three)
u.afterMoveNode(one_clone, "move node to top", undoData)

# Maybe the next step is to set a breakpoint in the "invalid position"
# and trace it from there.
c.executeMinibufferCommand('undo')
c.executeMinibufferCommand('redo')</t>

<t tx="bpt.20190821091623.2"></t>
<t tx="bpt.20190821091623.3"></t>
<t tx="bpt.20190821091623.4"></t>
<t tx="bpt.20190821091623.5"></t>
</tnodes>
</leo_file>

--
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/fced3afd-898a-493c-8b31-c3e5dbff7adb%40googlegroups.com.

[vitalije]

Well after `one_clone = one.clone()`, position `three` becomes invalid, because `one.clone()` inserts cloned node above the node `three`. If you add:

three._childIndex += 1

after `one_clone = one.clone()`, there will be no error after executing script.

If I were you I would make all tree changes using v-nodes, and for making things undoable I would create my own undoHelper and redoHelper. Here is for example one of your scripts that we have discussed before:

import time
import leo.core.leoNodes as leoNodes
def findFtlistAncestor(p):
    p = p.copy()
    while p and not p.h.startswith('@ftlist'):
        p = p.parent()
    return p


def moveToTopAndCloneToAtDay(c, p):
    """
        Move the node identified by position p to the first child of
        an ancestor @ftlist node and also clone it to a sibling @day
        node with date matching today's date
    """
    ftlist = findFtlistAncestor(p)
    if not ftlist:
        g.es("Not in ftlist tree")
        return {}
    vft = ftlist.v
    before = make_snapshot(vft)
    v = p.v
    if vft in v.parents:
        i = vft.children.index(v)
        del vft.children[i]
        vft.children.insert(0, v)
    else:
        vft.children.insert(0, v)
        v.parents.append(vft)
    today = "@day " +  time.strftime("%Y-%m-%d", time.gmtime())
    for i, ch in enumerate(vft.children):
        if ch.h == today:
            break
    else:
        i = len(vft.children)
        ch = leoNodes.vnode(c)
        ch.h = today
        vft.children.append(ch)
        ch.parents.append(vft)
    if ch in v.parents:
        i = ch.children.index(v)
        del ch.children[i]
        ch.children.insert(0, v)
    else:
        ch.children.insert(0, v)
        v.parents.append(ch)
    return ftlist, before, make_snapshot(vft)


def from_snapshot(data):
    for v, children, parents in data:
        v.children[:] = children
        v.parents[:] = parents


def make_snapshot(v0):
    def it(v):
        yield v, tuple(v.children), tuple(v.parents)
        # if you need to keep track of headlines and boides too
        # yield v, tuple(v.children), tuple(v.parents), v.h, v.b
        for ch in v.children:
            yield from it(ch)
    return tuple(it(v0))


def do_change():
    undo_data = c.undoer.createCommonBunch(p)
    undo_data.kind = 'my-special-operation'
    undo_data.undoType = 'my-special-operation'
    ftlist, before, after = moveToTopAndCloneToAtDay(c, p)
    undo_data.before = before
    undo_data.after = after
    undo_data.ftlist = ftlist
    undo_data.undoHelper = lambda:from_snapshot(undo_data.before) or c.redraw(undo_data.p)
    undo_data.redoHelper = lambda:from_snapshot(undo_data.after) or c.redraw(undo_data.ftlist.firstChild())
    c.undoer.pushBead(undo_data)
    c.redraw(ftlist.firstChild())

do_change()

If you make a script button of this code, you can try your action and its undo/redo functionality on some of the descendants of your @ftlist tree.

HTH Vitalije

[Brian Theado]

Thanks vitalije for noticing the position error. I wonder why it only gave an error on redo and not also for the original operation.

Thanks for the vnode-based code. Based on a quick test, it seems better than my position based code as I haven't seen any errors on undo/redo.

I'm finding the vnode code harder to read, but if it works better then it is better :-). However, is it better because you are the one who wrote it instead of me :-), or because it is easier to avoid mistakes using vnodes? I'll take a close look at it and add the insert functionality I also had in my code. Maybe that way I will if/how out how vnodes help avoid the mistakes.

About this comment:

        # if you need to keep track of headlines and boides too
        # yield v, tuple(v.children), tuple(v.parents), v.h, v.b

Does that only mean if headlines and bodies are changing that the above needs to be done?

--

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/20942315-145c-4dff-83ea-d0b6a1a39afe%40googlegroups.com.

[vitalije]

I'm finding the vnode code harder to read, but if it works better then it is better :-). However, is it better because you are the one who wrote it instead of me :-), or because it is easier to avoid mistakes using vnodes?

Well I didn't tried too hard to make it more readable. It can be improved. But changing tree using vnodes has several benefits. First of all, vnodes are stable (immune to tree changes) - positions are not. The second they work much faster because every change in the tree performed through positions are followed by a redraw and a lot of code that doesn't need to be executed until the complete tree change is done. You don't usually want to see intermediate versions of tree. You want to see the finished tree. When making changes using vnodes, nothing is redrawn until you explicitly call `c.redraw()`.

Iterating through every node is at least 2-3 times faster using vnodes than using position iterators.

 

I'll take a close look at it and add the insert functionality I also had in my code. Maybe that way I will if/how out how vnodes help avoid the mistakes.

About this comment:

        # if you need to keep track of headlines and boides too
        # yield v, tuple(v.children), tuple(v.parents), v.h, v.b

The function make_snapshot creates a snapshot of data needed to recreate subtree structure. In the variant where no changes to headlines nor bodies are made, this function takes enough data to undo the changes. However if you need to keep record of headlines and bodies too, then you should add them too to snapshot. Of course  in that case you must change the function from_snapshot too. 

def from_snapshot(data):
    for v, children, parents, h, b in data:

        v.children[:] = children
        v.parents[:] =

parents
        v.h = h
        v.b = b

def make_snapshot(v0):
    def it(v):

        yield v, tuple(v.children), tuple(v.parents), v.h, v.b
        for ch in v.children:
            yield from it(ch)
    return tuple(it(v0))

This two functions can store and recreate the exact copy of the given vnode. If you want to make snapshot of the whole outline, pass the c.hiddenRootNode as the argument to the make_snapshot function. However, if you are certain that the only changes made are under one vnode (for example @ftlist node), then it is more efficient to store only subtree of @ftlist.

HTH Vitalije

[Brian Theado]

Vitalije,

On Wed, Aug 21, 2019 at 5:41 PM vitalije <vita...@gmail.com> wrote:

Well I didn't tried too hard to make it more readable. It can be improved.

Ok, after looking at it some more, I see that the vnode tree structure involves only a list of children and a list of parents (one parent for each clone). That's pretty straight forward and I added a few comments and it seems plenty readable. I was able to change the code (without breaking it!) so it creates the day node at the second position instead of the last position. That is an important feature for me.

 

But changing tree using vnodes has several benefits. First of all, vnodes are stable (immune to tree changes) - positions are not.

And I think this is what appeals the most to me.  With the position code, I tried to factor out some common code, but it took two positions as input (saved positions issue!) and for one of the cases I was using it, one of the positions got invalidated. I suspect if I try to factor out some of the vnode code in a similar way, it won't be as fragile as that.

 

The second they work much faster because every change in the tree performed through positions are followed by a redraw and a lot of code that doesn't need to be executed until the complete tree change is done. You don't usually want to see intermediate versions of tree. You want to see the finished tree. When making changes using vnodes, nothing is redrawn until you explicitly call `c.redraw()`.

I took a quick look through the vnode code and only saw the redraw for .h and .b changes, but not for the tree structure changes. Is that what you are referring to or was there something else I missed?

 [...]

The function make_snapshot creates a snapshot of data needed to recreate subtree structure. In the variant where no changes to headlines nor bodies are made, this function takes enough data to undo the changes. However if you need to keep record of headlines and bodies too, then you should add them too to snapshot. Of course  in that case you must change the function from_snapshot too. 

Got it, thanks.

 

[...] 

This two functions can store and recreate the exact copy of the given vnode. If you want to make snapshot of the whole outline, pass the c.hiddenRootNode as the argument to the make_snapshot function. However, if you are certain that the only changes made are under one vnode (for example @ftlist node), then it is more efficient to store only subtree of @ftlist.

Thanks a lot for your help.

Brian