LeoU: Formal code review of vnode class

[Edward K. Ream]

This post will be pre-writing for a LeoU issue. Recent discussions about Leo's design have stimulated this post.

Here, I'd like to simulate a formal code review of Leo's vnode (VNode) class.  I'll wear three hats: the presenter, the secretary and the project manager.  I'll use Vitalije's comments as a starting point for a discussion. This discussion would not normally be part of a code view, but imo it's a good teachable moment.

See the summary for conclusions.

Presentation

The VNode class contains all data in an outline, as well as crucial, low-level operations on those data. There is no other place to store node-related data!  In particular, the position class concerns itself only with traversing the tree. The position class contains no node-related data.

The VNode class is one of the foundations of Leo's scripting. As a result, none of the ivars or methods of the VNode class can change in any way. That is, all of the ivars of the VNode class are "official". Scripts can, and do, access these ivars directly.

Theoretically, it would be possible to add new kwargs to VNode methods, but that is extremely unlikely. It would also be possible to add new ivars to the VNode class, but uA's make that unnecessary.

The properties of the VNode class, v.b, v.h, v.gnx, v.u, allow higher-level access to various VNode ivars, but in some cases Leo's code accesses the corresponding ivars directly, for greater performance.

Discussion

Here I'll respond to Vitalije's comments in detail.  This would typically not happen during a formal code review, but these comments will give me a chance to more fully explain the design of the VNode class.

> one of the most basic element of Leo's data is certainly VNode. And yet one can not instantiate VNode class without acquiring full blown commander, which in VNode is known as context. In the comment of context attribute is explicitly declared that it is called context to indicate its limited usage.

Perhaps the "context" kwarg should just be called "c", but of course that can't be done now.  Almost all of Leo's classes have access to "c", the present commander.  There is nothing wrong with that.

What data hiding means to me is that the boundary between the "inside" and the "outside" of a class is (relatively) strict.  As far as the "context" arg goes, the caller (creator) of VNode instances does not need to know anything about how the VNode class uses the context.

The clients of the VNode class must not (and do not) know anything about how the methods of the VNode class work.  All methods (of all classes) must be black boxes.

> However [the context ivar] is used not only as a holder of hiddenRootNode but there is a method on VNode that knows about c.frame.body.wrapper and knows that c.frame.body.wrapper has setInsertPoint, setYScrollPosition...

The VNode class must know everything about c.hiddenRootNode.  It's another VNode instance.

The vnode class must contain the v.scrollBarSpot ivar.

Yes, it would be possible to use c.restoreCursorAndScroll() instead of v.restoreCursorAndScroll(). Ditto for c/v.saveCursorAndScroll().  But such changes would have no consequences for Leo's architecture.

> [The VNode class] also knows indirectly about c.fileCommands.gnxDict.

To my knowledge, this is not true.  The VNode class does not in any way become entangled with other classes merely because v.context exists! The VNode class knows nothing about how other classes use gnx's.

> Now, whichever module needs to access VNode class, can't just import leo.core.leoNodes and use it. It needs to acquire commander instance before.

True, but so what?  Every class in Leo has access to a commander instance via its "c" ivar.  That's why all Leonine scripts have the "c" var predefined.  Imo, it's useless to complain about this.

However, this comment does suggest creating c.new_vnode method:

import leo.core.leoNodes as leoNodes
    # already exists in leoCommands.py
...
def new_vnode(self, gnx=None):
    c = self
    return leoNodes.VNode(context=c, gnx=gnx)

The (small) advantage is that clients of c.new_vnode(gnx) would no longer need to import leo.core.leoNodes.

> Recently, we discussed the copying and pasting outline. It delegates its task to fileCommands. FileCommands OTOH knows about c.frame.resizePaneToRatio, c.selectPosition, c.frame.initialRatios, c.atFileCommands.readAll, c.frame.body.onBodyChanged...

> It seems to me that copying and pasting outlines should belong just to VNode. I would expect that v instance knows best how to encode itself into a string, and to decode itself from string.

The VNode class does know about unicode.  v._headString and v._bodyString contain unicode characters. The v.b and v.h setter properties call v.setBodyString() and v._setHeadString(), which handle the conversion to unicode.

Ironically, the reason this is not obvious is that there is a strong boundary between the VNode class and client code.

> There is no real need for v instance to know anything about c.fileCommands, c.atFileCommands, c.frame.body.wrapper.

Nor does it, in any harmful way.

> You could say that fileCommands is designed to encode/decode vnodes.
> But why then it selects position, sets frame ratios

FileCommands class handles everything associated with reading .leo files. That's its purpose.  Yes, it must do a lot, but that can't be helped.

The FileCommands class creates VNode instances, without in any way knowing the internal details of the VNode class.  Yes, the FileCommands class must know about gnx's, but the converse is not true.  The VNode class knows nothing about how other classes use gnx's.

Crucially, the v.context ivar does not "pollute" the VNode class in any way.  The boundary between the VNode class and all other classes remains intact.

