ENB: Ahas re positions

[Edward K. Ream]

For several years now, Vitalije has rightly complained that copied positions can become invalid when the outline changes.

Just now, after a bicycle ride, I saw what seemed at first to be an elegant solution, namely p.cloned_positions. This would be a list of copied positions that Leo will automatically update when the original position changes. This would be easy to do. All the Position methods that change a position p would update the positions in p.cloned_positions.

Hmm. Instead of copying and updating, why not just use the "original" position?  Ah. I see the problem. The position p1 that Leo updates might be equal to another position, p2, but p1.cloned_positions will likely be empty, no matter what p2.cloned_positions is. This problem probably dooms p.cloned_positions.

Instead, it looks like we need a global list, say c.cloned_positions. This will be a list of copied positions that must be kept in sync with any change to positions that are equal to any position p_i in c.cloned_positions. This approach looking promising.

Suppose, we simply change p.copy so that it updates c.cloned_positions. Now, copied positions might "just work". That is, all copied positions will automagically remain in sync, no matter what happens to the outline!

What about deleted nodes and their corresponding positions?

Because of undo, no outline node ever is completed deleted. So what happens when a node (and its associated positions) gets re-inserted into the outline as the result of an undo/redo operation? The undo/redo code will likely have to handle this explicitly as follows:  Whenever undo/redo deletes one or more nodes, it will have to remember the corresponding positions in the undo info (the undo "bead"), just before deleting the positions from c.cloned_position. When these nodes come back to life, the corresponding positions must be added again to c.cloned_positions.

Hmm.  When undo/redo deletes a node(tree) the corresponding Position method might be able to do the housekeeping chores. Ditto for when undo/redo re-inserts a node (tree). But I have a bad feeling about this. Just putting deleted positions on, say, c.deleted_cloned_positions doesn't seem like it retains enough data. This scheme probably has the wrong granularity. In contrast, remembering deleted positions in undo beads looks bullet proof, if a bit clumsy.

Summary

This post has discussed a straightforward way to make all copied positions robust. p.cloned_positions won't work, but a global, c.cloned_positions probably will work.

undo/redo is likely the acid test. The code must be rock solid.

To be investigated:

- Performance issues, including the typical size of c.cloned_positions.

- The proper way to bring cloned positions back from the grave.

Edward

[Thomas Passin]

I think that if a copy of the current list of all positions were included in the undo data, then it could be restored upon undo and everything would work.  I don't know if that would be too slow, so that it would affect the perceived speed of the keystrokes.

[Edward K. Ream]

On Wed, Aug 26, 2020 at 10:11 AM Thomas Passin <tbp1...@gmail.com> wrote:

I think that if a copy of the current list of all positions were included in the undo data, then it could be restored upon undo and everything would work.  I don't know if that would be too slow, so that it would affect the perceived speed of the keystrokes.

Possible.

Note that the default granularity (@string undo-granularity)  of keystrokes is 'line', not 'char'.

Edward

[Thomas Passin]

Maybe the copied position list could be a reference and not a copy if the positions have not changed since the last undo - which would usually be the case.  A dirty flag for the list would have to be carried somewhere.

[vitalije]

I doubt this would be a very good idea. Undo/redo already has a lot of things to do. Adding some more chores to them will likely make things more complicated, slower and probably introduce more bugs.

Instead I suggest another approach. Please consider the rest of this message not as a critic of Leo's internals. I can't write any formal proof of the following claims, so consider them just as my personal feelings about Leo's position class.

The way I see it, right now, positions are mixture of Model, View and Controller. Position methods are used to:

1. [Model] traverse the tree, retrieve the outline data
2. [View] represent outline nodes that can be viewed, clicked, double clicked, expanded or collapsed
   
3. [Controller] modify tree

