[Fenfire-dev] Mudyc's merge request

[Tuomas Lukka]

Overall, this seems a really good patch.

There's a LOT of details that need to be fixed though.
Could you try to fix these without adding any extra work
(e.g. make another branch where you work on stuff beyond
these fixes) so that we can get you synched up?


RDFActions: I'd prefer "globalactions" or something like
that. actions/rdf.py isn't really clear to me at least.

Extensions: why not use a map in python for which extensions
are enabled - a static "if" statement is not as fun. 

Also, this should probably be in the main FenPDF object

So:

        def useExtension(self, extname):
                return self.extensions.get(extname, 0)

ParallelView2D:

        /** Array of parallel View2D intances enclapsed
         *  to one View2D. The first item in array is
         *  the most meaningful, i.e., it determines the size
         *  and it is first asked to return a child view2d.
         *  The simple idea of parallelism is to have only
         *  one instance of no meaningful View2D instances
         *  by sharing them for different views.
         */

Number one: "ParallelView2D" is an unclear name.
How about "View2DDecorator" or "View2DList".

What is this class used for? Explaining that as an example
would be a great step forward in the javadoc.

Overriding the javadocs of the methods in View2D may or may
not be good.


"PlaneContent"?

We were discussing this piece of architecture but I really
don't think this interface and implementation are a good idea.
What I meant is that AbstractMainNode should have an extra
variable, 

        /** The View2D that is the logical "content" of
         * the plane rendered by this MainNode.
         */
        View2D planeContentView2D;

and (with corresponding javadocs)

        View2D getPlaneContentView()

Making an extra object that still uses the "getChildView" trick
to get the view isn't IMHO a good solution.


The changes to fenpdf10.py: Very good


Then:

        --- orig/org/fenfire/fenpdf/actions/eventgrabs.py
        +++ mod/org/fenfire/fenpdf/actions/eventgrabs.py
        @@ -136,6 +137,12 @@
                 self.createFlyingNode(vs, self.node, self.ev, self.fenPDF)
                 self.drewMainNode = mainNode

        +        viewFunction = 
self.fenPDF.views.getMultiplexerNodeContentFunction()
        +        placeable = viewFunction.f(self.fenPDF.fen.graph, self.tipNode)
        +        cs = vs.orthoBoxCS(0, 'TipNode', -1000,50,50, 1,1,
        +                           placeable.getWidth(), placeable.getHeight())
        +        placeable.place(vs, cs)
        +

             def eventGrabber(self, ev, vs):
                 self.ev = ev

What in the world does that do? I have no idea what a tipnode is. Could you
add some comment explaining what this is.


+            if 0:
+                p('start')
+                for i in range(10000):
+                    node = self.getNodeOnPlane(vs, ev, self.node)
+                p('done')

You might leave this out...

+    def hideTipNode(self, vs):
+        cs = vs.matcher.getCS('TipNode')
+        if cs < 2: return
         vs.coords.setOrthoBoxParams(cs, 0, 0,0,0,0,0,0)

Docstring please. What is a tip node?

actions/keyboard.py: good

actions/mouse.py:

adds methods getNodeCS(), getNodeOnPlane, getNodeCSbyNode

There are three methods that look very similar; docstrings
wouldn't hurt at all.


fenpdf.py:

        +class Cursor(ff.view.lava.Cursor):
        +    """ The cursor in text or something else.
        +    """
        +    def __init__(self, alphContent, multiplexer):
        +        ff.view.lava.Cursor.__init__(self, alphContent)
        +        self.multiplexer = multiplexer
        +    def setAccursed(self, node):
        +        ff.view.lava.Cursor.setAccursed(self, node)
        +        set = java.util.HashSet()
        +        set.add(node)
        +        self.multiplexer.setMultiplexerNodes(set)
        +

I'd like a somewhat longer docstring for anything that goes into fenpdf.py.

It should answer:
        What "something else"?
        How does fenpdf use this?
                - how many instances are there of this class
                - what is it used to represent


fenpdfcontext:

I'm all for refactoring things out of context, you did 
very well there, good work.

Then:

+    /** An interface for painter of selection.
+     */
+    private interface SelectionPainter {
+

That's the type of javadoc that should be avoided. If it
doesn't add any information to the "interface SelectionPainter",
it needs to be changed.

It should answer

        Who calls it?
        What for?
        When?
        What different implementations can there be?

Since it's a private interface, I won't go into the
details of the method javadocs.

NodedView2D: I pointed out the javadoc's bad, you pointed
out it's mine ;) 

So I'll can the main javadoc but you need to fix that the way
to get a node from a view and some coordinates is not
explained anywhere. Only the node coordinate system.

Language:
+    /** Return the node's coordinate system (or -1)
+     *  pointed to by a mouse click.

Should be:

Return the node coordinate system that is at the given coordinates,
or -1 if none.

FastView: VERY good javadoc changes! Excellent!

papercanvas2d.test: good, you also fixed it to use the correct
structure stuff. The only problem I had was that the method
of getting the node that is used at the end is explained nowhere.

        Tuomas