New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Section Reference causes clone on Leo-Editor file open #132

Closed
SegundoBob opened this Issue Jan 17, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@SegundoBob
Contributor

SegundoBob commented Jan 17, 2015

Bug Description

A cloned node reappears every time a particular Leo-Editor file is opened, no matter how many times you delete the clone and save the file without the clone, the clone reappears when the file is opened.

Bug Analysis

My best guess is that the crucial circumstances causing the bug are: (1) The triggering syntax must be in an @file. The type of the @file doesn't matter. (2) The section reference must be in the grandparent of the section node.

No errors are reported to the log pane. No errors are reported to the terminal.

Bug Demonstration

Put the following two files into any one folder.
Open sectionCausesClone.leo.
Note that node "Child" is cloned. This is the bug. In these two files, node "Child" is NOT cloned. This clone was created when sectionCausesClone.leo was opened. You can prove this to yourself by deleting either of the cloned nodes, save, exit, and reopen. Note that once again "Child" is cloned.

sectionCausesClone.leo

<?xml version="1.0" encoding="utf-8"?>
<!-- Created by Leo: http://leoeditor.com/leo_toc.html -->
<?xml-stylesheet ekr_test?>
<leo_file xmlns:leo="http://leoeditor.com/namespaces/leo-python-editor/1.1" >
<leo_header file_format="2" tnodes="0" max_tnode_index="0" clone_windows="0"/>
<globals body_outline_ratio="0.5" body_secondary_ratio="0.5">
    <global_window_position top="50" left="50" height="500" width="700"/>
    <global_log_window_position top="0" left="0" height="0" width="0"/>
</globals>
<preferences/>
<find_panel_settings/>
<vnodes>
<v t="bob05.20150117111055.4"><vh>@file sectionCausesClone.py</vh></v>
</vnodes>
<tnodes>
</tnodes>
</leo_file>

sectionCausesClone.py

#@+leo-ver=5-thin
#@+node:bob05.20150117111055.4: * @file sectionCausesClone03.py
#@+<< imports >>
#@+middle:bob05.20150117111055.5: ** Child
#@+node:bob05.20150117111055.6: *3* << imports >>
#@-<< imports >>
#@+others
#@+node:bob05.20150117111055.5: ** Child
#@+others
#@-others
#@-others
#@-leo

Test Systems

Xubuntu32 12.04

I first noted this bug on 2014-12-31 Wed using commit ad0392f
and Python 2.

I have verified that the bug still exists using commit f6cdb2b
and
Python 2.7.3, PyQt version 4.8.1
and
Python 3.2.3, PyQt version 4.8.1

@edreamleo edreamleo self-assigned this Feb 27, 2015

@edreamleo edreamleo added the Bug label Apr 27, 2015

@edreamleo edreamleo added this to the 5.2 milestone Apr 27, 2015

@edreamleo edreamleo removed this from the 5.2 milestone Jun 9, 2015

@edreamleo edreamleo added this to the 5.3 milestone Mar 15, 2016

@edreamleo

This comment has been minimized.

Show comment
Hide comment
@edreamleo

edreamleo Apr 5, 2016

Member

The original bug report clearly demonstrates that the bug is in the write logic. The .leo file contains:

- @file sectionCausesClone.py
  << imports >> # section reference
  @others
  - Child
    @others
    - <<imports>> # section definition node

As you can see from the given .py file, there are two @+node sentinels for Child, so the read logic is perfectly justified in creating the clones. Note: There are two @Others statements. One would expect that the output logic would write something like this instead:

#@+leo-ver=5-thin
#@+node:bob05.20150117111055.4: * @file sectionCausesClone.py
#@+<< imports >>
#@+node:bob05.20150117111055.6: *3* << imports >>
#@-<< imports >>
#@+others
#@+node:bob05.20150117111055.5: ** Child
#@+others
#@-others 
#@-others 
#@-leo