The fact that they are so involved in all three parts of MVC makes them vulnerable and unreliable not only because they can become invalid - that is just a part of the problem. There are a lot of other parts of Leo that can be simplified by avoiding usage of positions. In every piece of code I wrote in Leo, so far, I found that avoiding positions makes code shorter, cleaner and more robust. More than once I've found some method in Leo that was too complex or it contained a bug somewhere, and when I rewrote it without positions it became shorter, faster and more reliable. At least that is my experience with the Leo positions. There are even some places where one piece of code doesn't have a position but just a vnode, and in order to call some method that expects position one must first construct a dummy position containing the vnode, then call the other method which expects position, and inside this method uses p.v to do its job.

Now, regarding the initial problem (I believe it was about expanded/collapsed state of nodes). Whichever GUI we can imagine that Leo will have in the future, and also in the Qt GUI at the present time, the GUI code will have the information about which visible nodes are expanded and which are collapsed. It is an essential bit of information required to show the outline. So, most natural place to hold this information is GUI code. If we put it somewhere else, we will have to deal with the synchronization between GUI and that other place.

Some Leo commands for moving around the outline, selecting relative positions depend on this information (expanded/collapsed) but apart from that, expanded/collapsed state has no effect on the true meaning of the outline. External files will contain the same content regardless of whether their nodes were expanded or collapsed. So, we can consider this information as something inherently tied to the GUI or View part of MVC.

The simplest possible solution will be just to delegate p.isExpanded method to the GUI and find corresponding tree item and ask it if it is expanded or not. For example:

g.app.gui.isExpanded(p) # or c.frame.tree.isExpanded(p)
# instead of
p.isExpanded()

IMHO the most natural place to store the expanded/collapsed bit about outline node is in the GUI/View. Also rethinking positions and their methods might be beneficial.

My 2 cents.

Vitalije

[Edward K. Ream]

On Wed, Aug 26, 2020 at 11:19 AM vitalije <vita...@gmail.com> wrote:

> Now, regarding the initial problem (I believe it was about expanded/collapsed state of nodes)...The simplest possible solution will be just to delegate the p.isExpanded method to the GUI and find the corresponding tree item and ask it if it is expanded or not.

This could be a good idea.

> For example:

g.app.gui.isExpanded(p) # or c.frame.tree.isExpanded(p)
# instead of
p.isExpanded()

p.isExpanded could just call g.app.gui.isExpanded(p). Similarly, to preserve the existing api, p.expand() and p.contract() could call corresponding g.app.gui methods.

The point is that there is always a two-way interaction between Leo's api's (based on positions or vnodes) and the gui. Having the gui be responsible for the "real" expanded bit is feasible. The expanded bit could go away in the vnode and position classes. Leonine scripts could still determine whether any node/position is expanded.

> IMHO the most natural place to store the expanded/collapsed bit about outline node is in the GUI/View. Also rethinking positions and their methods might be beneficial.

This certainly merits further consideration. Let me consider your overall post further.

Edward

[Félix]

Hello dear Leonista friends !

