Re: LeoU: Formal code review of vnode class

[Edward K. Ream]

On Mon, Apr 30, 2018 at 7:16 AM, Terry Brown  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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[Edward K. Ream]

​​
On Sun, Apr 29, 2018 at 1:51 PM, vitalije  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 ne

Re: LeoU: Formal code review of vnode class

[Terry Brown]

On Mon, 30 Apr 2018 05:07:05 -0500
"Edward K. Ream"  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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[Edward K. Ream]

On Mon, Apr 30, 2018 at 5:50 AM, vitalije  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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[Edward K. Ream]

On Mon, Apr 30, 2018 at 2:13 AM, vitalije  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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[Edward K. Ream]

On Sun, Apr 29, 2018 at 4:00 PM, Terry Brown  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 ev

Re: LeoU: Formal code review of vnode class

[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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[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 

Re: LeoU: Formal code review of vnode class

[Edward K. Ream]

On Sun, Apr 29, 2018 at 4:54 AM, vitalije  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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[Edward K. Ream]

​​
​On Sat, Apr 28, 2018 at 6:46 AM, vitalije  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 aboutthe 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

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

Re: LeoU: Formal code review of vnode class

[vitalije]

O.k. No more complains from me.
Vitalije

-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to leo-editor@googlegroups.com.
Visit this group at https://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.

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 ob