This is the "real" cause of the bug. It should be fairly straightforward to fix.. The line:

#@+middle:bob05.20150117111055.5: ** Child

seems to be necessary in order create a placeholder node for child to which the <<imports>> can be placed as a child. This may affect both the read and write code. Let's run some experiments:

Experiment 1: Deleting the line:

#@+middle:bob05.20150117111055.5: ** Child

and reloading Leo yields this (more typical) outline:

- @file sectionCausesClone.py
  << imports >> # section reference
  @others
  - << imports >>
  - Child
    @others

Alas, this outline doesn't match the original outline.

Experiment 2: The following .py file recreates the original outline:

#@+leo-ver=5-thin
#@+node:bob05.20150117111055.4: * @file sectionCausesClone03.py
#@+<< imports >>
#@+middle:bob05.20150117111055.5: ** Child
#@+others
#@-others
#@+node:bob05.20150117111055.6: *3* << imports >>
#@-<< imports >>
#@+others
#@-others
#@-leo

To make this work, the write code must write the @+middle node completely, and the write code must remove Child from the pending list of nodes to be written by @Others.

Summary

Any change to Leo's write code is, in fact, a major change to Leo. Happily, the affect code is never used in typical Leo outlines. At present, it does not appear that Leo's read code needs to change, but that is far from proven.

Member

edreamleo commented Apr 5, 2016

The original bug report clearly demonstrates that the bug is in the write logic. The .leo file contains:

- @file sectionCausesClone.py
  << imports >> # section reference
  @others
  - Child
    @others
    - <<imports>> # section definition node

As you can see from the given .py file, there are two @+node sentinels for Child, so the read logic is perfectly justified in creating the clones. Note: There are two @Others statements. One would expect that the output logic would write something like this instead:

#@+leo-ver=5-thin
#@+node:bob05.20150117111055.4: * @file sectionCausesClone.py
#@+<< imports >>
#@+node:bob05.20150117111055.6: *3* << imports >>
#@-<< imports >>
#@+others
#@+node:bob05.20150117111055.5: ** Child
#@+others
#@-others 
#@-others 
#@-leo

This is the "real" cause of the bug. It should be fairly straightforward to fix.. The line:

#@+middle:bob05.20150117111055.5: ** Child

seems to be necessary in order create a placeholder node for child to which the <<imports>> can be placed as a child. This may affect both the read and write code. Let's run some experiments:

Experiment 1: Deleting the line:

#@+middle:bob05.20150117111055.5: ** Child

and reloading Leo yields this (more typical) outline:

- @file sectionCausesClone.py
  << imports >> # section reference
  @others
  - << imports >>
  - Child
    @others

Alas, this outline doesn't match the original outline.

Experiment 2: The following .py file recreates the original outline:

#@+leo-ver=5-thin
#@+node:bob05.20150117111055.4: * @file sectionCausesClone03.py
#@+<< imports >>
#@+middle:bob05.20150117111055.5: ** Child
#@+others
#@-others
#@+node:bob05.20150117111055.6: *3* << imports >>
#@-<< imports >>
#@+others
#@-others
#@-leo

To make this work, the write code must write the @+middle node completely, and the write code must remove Child from the pending list of nodes to be written by @Others.

Summary

Any change to Leo's write code is, in fact, a major change to Leo. Happily, the affect code is never used in typical Leo outlines. At present, it does not appear that Leo's read code needs to change, but that is far from proven.

@edreamleo

This comment has been minimized.

Show comment
Hide comment
@edreamleo

edreamleo Apr 6, 2016

Member

This discussion shows that the problem is far more serious than accidental cloning.

The proposed solution: eliminate @+middle sentinels.

Member

edreamleo commented Apr 6, 2016

This discussion shows that the problem is far more serious than accidental cloning.

The proposed solution: eliminate @+middle sentinels.

edreamleo added a commit that referenced this issue Apr 6, 2016