> If fileCommands need to provide reading and saving Leo documents, then it would probably use the same low level helper module as Vnodes for encoding/decoding them from/to string.

v.b and v.h are properties that handle all the details of converting to unicode.

The v.b and v.h properties are a perfect example of enforcing the boundaries between the "inside" of the VNode class and everything else.  The boundary may not be apparent (because v.b and v.h look like ivars), but the boundary exists nonetheless.

There is a huge practical advantage to using properties instead of getter/setter methods on the VNode class.

Most Leonine scripters don't know what v.b and v.h actually are, which is kinda the point. Otoh, those who want to understand the VNode class in full detail must understand that v.context has no harmful architectural consequences whatsoever.

> I could write many more examples but these few would suffice to make my point clear.

I am glad to discuss any other Leo class.

Summary

The VNode class contains all node-related data. That's it's primary purpose.  It also contains crucial low-level operations on VNodes.

The ivars and methods VNode class must never change, except in an upward compatible manner.

The VNode class is, in fact, an excellent example of information hiding.  The boundary between the VNode class is well defined and well enforced.

The v.context ivar is completely benign. It has never caused problems, and it never will. It does not, in any significant way, make the VNode class dependent on any other class. It has no harmful architectural consequences.

In my role as project manager, I pronounce the VNode class to be completely sound, for the reasons just given.

See the Post Script for a summary of possible minor changes.

This thread is the place for all comments and debate about Leo's VNode class.

Edward

P.S.  Here is the "secretary's report", a summary of possible changes related to Leo's VNode class:

1. A new c.new_vnode() method would slightly simplify the creation of new VNodes by making it unnecessary for clients to import leo.core.leoNodes.

2. Leo could use c.restoreCursorAndScroll() instead of v.restoreCursorAndScroll().
   Ditto for c/v.saveCursorAndScroll().

Imo, having v.restoreCursorAndScroll() call a commander method is, for all practical purposes, completely benign. This detail does not violate the boundary between VNodes and the rest of Leo. In particular, code that uses VNodes is completely unaware of this harmless "connection".

EKR

[vitalije]

O.k. No more complains from me.

Vitalije

[Edward K. Ream]

​​

​On Sat, Apr 28, 2018 at 6:46 AM, vitalije <vita...@gmail.com> wrote:

​​

O.k. No more complains from me.

​ Well, I hope you mean, no more complaints about the VNode class ;-) 

Please do continue to question Leo's design, code and style, or anything else. The last thing I want is to stifle discussion and debate.

My brother (a very fine programmer) and I have a running joke.  We ask, "Who wrote this ***?  Oh yeah, I did".  Every time we look at our code with fresh eyes we see something that could be improved.

Thinking back about my reply, I worry that all the italics and boldface might be misunderstood.
I was not at all upset about your criticisms and questions. I was trying to emphasize what I think are truly important ideas underlying Leo's architecture.  These ideas exist explicitly nowhere in the code itself! So I welcome any chance to explain them more fully.

Indeed, I want to say at least a few more things about Leo's architecture.  The fundamental principle could be restated as follows:

    Modules and classes should never know about
    the inner workings of other classes

Just now I see that this isn't as simple an idea as I thought.

Usually, knowing about the existence of methods of other classes is benign.  But calling low-level methods of other classes would be asking for trouble.  But what is a low-level method?

The outline for the VNode class has an organizer node for its low-level methods. Furthermore, all low-level methods of the VNode class have names starting with underscore.  So, for the VNode class, we know for sure which are the low-level methods.

But for other classes, that might not be so obvious.  Leo's code doesn't follow the underscore convention consistently.  I often use the "Helper" suffix instead.

I think it's safe to use methods that correspond directly to user-level commands, but that's only a partial guide...

Turning from methods to ivars, the rule is:

    Code should use only official ivars from other classes

Alas, it may not be obvious which are the official ivars.

"@test official g.app ivars" defines the official ivars of one class.  Other classes have comments about what their official ivars are. CheatSheet.leo contains lists of the important ivars, so I guess the concept of official ivar isn't too mysterious.  In any case, code like:

    c.frame.body.wrapper

should be fairly easy to understand. But I look at the code every day...

Dubious design and code

This is a good time to discuss truly dubious aspects of Leo's code base.

1. The identification of c (and the Commands class) with a single outline on the screen.

The code assumes that "c" means outline. There is no easy way have two separate outline views of the same file.  This greatly complicates all code involved in switching between body editors.

This design choice can't change, but I would think about this more carefully were I to design Leo from scratch.

There have been proposals to handle multiply-selected outline nodes.  That can be indeed be done in a fairly straightforward way, provided that any new commands don't attempt to generalize what c.p means.

2. LeoTree.select is horribly complex.

This code handles all the details of switching between nodes. It can be called as the result of user actions or as the result of commands.  It's likely that all the complexity is inherent, and I wouldn't recommend changing it, but it's still horrible ;-)

3. The key-handling code is still too complex.

The new code is a huge improvement, but problems remain.  As yesterday's bugs show, ks.isPlainKey must be exactly right or embarrassing bugs will result.