When I think of commands such as 'selectNextVisible' (or something similar, i'm not at my computer right now) and consider the state of the 'currently selected node' as having importance when executing some model/controller related commands, I fail to consider the expanded/collapsed state as being 'gui' related only.

 It is visualised as expanded/collapsed in the gui, indeed right, but its important in the model/controller as much as the 'headline string' or 'body content' because of those commands which set the selected node based on whats collapsed/expanded. So it is real model data/state as much as headline content and body content.
--

Félix

[vitalije]

 

 It is visualised as expanded/collapsed in the gui, indeed right, but its important in the model/controller as much as the 'headline string' or 'body content' because of those commands which set the selected node based on whats collapsed/expanded. So it is real model data/state as much as headline content and body content.

Nevertheless this piece of data *IS* stored in the Qt GUI, and I am sure that any GUI you can think of that implements Tree widget will have this information too. Whenever you have to keep a copy of some data in two or more places, you'll have to synchronize this data and there is always a possibility of getting out of sync. OTOH, when the model is designed in such a way that a single piece of information is always kept in one and only one place, there is no need for synchronization and it is impossible by design to get out of sync.

You might decide for your GUI not to keep track about expanded/collapsed state and to ask through the Leo bridge every time you need this information, but still under the hood, bridge won't be able to give you a stable response. If you decide to keep track of the expanded/collapsed state in your GUI, you will be able to set this information in Leo correctly before executing any of the commands you mentioned, or better yet, those commands can be separated in several different commands and you just choose one you want depending on the expanded/collapsed state.

This approach can be taken in any GUI. For example:

# instead of
def my_command(...):
    if p.isExpanded():
        do_one_thing()
    else:
        do_other_thing()

# it should be
def my_command_when_collapsed(...):
    do_one_thing()

def my_command_when_expanded(...):
    do_other_thing()

And then, let the GUI choose which command to call, depending on the current expanded/collapsed state.

Vitalije

[Edward K. Ream]

On Wednesday, August 26, 2020 at 9:16:54 AM UTC-5, Edward K. Ream wrote:

For several years now, Vitalije has rightly complained that copied positions can become invalid when the outline changes.

Let me restate the problem I am trying to solve:

1. I would like a subset of positions to remain valid when the outline changes. Call such positions robust positions. Important: robust positions can still become invalid when the corresponding node is deleted. In other words, for any kind of position p, c.positionExists(p) might be False.

2. p.copy() will not deliver robust positions. Only a new method, something like p.moveable_copy() or p.permanent_copy(), would deliver robust positions.

That's all!  I am not interested in any of the following:

A. Eliminating the Position class. That class will remain while I have anything to say about Leo. The Position class is the foundation of Leo's generators. Many user scripts depend on positions.

B. Changing the responsibilities of Leo's various guis.

C. Changing Leo's api's. Even seemingly innocuous changes are fraught with subtle consequences. For example, setting or clearing the expanded bit in a vnode, followed by a call to c.redraw, must work exactly as it does now. It's no good pretending that the gui can be fully responsible for the expanded state of all nodes. Leo's scripts may have something to say about that.

Summary

The stated task is non-trivial. Like many parts of Leo's code, the task will require attention to speed and memory requirements. It will have to be compatible with undo/redo.

Robust positions will become invalid when the corresponding part of the outline is deleted.

Edward

[Edward K. Ream]

On Wednesday, August 26, 2020 at 11:19:56 AM UTC-5, vitalije wrote:

I doubt this would be a very good idea. Undo/redo already has a lot of things to do. Adding some more chores to them will likely make things more complicated, slower and probably introduce more bugs.

Yes, what I now call robust positions would make some parts of the code more complicated. However, the undo/redo code is already complicated, and it passes many substantial tests. Only a few more unit tests would be necessary, imo.

I am not interested in grand alternatives to positions. True, it's often best to recast a problem in terms of vnodes, but I can't mandate that all script writers do that. What I want is for positions to work a bit better.

Edward

[Félix]

(continuing my example)

It's not about which command to run if expanded/collapsed, its about which will be selected when the command is run. (if we're considering the command 'selectNextVisible', for instance... 

So what is computed by the controller, is which p node to set the selection to, based on the currently selected node p, and what nodes are collapsed/expanded. 

So the computation to choose the newly selected node, in the controller with the model, is based on expand/collapse data.

I could write a script on a node, run it through a button or ctrl+b, and that script could do stuff based on data in the tree, headlines, body content, but also expanded/collapsed states. So its a boolean bit of data, that is merely reflected in the gui, but not primarily stored there, in my opinion. It's important data as much a as headline text and body content. (but only 1 small bit). 

It should be treated as such, just like headline labels and body text: rendered by the gui, which also exposes ways to change them (body, headlines and collapsed state - oh, and also user attributes). 

--

Félix

[Félix]

Realized it was silly, & changed my mind about the 'saved with the leo file' part. I removed the last message