Progress report re #1451: unsaved files

[Edward K. Ream]

The complexity of the undo code has collapsed. This is a most welcome milestone:

- All the dirtyVnodeList kwargs and associated logic are gone.  This is big.

The undo/redo code now just calls p.setDirty one or more times. This works because p.setDirty now calls v.setAllAncestorAtFileNodesDirty. This is feasible because Vitalije's code is super fast, and because it uses vnodes, not positions.

When moving node.p, the pattern is:

p.setDirty()
<< do the move >>
p.setDirty()

The point is that moving a node may change its parents, so both calls to p.setDirty are necessary.

This is surely the simplest thing that could possibly work.  However, I am tempted to have the low-level VNode methods call v.setAllAncestorAtFileNodesDirty "automagically".  This is feasible now that marking nodes dirty happens in the vnode world, not the position world.  However, I probably won't go that far: the low-level vnode methods probably don't have enough context to do the right thing reliably.

- v.setAllAncestorAtFileNodesDirty no longer returns a dirtyVnodeList, so I was able to collapse the complexity of that method as well.

I have thoroughly reviewed all aspects of the undo code.  However, more simplifications may be possible, and a few arcane questions remain.  It's possible that those questions represent edge cases that could cause bugs.  I expect to work on this issue for a day or two more.

Edward

[Brian Theado]

If I'm understanding correctly, with your planned changes if a user opens a leo file and makes a few changes and then undoes all the changes and tries to close the file, then the user will be prompted about saving changes even though the outline matches everything on disk. Is that correct?

That behavior doesn't seem to be the most user friendly. Better that than having the bug where changes are lost, but I wonder if there is a way to have both without it being too complicated?

Just thinking out loud, but maybe it would work to have a dirty counters for the nodes. After a save, all the dirty counters are reset to zero. Undo operations decrement the dirty counter for affected nodes and redo operations increment the counter. Any time a given node's counter is at zero, the dirty bit is cleared and any time the counter is non-zero, the bit is lit.

--
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/2874489c-defe-4bdf-90a9-b2c6639858ef%40googlegroups.com.

[Edward K. Ream]

On Sun, Dec 15, 2019 at 7:59 AM Brian Theado <brian....@gmail.com> wrote:

If I'm understanding correctly, with your planned changes if a user opens a leo file and makes a few changes and then undoes all the changes and tries to close the file, then the user will be prompted about saving changes even though the outline matches everything on disk. Is that correct?

Yes.

That behavior doesn't seem to be the most user friendly. Better that than having the bug where changes are lost, but I wonder if there is a way to have both without it being too complicated?

This is actually a completely different issue :-)  #1451 involves making sure that all external files are written, which devolves into making sure that all @<file> nodes are marked dirty when they need to be.

The behavior you mention involves making sure that the user is given the opportunity to save external files (and the outline itself) when they try to close the outline.  Leo's behavior is standard in this regard, with the proviso that change may "cancel" so that no change is actually made.  It would be possible to do a trial write to see if there were any change, but that's a nit, and it would substantially complicate the code.  Imo, it's not worth doing.

Just thinking out loud, but maybe it would work to have a dirty counters for the nodes. After a save, all the dirty counters are reset to zero. Undo operations decrement the dirty counter for affected nodes and redo operations increment the counter. Any time a given node's counter is at zero, the dirty bit is cleared and any time the counter is non-zero, the bit is lit.

There have been proposals for per-node undo, and this is similar.  After congratulating myself on eliminating evil undo-related state, I am in no mood to add any new caching.  The total advantage of such a scheme, in the entire projected history of the universe, does not begin to compare with the pain that any bug in such a scheme would cause.

Edward

[Brian Theado]

Edward,

On Sun, Dec 15, 2019 at 5:42 PM Edward K. Ream <edre...@gmail.com> wrote:

On Sun, Dec 15, 2019 at 7:59 AM Brian Theado <brian....@gmail.com> wrote:

[...] 

That behavior doesn't seem to be the most user friendly. Better that than having the bug where changes are lost, but I wonder if there is a way to have both without it being too complicated?

This is actually a completely different issue :-)  #1451 involves making sure that all external files are written, which devolves into making sure that all @<file> nodes are marked dirty when they need to be.