A proposed fix for #132: Section Reference causes clone on Leo-Editor…
… file open

All unit tests pass, but I won't push until I verify that writing all existing files leaves them unchnaged.

Leo build: 20160406074539
@edreamleo

This comment has been minimized.

Show comment
Hide comment
@edreamleo

edreamleo Apr 6, 2016

Member

rev e10a989 appears to fix this bug, completely and safely:

  1. The write code no longer writes @+middle or @-middle sentinels. In fact, it double-checks that it doesn't.
  2. The read code warns when reading @+middle sentinels, but otherwise is completely unchanged.
  3. The new code passes every test I can think of. In particular, LeoPy.leo loads properly without caching, and write-at-file-nodes leaves all of Leo's sources and plugins unchanged.
  4. The new code handles sectionCausesClone.leo/.py exactly as expected in the new scheme of things:
  • The code warns about @+middle nodes.
  • The code reads the file as expected, putting the << imports >> node in a better place.
  • The code writes sectionCausesClone.py as desired.
  • The code re-reads sectionCausesClone.leo/.py without further changes or warnings.

Imo, this is a fairly minor change to Leo's read/write code, as far as such major changes go ;-) Please report any problems, no matter how seemingly small.

Member

edreamleo commented Apr 6, 2016

rev e10a989 appears to fix this bug, completely and safely:

  1. The write code no longer writes @+middle or @-middle sentinels. In fact, it double-checks that it doesn't.
  2. The read code warns when reading @+middle sentinels, but otherwise is completely unchanged.
  3. The new code passes every test I can think of. In particular, LeoPy.leo loads properly without caching, and write-at-file-nodes leaves all of Leo's sources and plugins unchanged.
  4. The new code handles sectionCausesClone.leo/.py exactly as expected in the new scheme of things:
  • The code warns about @+middle nodes.
  • The code reads the file as expected, putting the << imports >> node in a better place.
  • The code writes sectionCausesClone.py as desired.
  • The code re-reads sectionCausesClone.leo/.py without further changes or warnings.

Imo, this is a fairly minor change to Leo's read/write code, as far as such major changes go ;-) Please report any problems, no matter how seemingly small.

@edreamleo

This comment has been minimized.

Show comment
Hide comment
@edreamleo

edreamleo Apr 6, 2016

Member

Just for the record, this also resolves what is, in essence, a duplicate issue:

#89: Spontaneous clones when using sections

Member

edreamleo commented Apr 6, 2016

Just for the record, this also resolves what is, in essence, a duplicate issue:

#89: Spontaneous clones when using sections

@SegundoBob

This comment has been minimized.

Show comment
Hide comment
@SegundoBob

SegundoBob Apr 6, 2016

Contributor

Your fix appeals to me.

I have verified that now Leo-Editor handles test sectionCausesClone.leo in a reasonable manner.

Leo 5.3-devel, build 20160406093412, Wed Apr  6 09:34:12 CDT 2016
Git repo info: branch = None, commit = 653af30ab842
Python 3.4.3, PyQt version 5.2.1
linux

But it is unfortunate that Leo-Editor does not display a warning when it changes the tree structure.

In the sectionCausesClone.leo case, the "section" node is moved up one level in the tree.

The other case that I've noticed was when I wanted four or five "section" nodes to be in an order different from the order of the "section" references in their parent node. No matter how many times I wrote the .leo with the nodes in my desired order, when I re-opened the file, they were in a different order.

As you pointed out they were in the order of the "section" references and that is the only node order information in the .leo file. So Leo-Editor can't notice the change on the read, but would it be too much trouble to notice the problem on the write?

In the sectionCausesClone.leo case, would it be too much trouble to display a warning on the write or read?

So far I'm one of only two people who have reported noticing unrequested tree structure changes. Consequently, I would give adding warning messages very low priority.

Contributor

SegundoBob commented Apr 6, 2016

