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