I have just added this line to the docstring:

     A plain key is a key that can be inserted into text.

Clearly, the code must eventually make this distinction.  It can't treat '\b' like any other character. 

The helpers of k.masterKeyHandler disgust me every time I look at them.  They are a maze of code that just barely manages to work. There are reasons why the code is as it is, but still...

Edward

[vitalije]

Well, I hope you mean, no more complaints about the VNode class ;-)  

To me it is obvious that we look at the same code and see quite opposite things. I feel like few last threads that I have started brought nothing good, and yet they made us both spend lots of time, perhaps even spoiled our days. So, I guess it would have been better if I didn't start them at all.

I can live with the present code. I will try to fix as many bugs as I can. If I ever make something worthy adding to Leo I will share it. But it seems to me that criticizing the code won't help so I will do my best to avoid it. 

Every time we look at our code with fresh eyes we see something that could be improved. 

Or something that we haven't seen last time we looked. If you ever look again at the VNode class and see that something isn't right I will be glad to answer any question you may have regarding how I see it. But, I won't start the discussion myself. 

Vitalije

[Edward K. Ream]

On Sun, Apr 29, 2018 at 4:54 AM, vitalije <vita...@gmail.com> wrote:

Well, I hope you mean, no more complaints about the VNode class ;-)  

To me it is obvious that we look at the same code and see quite opposite things.

​I greatly value other points of view, especially yours.  ​

 

​Sometimes the most important things aren't in the actual code at all.​

 
 

I feel like few last threads that I have started brought nothing good, and yet they made us both spend lots of time, perhaps even spoiled our days.

​Oh my.  I do apologize for distressing you.
​

 

So, I guess it would have been better if I didn't start them at all.

​On the contrary, you have given us all the opportunity to discuss Leo's architecture. This discussion is worth any amount of my time.

You have contributed greatly to Leo, in many ways.  Sometimes small things make a big difference.

Getting rid of trace statements was one of those things.  It clarifies the code and has lead to new --trace command-line options.  Now, we can investigate code without actually changing it. Not having to toggle trace vars reduces git commits, which is surprisingly helpful when merging git branches.

I can live with the present code. I will try to fix as many bugs as I can. If I ever make something worthy adding to Leo I will share it. But it seems to me that criticizing the code won't help so I will do my best to avoid it. 

​I want to encourage dissent, not squash it. How else can I become aware of my blind spots?

If you ever look again at the VNode class and see that something isn't right I will be glad to answer any question you may have regarding how I see it. But, I won't start the discussion myself. 

If you are willing, please pick just one VNode method, ivar or property that seems not right to you, and tell us why.  I'll do my best to read what you say with an open mind.  Thanks.

Edward

[vitalije]

Oh my.  I do apologize for distressing you.

There is no need that you apologize to me. In fact I am the one who has been rude. 

To me having most basic data element that can't be instantiated on its own is a big issue. While on the other side you think it is not a big deal.

In the comment attached to context ivar it is stated:

self.context = context # The context containing context.hiddenRootNode.

           # Required so we can compute top-level siblings.

           # It is named .context rather than .c to emphasize its limited usage.

However if we examine the VNode class, we would find that c.hiddenRootNode has been mentioned only once: in method findAllPotentiallyDirtyNodes where it is used only to be excluded from the results of this method.

Vitalije: > [The VNode class] also knows indirectly about c.fileCommands.gnxDict.

Edward: To my knowledge, this is not true.  The VNode class does not in any way become entangled with other classes merely because v.context exists! The VNode class knows nothing about how other classes use gnx's.

 

OTOH Let's try to instantiate VNode for the sake of testing, using the mock object for context:

import leo.core.leoNodes as leoNodes

class DummyC:

    def __init__(self):

        self.hiddenRootNode = None

dc = DummyC()

v = leoNodes.VNode(dc, gnx="dewrwer.213213432244")

g.es('ok')

Executing this script we get an error:

AttributeError: 'DummyC' object has no attribute 'fileCommands'

If we add dummy fileCommands and try again, it will report that dummy fileCommands has no attribute 'gnxDict'.

Why I think that this is not a small issue? First it makes testing much harder than it can be. The other reason is that it can be relatively easily avoided. And third it can cause hard to understand bugs.

Let me explain about bugs. There are some places in Leo code where the execution path depends on whether we have a brand new vnode or vnode which is already known (i.e. clone). And test to distinguish those two cases is the presence or absence of the node in c.fileCommands.gnxDict. In some cases it doesn't matter if the node is already attached to the main tree or not. But in some cases it matters. Just instantiating a vnode creates the node instance which is present in gnxDict, but is not part of the tree yet. This can lead to mysterious bugs, that is hard to spot.

There are cases when one might want to have vnode with perhaps some children and grandchildren to keep around in detached state. For example settings whose values are subtrees. Also when building and transforming tree it is sometimes useful to build it outside of the main tree without disturbing gnxDict. 

It also seem to me that the relation between commander and vnode violates logic. There should be no commander without hiddenRootNode and yet node can't be created without commander instance. That causes complications in initialization process which is complex enough on its own.

Now, how this situation could be avoided:

from collections import defaultdict

class VNode(object):

    __pool = defaultdict(dict)

    

    def __init__(self, context, gnx=None):

        # in this case context can be just a string for example c.hash()

        

        # for backward compatibility 

        if isinstance(context, Commander):

            context = context.hash()

            # issue a deprecation warning

        ...

        if gnx is None:

            gnx = make_new_gnx(context)

        if context:

            # passing context = None prevents pool polution

            VNode.__pool[context][gnx] = self

    @staticmethod

    def getGnxDict(ctx):

        return VNode.__pool[ctx]

    

class FileCommands(object):

    ....

    @property

    def gnxDict(self):

        return VNode.getGnxDict(self.c.hash())

This is backward compatible. All scripts that use c.fileCommands.gnxDict can continue to work. However if one needs to build temporary tree of nodes it can be done cleanly and easily. 

Now, copying outline can be a method defined on vnode:

# inside VNode class definition

def to_list(self):

    ua = None

    if self.u:

        ua = base64.encode(pickle.dumps(self.u))

    return [self.gnx, self.h, self.b, ua, self.statusBits,

                [v.to_list() for v in self.children]]

def copyOutline(self, set_clipboard):

    set_clipboard(json.dumps(self.to_list()))

Pasting can be also method of vnode as I have demonstrated it in the prototype recently.

Both, copyOutline and pasteOutline would be completely self contained and the way they work will be completely hidden from the world outside VNode class.

There would be no need for vnode to know anything about classes that are higher up in hierarchy. VNode would be basic level class, that could be used everywhere and anytime without introducing any new inter-dependency with other modules.

We can go even further if we like.

The idea behind gnxDict (as I understand it) is to prevent the possibility to have two Vnode instances with the same gnx. This can be achieved using fly pattern. VNode could keep just a gnx as own ivar, and the rest ivars become properties which keep their values in a private static dict whose keys would be gnx and values could be lists [h, b, u, statusBits, children, parents]

For example v.b would be:

# inside VNode class definition
def _setB(self, b):

    VNode.__pool[self.context][self.gnx][1] = b

def _getB(self):

    return VNode.__pool[self.context][self.gnx][1]

b = property(_getB, _setB)

and the rest of ivars would be implemented the same way.

In that case it would be enough for vnodes to have the same gnx and that would automatically made them clones. No other class should know about pool and gnxDict. If they know gnx they can instantiate new vnode with that gnx wherever and whenever and the node will automatically have all fields as any other node instance with the same gnx. Children and parents lists could contain gnx-es instead of vnode instances. 

This may seem a bit odd at first and that it would be slow. Performance could be experimentally checked. I don't expect this to be much slower especially in python >= 3.5. OTOH, having ability to check equality of two vnodes just by looking in their gnx would allow many other improvements in code. Making connections between nodes, i.e. linking outline can be supervised by the VNode class. It means we would have 100% safe update signals, much easier calculations of changes for the undo/redo code. This capabilities would allow us to simplify undo/redo and also c.selectPosition and of course incremental visual tree updates.

Prototypes that I have made in coffeescript and clojurescript prove this to be very efficient way of displaying tree, making tree selection and updates of body and headline.

Even if these are too big changes to be accepted or even discussed, the first part regarding breaking dependency of VNode to Commander could be done without too much risk or effort, and that alone would allow other parts of code to be simplified. Also it would make VNode class testable (more testable).

I hope that these code examples are more clear than my previous code critics.

Vitalije

[Terry Brown]

FWIW, I think I generally agree with Vitalije. First, seeing
everyone's apologizing ;-) let me apologize for dragging an attempt at
general discussion into the weeds of specifics, I know that's
annoying. I know that the point doesn't rest on the particular history
of uAs or VNodes or whatever.

From a general design principle, I find it extremely jarring to need a
Commands object to instantiate a VNode. This is the (general) question
of layering I've tried to express before.

Back here I was trying to express the idea of layers (I was
calling them levels) within the GUI layer:
https://groups.google.com/d/topic/leo-editor/Cgzy6CYGJ8w/discussion

But the GUI layer itself is just one layer in a higher lever set of
layers I started to try and list a couple of days ago:

- node storage and mutation
- outline storage / representation / manipulation
- outline loading / session stuff (Commands object here?)
- importers here? or as plugins?
- collection of outlines
- maybe now the GUI layer (with sub-layers as in above linked thread)
- plug-ins and FNMW like paste-as-xml

Not suggesting this is complete or optimal or anything. Key point is
that things higher on the list know nothing of things lower on the list.

Nodes knowing about outlines doesn't ruin Leo, perhaps that has no
impact at all from a user perspective. But I do think a lot of the
vertical integration between layers makes it hard to develop Leo,
from the point of view of understanding code flow and working out where
changes can be made.

In my first attempt at using QDocks as a more accessible layout system
to replace free_layout I repeatedly got bogged down in trying to
untangle the GUI initialization code, which has a lot of delayed
initialization and cross linking of components through ivars. In my
current attempt, I've simply side-stepped all that by letting regular
GUI initialization occur, then moving everything around with a delayed
execution callback - far from ideal but the only way I could find to
move forward. As an aside, the current QDocks effort has just returned
from a long and ultimately unfruitful attempt on my part to persist
dock layout, I've finally realized that to restore dock geometry you
can call QMainWindow.restoreState(), and QMainWindow.restoreGeometry()
is just there to confuse you :-}

I think the leo-edit-pane project has struggled for similar reasons.

It seems there's a similar cross linking of layers in the Find
code. I forget the details, but i wanted to get a list of found nodes
and do something with it, but it seemed that what is to be done with
found nodes is passed down into the inner layers of the find code. So
whereas I'd expect clone-find-all, mark-finds, regular user interaction
type find etc. to call a generic find function that returns a list or
generator and is completely ignorant of the client code's use of the
result, the call to the find code includes kwargs etc. indicating what
is to be done with the found nodes, and I would have needed to add my
action in with the existing options. Which doesn't work well for a
plugin, so I just went with a more simplistic recursive find.

I don't think these issues are because Leo is badly designed or
implemented, I think it's because Leo's nearly twenty years old(?) and
has evolved, perhaps from something unusual (a literate code editor) to
something quite unique (a tree based information manager / environment).

So GUI startup for example. Leo used to be a three widget app. - tree,
log, and body. Then over time a bunch more widgets got jammed into the
log pane, and suddenly startup logging and plugins loading widgets is
interrelated. And it works fine, it's just hard to change. Who could
have known, back at the beginning, that managing a bunch more widgets
no one had thought of could be a consideration?

Finally though another point that I think Vitalije also touches on -
these discussions are not unhealthy, but they're not really productive
either, unless there's serious interest in major restructuring.
I think we'd really need to consider whether these changes could be
made within Leo, or more efficiently as LeoII - either way is a scary
amount of work.

Kind of a long email to wrap up by questioning the productivity of the
discussion, but oh well :-)

Cheers -Terry

[vitalije]

I don't think these issues are because Leo is badly designed or 
implemented, I think it's because Leo's nearly twenty years old(?) and 
has evolved,

Precisely. 

vertical integration between layers makes it hard to develop Leo, from the point of view of understanding code flow and working out where changes can be made. 

 

Again, I fully agree. Because we are not sure what and where can be broken when we change something, most often we are fixing bugs using just minor tweaks, adding guards here and there, adding more kwargs. That strategy may suffice for a first-aid. But in long terms it accumulates more and more complexity and over twenty years we have what we have. 

I think it might be wise (at least once in a while) to invest some time in serious refactorings aimed at reducing accumulated complexity. Keeping backward compatibility wherever is possible is great. But I wish that we stay open also for the cases when breaking backward compatibility in certain areas can bring great reduce of complexity. In such situations, perhaps we may ask users to vote or to present their reasons against breaking backward compatibility.

I don't think it is wise to keep backward compatibility forever. If there is a chance to improve code quality or/and its readability by breaking backward compatibility I would suggest keeping it for some limited time, and announcing that it will be dropped in one of the next releases. That way users will see deprecation warnings and they would have time to adjust their code accordingly. They would be able to ask for help on migration to the new API. If after such announcement and in provided period of time some users still keep the old deprecated code in their scripts, I think we shouldn't feel guilty for breaking their scripts at all. That way, project keeps a chance to advance. Otherwise it is bound to the limits of its first version or the limits of some ill turn in its evolution path.

Vitalije

[Edward K. Ream]

On Sun, Apr 29, 2018 at 4:00 PM, Terry Brown <terry...@gmail.com> wrote:

From a general design principle, I find it extremely jarring to need a
Commands object to instantiate a VNode. 

​Interesting. This has never bothered me (before now).
​

 

This is the (general) question

​ ​

of layering I've tried to express before.

Back here I was trying to express the idea of layers (I was
calling them levels) within the GUI layer:
https://groups.google.com/d/topic/leo-editor/Cgzy6CYGJ8w/discussion

But the GUI layer itself is just one layer in a higher lever set of
layers I started to try and list a couple of days ago:

 - node storage and mutation
 - outline storage / representation / manipulation
 - outline loading / session stuff (Commands object here?)
 - importers here? or as plugins?
 - collection of outlines
 - maybe now the GUI layer (with sub-layers as in above linked thread)
 - plug-ins and FNMW like paste-as-xml

Not suggesting this is complete or optimal or anything.  Key point is
that things higher on the list know nothing of things lower on the list.

​Again, interesting.  I've never thought that way.
​

 

Nodes knowing about outlines doesn't ruin Leo, perhaps that has no
impact at all from a user perspective.  But I do think a lot of the
vertical integration between layers makes it hard to develop Leo,
from the point of view of understanding code flow and working out where
changes can be made.

​I think the Find code may be the best example of this.

It seems there's a similar cross linking of layers in the Find
code.

​I agree. 
​

 

I'd expect clone-find-all, mark-finds, regular user interaction
type find etc. to call a generic find function that returns a list or
generator and is completely ignorant of the client code's use of the
result, the call to the find code includes kwargs etc. indicating what
is to be done with the found nodes, and I would have needed to add my
action in with the existing options. 

​Parts of the find code are horrible, because that code has to maintain interactive find state.  That's really really tricky.

However, there is now a super easy way to find anything: c. cloneFindByPredicate.  Yes, it has lots of kwargs, but it's a Swiss Army Knife.  In practice, it works well. See, for example, c.cloneFindMarkedHelper.

Which doesn't work well for a

​ ​

plugin, so I just went with a more simplistic recursive find.

​​

The code of c. cloneFindByPredicate is probably just as simple/simplistic.

I don't think these issues are because Leo is badly designed or
implemented, I think it's because Leo's nearly twenty years old(?) and
has evolved, perhaps from something unusual (a literate code editor) to
something quite unique (a tree based information manager / environment).

​Yes, Leo has evolved, and the VNode class contains some of the most important "evolutionary" details.  I'll explain in great detail in my reply to Vitalije's original post.

So GUI startup for example.  Leo used to be a three widget app. - tree,
log, and body.  Then over time a bunch more widgets got jammed into the
log pane, and suddenly startup logging and plugins loading widgets is
interrelated.  And it works fine, it's just hard to change.  Who could
have known, back at the beginning, that managing a bunch more widgets
no one had thought of could be a consideration?

​Imo, some parts of Leo do suffer from code that has to consider the types of widgets on a case-by-case basis.  The VR plugin is one example, parts of the key handler are another, and as you say, parts of the outline-widget-management code are another.  I don't think we are going to define a base LeoWidget class and use standard Object-Oriented inheritance to eliminate the case statements.  It just isn't worth it.
​

 

Finally though another point that I think Vitalije also touches on -
these discussions are not unhealthy, but they're not really productive
either, unless there's serious interest in major restructuring.

​Let's be very clear here.  These discussions are productive because they expose design and coding details that otherwise would not come to the surface.  As my mentor, Bob Fitzwater, constantly stressed, design is far more important than code, and design is only tangentially apparent in the code.  But some of the code details I'll discuss in my reply to Vitalije are also quite important.

True, I see no reason to change the actual code, for reasons I'll explain fully in my next reply, but that in no way means this discussion is unproductive!

I think we'd really need to consider whether these changes could be
made within Leo, or more efficiently as LeoII - either way is a scary
amount of work.

​It's easy to consider giant reorgs​, but the VNode class will remain exactly as it is.

Kind of a long email to wrap up by questioning the productivity of the
discussion, but oh well :-)

​To repeat, this kind of discussion is extremely valuable even thought the code isn't going to change.

I'm thinking of my legacy now. I want all devs to understand what I think is important.  After I'm gone, you will make your own decisions, but imo it would be a pity if you all decided to rewrite large parts of the code merely because you didn't understand it.

Edward

[Edward K. Ream]

On Mon, Apr 30, 2018 at 2:13 AM, vitalije <vita...@gmail.com> wrote:

Because we are not sure what and where can be broken when we change something, most often we are fixing bugs using just minor tweaks, adding guards here and there, adding more kwargs. That strategy may suffice for a first-aid. But in long terms it accumulates more and more complexity and over twenty years we have what we have.

​Imo, Leo's code base is clean enough, in general, but...

I think it might be wise (at least once in a while) to invest some time in serious refactorings aimed at reducing accumulated complexity.

​You have already simplified important parts of Leo's code. I will welcome further simplifications.​

 
 

Keeping backward compatibility wherever is possible is great. But I wish that we stay open also for the cases when breaking backward compatibility in certain areas can bring great reduce of complexity.

​I am open to that possibility. "Minor" or "interior" parts of Leo's API can probably change with little or no affect on user code.  But some parts of the API are fixed: we aren't going to change the methods of p and c, except in a strictly upward compatible manner.

Having said that, I'm not going to hide behind compatibility in my reply to your post yesterday.  I'll consider all possibilities, regardless of their affect on the API, and see where that leads.

In such situations, perhaps we may ask users to vote or to present their reasons against breaking backward compatibility.

​I agree, but I'm one user who will veto any drastic changes ;-)
​

 

I don't think it is wise to keep backward compatibility forever.

​Imo, this may be the biggest question facing Leo's devs after I am gone.  My first thoughts on this subject are the following:

1. It would be crazy to change the API of the Position and VNode classes.  The payoff would not remotely justify a fork in Leo's code base. Do not go there.​

 

​Imo, it is tragic that python decided to fork itself into python 3.  The pain will be eternal.  There were other alternatives (from future import x).  Forking python was a huge blunder.  Do not repeat that mistake!

2. Similarly, there is little reason to change the API of any Commander (c) methods that correspond directly to user commands.  These methods do one well-defined thing, and existing code might well use them.

3. In general, the API of subcommanders like c.atFileCommands and c.fileCommands, etc, could be changed without much change to user scripts. See c.initObjects for the full list of subcommanders. Do not delete any subcommander,but refactoring is possible within subcommanders.

As I have said several times, refactoring at.read, and indeed all of leoAtFile.py, might be desirable.  But please do not change code just because you might not have written it exactly the same way.  There are far more interesting things to do with Leo.
​

If there is a chance to improve code quality or/and its readability by breaking backward compatibility I would suggest keeping it for some limited time, and announcing that it will be dropped in one of the next releases.

​Sure.  Deprecating method before killing them is desirable.  And even this probably isn't necessary for the only-vaguely-defined helper methods.  It depends on the probability that user code is likely to use low-level helpers.

Summary

When in doubt, leave Leo's API unchanged.  Leo's users will thank you.

Breaking Leonine code is the best way I know to blight Leo's future.

Edward

[vitalije]

1. It would be crazy to change the API of the Position and VNode classes.  The payoff would not remotely justify a fork in Leo's code base. Do not go there.​

 

​Imo, it is tragic that python decided to fork itself into python 3.  The pain will be eternal.  There were other alternatives (from future import x).  Forking python was a huge blunder.  Do not repeat that mistake!

Have you looked in the code examples I gave in my previous message. Moving gnxDict from c.fileCommands to VNode itself and keeping c.fileCommands.gnxDict as a property that links to VNode would be 100% backward compatible and yet it would allow instantiation of VNode without commander instance?

AFAIK no user script should be broken with such refactoring.

Vitalije

[Edward K. Ream]

On Mon, Apr 30, 2018 at 5:50 AM, vitalije <vita...@gmail.com> wrote:

Have you looked in the code examples I gave in my previous message.

​No, I haven't.
​

 

Moving gnxDict from c.fileCommands to VNode itself and keeping c.fileCommands.gnxDict as a property that links to VNode would be 100% backward compatible and yet it would allow instantiation of VNode without commander instance?

​That's fine. I want to encourage simplifications where possible.

Edward

[Terry Brown]

On Mon, 30 Apr 2018 05:07:05 -0500
"Edward K. Ream" <edre...@gmail.com> wrote:

> > Finally though another point that I think Vitalije also touches on -
> > these discussions are not unhealthy, but they're not really
> > productive either, unless there's serious interest in major
> > restructuring.
>
> ​Let's be very clear here. These discussions are productive because
> they expose design and coding details that otherwise would not come
> to the surface.

The perception of productivity is probably partly tied to cost -
discussions like this and previous similar ones often consume 100% of
the time I have available to work on Leo in a day, so they need
valuable tangible outcomes to justify themselves.

Cheers -Terry

[Edward K. Ream]

​​

On Sun, Apr 29, 2018 at 1:51 PM, vitalije <vita...@gmail.com> wrote:

​Ok.  This is the "long" response that I promised in two other responses I have just written in this thread.​

It's really too long, but I want to discuss all the details just once.  This will be pre-writing for a much short LeoU lesson.

To me having most basic data element that can't be instantiated on its own is a big issue. While on the other side you think it is not a big deal.

​Actually, I've never even considered this question.  It's interesting that Terry has the same instinct on this question as you do.
​

 

However if we examine the VNode class, we would find that c.

​​

hiddenRootNode has been mentioned only once: in method findAllPotentiallyDirtyNodes where it is used only to be excluded from the results of this method.

This statement is misleading. In fact, v.context is essential, as a cff on hiddenRootNode shows.

​The low-level p._linkAsRoot accesses c.hiddenRootNode via p.v.context. Remember this fact.

To me, it's obvious that vnodes must know, somehow, about c.hiddenRootNode, so I had better explain in full detail.  You could call this part of my legacy. It will make its way into LeoU.

Performance

You have to go back 25 years to truly understand how good Leo's implementation of outlines is.

Leo's design visual and data design comes directly from the MORE outliner.  But the performance of the MORE (and the first versions of Leo) was O(N**2) when creating or changing clones.  I never saw the MORE code, but the Leo code simply made copies of trees when cloning them. The performance of MORE was similar to Leo's, so it's easy to infer that MORE copied outlines too.

Later versions of Leo did much better, using two kinds of outline nodes (tnodes and vnodes) The present version of Leo collapses all outline data into a single VNode class. All outline operation now have constant, O(1), performance.  Moving or changing a outline node just changes a few "pointers".

I'm reviewing all this because the VNode code is sometimes difficult and complex, but that complexity is what creates the O(1) performance.  In other words, the complexity is a justified optimization.

Why VNodes (and Positions) must know about c.hiddenRootNode

Recall that there is only a single vnode for all clones (all cloned positions).  This can make talking about clones a bit tricky. 

v.parents is the list of all parent vnodes of the node. This list will include c.hiddenRootNode (a vnode) if the node represents a top-level node/position.

So v is a clone if and only if len(v.parents) > 1.  This is an O(1) operation.

But for vnodes, there is no such thing as "the" parent of the node.  Only positions have a unique parent (in a traversal).

p.hasParent() is as follows:

def hasParent(self):
    p = self
    return p.v and p.stack