Your fix appeals to me.

I have verified that now Leo-Editor handles test sectionCausesClone.leo in a reasonable manner.

Leo 5.3-devel, build 20160406093412, Wed Apr  6 09:34:12 CDT 2016
Git repo info: branch = None, commit = 653af30ab842
Python 3.4.3, PyQt version 5.2.1
linux

But it is unfortunate that Leo-Editor does not display a warning when it changes the tree structure.

In the sectionCausesClone.leo case, the "section" node is moved up one level in the tree.

The other case that I've noticed was when I wanted four or five "section" nodes to be in an order different from the order of the "section" references in their parent node. No matter how many times I wrote the .leo with the nodes in my desired order, when I re-opened the file, they were in a different order.

As you pointed out they were in the order of the "section" references and that is the only node order information in the .leo file. So Leo-Editor can't notice the change on the read, but would it be too much trouble to notice the problem on the write?

In the sectionCausesClone.leo case, would it be too much trouble to display a warning on the write or read?

So far I'm one of only two people who have reported noticing unrequested tree structure changes. Consequently, I would give adding warning messages very low priority.

@edreamleo

This comment has been minimized.

Show comment
Hide comment
@edreamleo

edreamleo Apr 6, 2016

Member

Leo-Editor [now] handles test sectionCausesClone.leo in a reasonable manner.

Thanks for this confirmation. I appreciate it.

...when I wanted four or five "section" nodes to be in an order different from the order of the "section" references in their parent node. No matter how many times I wrote the .leo with the nodes in my desired order, when I re-opened the file, they were in a different order.

I don't believe this can ever change. The present read code is not simple, and it does honor the data in the external file. Once can imagine "heroic" attempts to retain existing outline structure, but I've already gone down that road with the ill-fated "View" project. See:
leoNotes.txt-->Unused code: do not delete-->leoViews stuff (keep)
I'm not going down that road again.

Yes, I know that there are languages for which sections must substitute for proper classes and methods. But I am not going to make any major changes to Leo's read code.

In the sectionCausesClone.leo case, would it be too much trouble to display a warning on the write or read?

Easy enough, but pretty much pointless, imo. The warning would quickly become noise, as all such warnings do. The only solution, it seems to me, is to put section definitions where they logically belong in the @file tree. If you want to manage those sections in other ways, do so in an organizer node outside the @file tree. You then have complete freedom.

HTH.

Member

edreamleo commented Apr 6, 2016

Leo-Editor [now] handles test sectionCausesClone.leo in a reasonable manner.

Thanks for this confirmation. I appreciate it.

...when I wanted four or five "section" nodes to be in an order different from the order of the "section" references in their parent node. No matter how many times I wrote the .leo with the nodes in my desired order, when I re-opened the file, they were in a different order.

I don't believe this can ever change. The present read code is not simple, and it does honor the data in the external file. Once can imagine "heroic" attempts to retain existing outline structure, but I've already gone down that road with the ill-fated "View" project. See:
leoNotes.txt-->Unused code: do not delete-->leoViews stuff (keep)
I'm not going down that road again.

Yes, I know that there are languages for which sections must substitute for proper classes and methods. But I am not going to make any major changes to Leo's read code.

In the sectionCausesClone.leo case, would it be too much trouble to display a warning on the write or read?

Easy enough, but pretty much pointless, imo. The warning would quickly become noise, as all such warnings do. The only solution, it seems to me, is to put section definitions where they logically belong in the @file tree. If you want to manage those sections in other ways, do so in an organizer node outside the @file tree. You then have complete freedom.

HTH.

edreamleo added a commit that referenced this issue Apr 6, 2016

Removed code related to *writing* @+middle and @-middle sentinels.
Considerable code remains related to *reading* such sentinels.  That code will remain.
Pylint is happy, which is significant, and all unit tests pass.
No further work is needed or planned for #132.

Leo build: 20160406153226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment