leo-editor

"Positioning outside of hoisted outline" usually causes problems

Bug #875327 reported by SegundoBob on 2011-10-15

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	leo-editor	Fix Released	Medium	Edward K. Ream	leo-editor 4.10-b1

Bug Description

"Positioning outside of hoisted outline" can be done by many different bits of code in Leo-Editor (and its plugins). These many different bits of code are not consistent with one another and most of them do not do the positioning correctly.

Chapters are an optionally enabled option of Leo-Editor. Hoists are enabled by enabling plugin chapter_hoist.py. Selecting a non-main chapter is equivalent (for the purposes of this bug description) to hoisting that non-main chapter's root node. Hoist is done by clicking button "save-hoist" which does the hoist and creates a button to that when clicked will redo the hoist.

Two bits of code "correctly" deal with "Positioning outside of hoisted outline," but in manners that are inconsistent with one another:

Find (Ctrl-F) prints an error message to the Log tab and leaves the position unchanged.

goto-next-clone Alt-N is inconsistent with itself as well as with Find. That is, Alt-N acts one way if the hoist was done by selecting a non-main chapter, and a different way if the hoist was done by the save-hoist button:

1) Alt-N always begins by selecting the "main" chapter--even when the next clone is in the selected chapter. So Alt-N avoids all the problems because it can never "Position outside of hoisted outline."

2) When save-hoist does a hoist, Alt-N ignores all clones that are outside of the "hoisted outline." Again, Alt-N can never "Position outside of hoisted outline." And it can position between clones within the "hoisted outline." All well and good, except you can position on a node clearly marked as a clone by Leo-Editor, but Alt-N prints an error message to the Log pane saying roughly "The selected node is NOT a clone." (Because all its clones are "outside of hoisted outline.") This is puzzling and should probably be considered a bug.

Most of the other bits of positioning code behave in a consistently bad way, but a few work perfectly. See below.

For the little it is worth since I'm not going to fix these problems and don't understand Leo-Editor very well, I recommend the Ctrl-F (Find) behavior. That is, if the position change stays in the "hoisted outline" then it should be done without undoing the hoist that is in effect. If the request would position outside the "hoisted outline," then an error message should be printed to the Log pane and the position should be left unchanged.

Implementing my recommendation for the Nav tab is less than ideal. For the user to see an error messages in the Log tab the focus must be shifted to the Log tab. The Nav tab shows all the matches in all the chapters. Perhaps the matches in the non-selected chapters should be gray instead of black since it is impossible to position to them.

Headline-Body mismatch

All of the positioning commands listed below suffer from the following objectionable behavior:

When the new position is outside the "hoisted outline," the node selection shown in the outline pane does not change, but the body pane is changed to contain the body of the new position. This is a headline-body mismatch.

Positioning Commands suffering headline-body mismatch:

goto-first-node
goto-last-node
goto-next-changed
goto-next-history-node (The nav_qt.py plugin's "next" arrow on the icon bar)
goto-next-marked (Ctrl-Shift-M)
goto-parent
goto-prev-history-node (The nav_qt.py plugin's "prev" arrow on the icon bar)
goto-prev-node

Position Changing Key Combinations suffering headline-body mismatch:

Alt-LeftArrow
Alt-RightArrow

These Command Work Fine in a hoisted outline

These commands work perfectly. Note that they should never position outside the "hoisted outline"--and they don't (provided you start from a position in the "hoisted outline." See bug 875323 https://bugs.launchpad.net/leo-editor/+bug/875323. If you start with an invalid position, even these "good" positioning commands behave badly.)

goto-first-visible-node (Alt-End)
goto-last-visible-node (Alt-Home)
goto-next-visible (Alt+Down)
goto-prev-visible (Alt+Up)
goto-first-sibling
goto-last-sibling

Revision history for this message

SegundoBob (bhossley) wrote on 2011-10-16:

Test System:
Ubuntu Studio 11.04
Leo-editor revision 4591

Leo-editor log pane on startup:

Leo Log Window
Leo 4.9 final, build 4411, June 21, 2011
Python 2.7.1, qt version 4.7.2
linux2
setting leoID from os.getenv('USER'): 'bob'
load dir: /home/bob/bzrWork/pluginPath/leo/core
global config dir: /home/bob/bzrWork/pluginPath/leo/config
home dir: /home/bob
reading settings in /home/bob/bzrWork/pluginPath/leo/config/leoSettings.leo
reading settings in /home/bob/.leo/myLeoSettings.leo
reading settings in /home/bob/tmp/c.leo
reading: /home/bob/tmp/c.leo
global @command command-reverse-siblings
global @command command-goto-last-child alt-=

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2011-10-18: Re: [Bug 875327] Re: "Positioning outside of hoisted outline" usually causes problems

On Sat, Oct 15, 2011 at 10:32 PM, SegundoBob

> "Positioning outside of hoisted outline" can be done by many different
> bits of code in Leo-Editor (and its plugins). These many different
> bits of code are not consistent with one another and most of them do
> not do the positioning correctly.

Thanks for this detailed report. If anything, the problem is harder
than you think :-)

Controlling multiple body editors is very complex. Attempting to
execute "significant" code in onFocusIn or onFocusOut event handlers
is difficult or impossible because new events can be generated.

I take these kinds of reports seriously; fixing them may require
significant new invention and can not be done simply. Nor can any
proposed fix be tested easily. Unit tests for this kind of problem
can be hard to set up and can often be misleading.

I'll look into these reports when other urgent code projects are
complete. In the meantime, the only workaround would seem to be to
avoid the problematic use cases, which may include chapters, hoists,
and various plugins that attempt to remember history. I would suggest
using clones instead, as I do.

Edward

Edward K. Ream (edreamleo) on 2011-10-26

Changed in leo-editor:
assignee:	nobody → Edward K. Ream (edreamleo)
importance:	Undecided → Medium

Edward K. Ream (edreamleo) on 2012-03-07

Changed in leo-editor:
milestone:	none → 4.10-b1

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2012-03-08:

Rev 5093 fixes an important bug in c.positionExists, which likely caused all sorts of problems throughout Leo.

In particular, fixing this bug will likely fix all problems with Leo's crucial c.selectPosition method relating to chapters.

As I write this, it seems to me that c.selectPosition should also consider c.hoistStack, and de-hoist as necessary to make the desired node visible. I'll experiment today. Hoisting and chapters are interrelated, so some care will be needed.

The main point, however, is that a general fix probably involves just c.selectPosition and its helpers. That's good news.

Note that the chapter_hoist plugin is not required. You can hoist at any time using the hoist and de-hoist commands.

Changed in leo-editor:
status:	New → Confirmed

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2012-03-08:

> As I write this, it seems to me that c.selectPosition should also consider c.hoistStack, and de-hoist as necessary to make the desired node visible.

I've just confirmed that double-clicking an @url node referring to a node outside a hoist still causes problems, even when in the main chapter.

I'll fix this next. This should be a relatively straightforward to c.selectPosition, but as always chapters could create complications...

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2012-03-08:

> I've just confirmed that double-clicking an @url node referring to a node outside a hoist still causes problems, even when in the main chapter.

> I'll fix this next. This should be a relatively straightforward to c.selectPosition, but as always chapters could create complications...

Done in the trunk at rev 5094. I am going to mark this bug as fixed, but there are at least two scenarios:

1. The fix is general. This isn't impossible :-) It might even be likely, given the importance of c.selectPosition.

2. The fix causes other problem, perhaps particularly with plugins.

There is no way to know for sure now how good the fix is. Only time, and future bug reports, will tell.

Feel free to reopen this bug. In that case, please be as specific as possible about what problems appear.

Changed in leo-editor:
status:	Confirmed → Fix Released

Revision history for this message

SegundoBob (bhossley) wrote on 2012-03-11:

Very small outline showing head-body mismatch problems. Edit (882 bytes, text/xml)

Test procedure:
Open outline headline-body-mismatch.leo (which is attached here).
Select the node with headline 0101-01-H.
Click on Outline --- Hoist.
Alt-x, goto-first-node.
A bug had occurred. There is now a mismatch between the selected node shown in the outline pane and the body shown in the body pane.
The body pane shows "01-01-B". This is the body of the first node in the outline.
The body pane should show "0101-01-B".

There are additional problems now:
1) Outline --- De-Hoist is grayed out. There is no way to de-hoist.
2) Ctrl-h does not work. It just causes the following message on the console:
error *** no item for <pos 162306060 childIndex: 0 lvl: 0 key: 172612012:0 0101-01-H> load,runMainLoop,eventFilter,masterKeyHandler,masterCommand,doCommand,editHeadline,editLabel
--- end error message ---
Headline "0101-01-H" can be opened for editing by double-left-clicking it.