So bool(p.hasParent()) will be False for top-level nodes, even though p.v is c.hiddenRootNode!  This can't be changed.  Many position methods depend on this fact.

So what, you ask?  I don't see any use of p.v.context.  The answer is, as I mentioned before, that p._linkAsRoot does use p.v.context.

So if any position method needs p.v.context, the v.context ivar is essential.

Yes, we could provide all position methods with a context/c ivar, but that's less useful because of the many-one relationship between positions and vnodes.  Given a position p, p.v is its (unique) vnode.  But the converse is not true.  Given a vnode v, there is no way to recover any position p such that p.v == v.

Heh, the vnode could traverse the entire outline looking for positions p such that p.v == v, but that would require access either to c or the hidden root vnode!

All this is obvious to me. It deserves to be common knowledge.

Alternatives

We could imagine other ways of organizing the position and vnode classes so as to make all positions and vnodes aware of the root of the vnode tree without requiring the Commands class.  For example, code that instantiates either vnodes or positions could pass c.hiddenRootVnode itself instead of c.

As Vitalije points out, this would simplify testing to some degree.  But not enough, imo.  Knowing the commander in which a vnode resides seems completely natural to me.  Vnodes do not live in a vacuum. More importantly, the context/c ivar is exactly the kind of information that we want the VNode class to have.  It's completely general, and completely harmless.  And to repeat, positions require v.context.

We could define a VnodeTree class that would encapsulate the (hidden) root vnode of the tree.  Instantiating the VnodeTree class would indeed be a bit simpler.  But then we would immediately have to connect the VnodeTree instance to the Commands class, so all we would have done would be to add a bit more complexity to the Commands class.  It's all pain and no real gain.

Other comments

There are some places in Leo code where the execution path depends on whether we have a brand new vnode or vnode which is already known (i.e. clone).

​Rev 6fcb92b82d0027 refactors some of this code and considerably simplifies it.  In particular, there is a new ​

fc.updateSaxClone method.​

 

And test to distinguish those two cases is the presence or absence of the node in c.fileCommands.gnxDict. In some cases it doesn't matter if the node is already attached to the main tree or not. But in some cases it matters. Just instantiating a vnode creates the node instance which is present in gnxDict, but is not part of the tree yet. This can lead to mysterious bugs, that is hard to spot.

​I agree that the fc code is difficult.  But we don't have to change the vnode class to do so.​

 

There are cases when one might want to have vnode with perhaps some children and grandchildren to keep around in detached state. For example settings whose values are subtrees. Also when building and transforming tree it is sometimes useful to build it outside of the main tree without disturbing gnxDict. 

​Yes, and the present code does all this.  Iirc, there are commander methods to copy trees.
​

 

It also seem to me that the relation between commander and vnode violates logic. There should be no commander without hiddenRootNode and yet node can't be created without commander instance. That causes complications in initialization process which is complex enough on its own.

​I agree that there are problems, but imo Leo just has to live with vnodes as they are.
​

 

Now, how this situation could be avoided:

​[snip]​

 

This is backward compatible. All scripts that use c.fileCommands.gnxDict can continue to work. However if one needs to build temporary tree of nodes it can be done cleanly and easily. 

​I have no objection to instantiating vnodes with context=None in certain situations.  We just have to ensure that no position is ever going to refer to the resulting vnode.

Pasting can be also method of vnode as I have demonstrated it in the prototype recently.

​I don't have the details in my mind, so I won't comment.
​

 

Both, copyOutline and pasteOutline would be completely self contained and the way they work will be completely hidden from the world outside VNode class.

​I have no objections as long as positions will never use the v.context field.  I suspect that is so in the scheme you propose.

​

We can go even further if we like.

​I'm all for simplifying or eliminating the gnxDict.  That's a separate topic, and a valuable one.

Vitalije, I have faith that you can continue to simplify the code.  Maybe a pull request would be a good next step, perhaps with a post discussing the theory of operation. The shorter the proposal, the more likely it is that I shall understand and approve it.

Summary

The position class must have access to c.hiddenRootNode.  That's why v.context must exist.

It's acceptable to instantiate a vnode with context=None if and only if no position instance will ever refer to it.  Let's call such vnodes positionless vnodes. This is a new distinction in Leo's history. Imo, it's worth all the discussion so far.

Positionless vnodes may be useful when cutting/pasting trees.  They may also help simplify the present ugly gnx-related logic.

Edward

[Edward K. Ream]

On Mon, Apr 30, 2018 at 7:16 AM, Terry Brown <terry...@gmail.com> wrote:

The perception of productivity is probably partly tied to cost -
discussions like this and previous similar ones often consume 100% of
the time I have available to work on Leo in a day, so they need
valuable tangible outcomes to justify themselves.

​I agree completely.

The present conversation will lead to one or more new LeoU lessons.  That will encourage people to become devs.  That's vital to Leo's future.

Simplifying Leo's code is almost always valuable, but having said that, it's important to wrap up this present code-oriented phase/obsession and move on to the bigger questions about what Leo is to become.

Edward