It is a different issue, but one being created as "collateral damage" by the dirty changes you are making for #1451. Or am I misunderstanding? I mean the scenario I described where the user gets unnecessarily prompted about saving changes it not present prior to #1451 changes.

 

The behavior you mention involves making sure that the user is given the opportunity to save external files (and the outline itself) when they try to close the outline.  Leo's behavior is standard in this regard, with the proviso that change may "cancel" so that no change is actually made.  It would be possible to do a trial write to see if there were any change, but that's a nit, and it would substantially complicate the code.  Imo, it's not worth doing.

I agree having code which does a trial write is not worthwhile. However, having code which did perfect dirty bookkeeping could be worthwhile if it were easy enough to do and be sure enough it won't cause any data loss.

 

Just thinking out loud, but maybe it would work to have a dirty counters for the nodes. After a save, all the dirty counters are reset to zero. Undo operations decrement the dirty counter for affected nodes and redo operations increment the counter. Any time a given node's counter is at zero, the dirty bit is cleared and any time the counter is non-zero, the bit is lit.

There have been proposals for per-node undo, and this is similar.  After congratulating myself on eliminating evil undo-related state, I am in no mood to add any new caching.  The total advantage of such a scheme, in the entire projected history of the universe, does not begin to compare with the pain that any bug in such a scheme would cause.

The way I describe it implies two separate pieces of data (dirty counter and dirty bit) and I think that's what you mean when you say "new caching"? I don't think perfect bookkeeping of dirty bits necessarily requires the use of caching, but I can appreciate that implementing such perfect bookkeeping might be quite difficult in a complicated code base and not worth the trouble.

I just wanted to raise the issue to make sure the trade offs were clear. The user is getting a good bargain: losing a little by getting unnecessarily prompted for saving files which are unchanged, but gaining a lot by making sure all external files are written.

Brian

[Edward K. Ream]

On Sun, Dec 15, 2019 at 8:03 PM Brian Theado <brian....@gmail.com> wrote:

> It is a different issue, but one being created as "collateral damage" by the dirty changes you are making for #1451. Or am I misunderstanding? I mean the scenario I described where the user gets unnecessarily prompted about saving changes it not present prior to #1451 changes.

The new behavior is, quite simply, the one and only correct behavior.

> I agree having code which does a trial write is not worthwhile. However, having code which did perfect dirty bookkeeping could be worthwhile if it were easy enough to do and be sure enough it won't cause any data loss.

The old code tried to do "perfect dirty bookkeeping".  Its subtle failures were the cause of #1451.

Imo, saving a file alters the bookkeeping in ways that make robust bookkeeping impossible.  I could try to explain, but the explanation would be complicated, and would not necessarily be correct.  And that's the point.

The new way ensures that the entire outline is marked changed and all relevant nodes are marked dirty any time a change could possibly result in one or more external files needing to be written.  This is the only safe way.

Summary

For the first time ever, the relevant code is simple and robust.  No state data are required in undo logic.

We aren't going back to the old way.

Being prompted when closing an outline is a tiny price to pay for the integrity of our data.  If you don't like such prompts, just bind save-all to Ctrl-s, and do Ctrl-s before quitting Leo.

Edward

[Edward K. Ream]

On Sun, Dec 15, 2019 at 8:03 PM Brian Theado <brian....@gmail.com> wrote:

I didn't notice these comments until just now...

> The way I describe it implies two separate pieces of data (dirty counter and dirty bit) and I think that's what you mean when you say "new caching"?

Yes.

> I don't think perfect bookkeeping of dirty bits necessarily requires the use of caching, but I can appreciate that implementing such perfect bookkeeping might be quite difficult in a complicated code base and not worth the trouble.

I am using the term "caching" loosely.  The old dirtyVnodeList was a kind of temporary cache.

The problems with the dirtyVnodeList were the direct cause of #1451.

> I just wanted to raise the issue to make sure the trade offs were clear. The user is getting a good bargain: losing a little by getting unnecessarily prompted for saving files which are unchanged, but gaining a lot by making sure all external files are written.

Exactly.  I would only add that imo the new scheme is not only the simplest thing that could possibly work, it is, quite literally, the only thing that could possibly work.

Edward