Test system:
Ubuntu 11.10 with Fluxbox window manager
Leo Log Window
Leo 4.9.1 devel, build 5113, 2012-03-10 15:43:07
Python 2.7.2, qt version 4.7.3
linux2
setting leoID from os.getenv('USER'): 'bob'
load dir: /home/bob/bzr/LeoLatest/leo/core
global config dir: /home/bob/bzr/LeoLatest/leo/config
home dir: /home/bob
reading settings in /home/bob/bzr/LeoLatest/leo/config/leoSettings.leo
reading settings in /home/bob/.leo/myLeoSettings.leo
reading settings in /media/datw1/BobH/1/Leo/other/headline-body-mismatch.leo
reading: /media/datw1/BobH/1/Leo/other/headline-body-mismatch.leo

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2012-03-11:

On Sat, Mar 10, 2012 at 7:21 PM, SegundoBob <email address hidden> wrote:

> ...There is now a mismatch between the selected node shown in the outline pane and the body shown in the body pane.
...
> There are additional problems now:...

Thanks for the new testing. These bugs must be fixed before b1.

Edward

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2012-03-11:

On Sun, Mar 11, 2012 at 7:18 AM, Edward K. Ream <email address hidden> wrote:

> These bugs must be fixed before b1.

This bug is really only the tip of an iceberg. Many similar bugs
likely could be discovered, one for most of the commands in the
Outline:Go to... menu.

The problem is that the way Leo implements hoist and chapters is
pretty much a fake: the tree.full_redraw method only draws what
should be visible, but Leo's commands (and especially Leo's tree
iterators) mostly don't know about hoists. They act as if the full
outline were visible.

In particular, c.rootPosition() does *not* change as the result of a hoist.

Actually, the situation is even a bit more complicated than that. The
goto-next/prev-visible commands *do* know about the hoist stack. But
the majority of commands in the Outline:Go to... menu are likely buggy
in this regard.

Imo, the best solution would be to implement the "different colored
links" world, so that all of Leo's iterators would act on links of the
present color. This would eliminate all the present special cases.
But this is too much to do for 4.10.

So the question is, what to do about this mess? It has existed,
almost surely, ever since Leo's hoist command saw first light, which
is a *long* time ago.

I don't believe it would be wise to change Leo's iterators in any way
now. Again, that would be too much to do for 4.10. BTW, no change in
the iterators would be needed in the "multiple-links" world: the
iterators would just work on the present set of links, blissfully
unaware that others sets of links exists.

I suppose the only reasonable way forward just now would be to deal
with each command in the Outline:Go to... menu separately. This can
be done safely, because these are Commands methods, not the iterators,
which are position methods. Thus, changing these commands have
strictly limited scope.

In effect, this plan adds multiple band-aids to the problem, rather
than fixing the problem at its source. But imo that can't be helped.
The alternative would likely delay 4.10 for a month or more while we
fully test the new world. I don't want to do that.

Edward

On Sun, Mar 11, 2012 at 7:18 AM, Edward K. Ream <edreamleo@gmail.com> wrote:

> These bugs must be fixed before b1.

This bug is really only the tip of an iceberg.  Many similar bugs
likely could be discovered, one for most of the commands in the
Outline:Go to... menu.

The problem is that the way Leo implements hoist and chapters is
pretty much a fake:  the tree.full_redraw method only draws what
should be visible, but Leo's commands (and especially Leo's tree
iterators) mostly don't know about hoists.  They act as if the full
outline were visible.

In particular, c.rootPosition() does *not* change as the result of a hoist.

Actually, the situation is even a bit more complicated than that.  The
goto-next/prev-visible commands *do* know about the hoist stack.  But
the majority of commands in the Outline:Go to... menu are likely buggy
in this regard.

Imo, the best solution would be to implement the "different colored
links" world, so that all of Leo's iterators would act on links of the
present color.  This would eliminate all the present special cases.
But this is too much to do for 4.10.

So the question is, what to do about this mess?  It has existed,
almost surely, ever since Leo's hoist command saw first light, which
is a *long* time ago.

I don't believe it would be wise to change Leo's iterators in any way
now. Again, that would be too much to do for 4.10.  BTW, no change in
the iterators would be needed in the "multiple-links" world: the
iterators would just work on the present set of links, blissfully
unaware that others sets of links exists.

I suppose the only reasonable way forward just now would be to deal
with each command in the Outline:Go to... menu separately.  This can
be done safely, because these are Commands methods, not the iterators,
which are position methods.  Thus, changing these commands have
strictly limited scope.

In effect, this plan adds multiple band-aids to the problem, rather
than fixing the problem at its source.  But imo that can't be helped.
The alternative would likely delay 4.10 for a month or more while we
fully test the new world.  I don't want to do that.

Edward

Edward K. Ream (edreamleo) on 2012-03-11

Changed in leo-editor:
status:	Fix Released → In Progress

Revision history for this message

SegundoBob (bhossley) wrote on 2012-03-11:

As my original description of this bug tried to make clear, I'm well aware that this bug affects many of the command on the "go to" menu--and all the "find" facilities (Ctrl+f, Shift+Ctrl+f).

I think it would be much, much safer to leave these bugs untouched till you can fix them properly. As you say, they have all existed for a very long time. It is almost that your band aids will cause more trouble than leaving them as they are.

But it will be your time that you will mostly waste, and trouble for yourself that you will mostly achieve.

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2012-03-11:

#10

> In effect, this plan adds multiple band-aids to the problem, rather than fixing the problem at its source.

Happily, a single band-aid seems to have sufficed. c.selectPosition now calls c.redraw when it changes the hoist stack.

Note that the call to cc.selectChapterForPosition also calls c.redraw in similar circumstances, which is proper.

BTW, c.redraw only queues a request for a later redraw--we never worry about "redundant" calls to c.redraw: they won't cause flicker.

Changed in leo-editor:
status:	In Progress → Fix Released

Revision history for this message

Edward K. Ream (edreamleo) wrote on 2012-03-11:

#11

On Sun, Mar 11, 2012 at 12:20 PM, SegundoBob <email address hidden> wrote:
> As my original description of this bug tried to make clear, I'm well
> aware that this bug affects many of the command on the "go to" menu--and
> all the "find" facilities (Ctrl+f, Shift+Ctrl+f).

I think the latest fix is pretty general. I just tried Ctrl-f, and I
got this message:

found match outside of hoisted outline
not found 'newHeadline'

Arguably, ctrl-f should just de-hoist the outline, but that's another bug ;-)

In any case, please continue your excellent testing.

Edward

Revision history for this message

SegundoBob (bhossley) wrote on 2012-03-11:

#12

Excellent fixing.

Works solidly for me.

Test System:
Ubuntu 11.10 with Fluxbox window manager

Leo Log Window
Leo 4.9.1 devel, build 5115, 2012-03-11 11:58:16
Python 2.7.2, qt version 4.7.3
linux2
setting leoID from os.getenv('USER'): 'bob'
load dir: /home/bob/bzr/LeoLatest/leo/core
global config dir: /home/bob/bzr/LeoLatest/leo/config
home dir: /home/bob
reading settings in /home/bob/bzr/LeoLatest/leo/config/leoSettings.leo
reading settings in /home/bob/.leo/myLeoSettings.leo
reading settings in /media/datw1/BobH/1/Leo/other/headline-body-mismatch.leo
reading: /media/datw1/BobH/1/Leo/other/headline-body-mismatch.leo