[ jEdit-devel ] [ jedit-Plugin Bugs-1693147 ] PerlSideKick: sub nodes contain invalid length

[SourceForge.net <no...@so...>]

Plugin Bugs item #1693147, was opened at 2007-04-02 20:40
Message generated for change (Comment added) made by hertzhaft
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=565475&aid=1693147&group_id=588

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Alan Ezust (ezust)
Assigned to: Martin Raspe (hertzhaft)
Summary: PerlSideKick: sub nodes contain invalid length

Initial Comment:
When using "tree follows caret" it is important that sidekick nodes have the proper length, otherwise when you click on a node in the tree, it jumps the cursor to the position in the editpane but then the selected node of sidekick gets reset to the root, as exhibited by PerlSideKick 0.6.




----------------------------------------------------------------------

>Comment By: Martin Raspe (hertzhaft)
Date: 2007-05-09 09:31

Message:
Logged In: YES 
user_id=1037294
Originator: NO

I can only second what Shlomy said :). Perl Code is not rarely composed of
subs which belong to different packages, like so:

Package1::sub1 { # code follows here }

sub2 { # package Main; code ... }

Package3::sub3 { # ... }

BTW, in Javascript similar constructs are possible, if you work with
prototype methods  (and probably in other languages).

Subs can be freely mixed, you can define packages and package subs
everywhere and repeatedly. In SideKick (I feel) all the subs belonging to
Package1 should be subsumed under a parent asset "Package1", but this asset
just has no "body" (so far assets need to be contiguous and can't have a
multi-part body). So where should the cursor jump when you click on the
Package1 asset? And where should the tree highlight go when the cursor is
between subs? I think it should go up to the framing package (it used to do
so), and if there was none, go to the "main" package (which it did).

It is not true that all assets have no length in PerlSideKick. All assets
with corresponding code have a starting and end position. Only "bodyless"
assets are empty. Before the "simplification" I had
getTreePathForPosition() look recursively for the most specific asset (=
farthest down in the hierarchy) with a corresponding range, not caring for
empty parents. Now it cuts short, hence the bug.


----------------------------------------------------------------------

Comment By: Matthieu Casanova (kpouer)
Date: 2007-05-09 09:12

Message:
Logged In: YES 
user_id=285591
Originator: NO

Ok I think that if the cursor is not inside an asset, I can fix Sidekick
to make him select the closest asset, it is better than nothing.
But only the next asset or the closest, next or previous asset ?
About the end asset, I don't remember enough about perl to know how to
find the end asset, but only the Perl parser can do that, the SourceParser
cannot guess where it is automatically

----------------------------------------------------------------------

Comment By: Dale Anson (daleanson)
Date: 2007-05-09 04:41

Message:
Logged In: YES 
user_id=187628
Originator: NO

Alan asked me to comment on this, so here goes.  Let me say up front that
my Perl skills are woefully out of date, the last serious Perl work I did
was probably 10 years ago, but I still regularly review Perl code written
by others.  Anyway, just looking at the code, nothing is setting the end of
the Assets in PerlParser.  It's delegating to SourceParser (in SideKick) to
complete the Asset, but it's just reusing the start Position as the end
Position.  So the cursor never falls within an Asset.  As I recall,
SideKick checks the cursor location, asks the SideKickParsedData for the
buffer to provide the Asset for that location, and highlights that Asset in
the tree.  The actual order of the tree as displayed doesn't matter (for
example, you can set JavaSideKick to sort the nodes by order in the file,
alphabetical by name, or by visibility, but clicking in the file still
highlights the correct node in the tree), as long as the Assets have a
start and end position.  

I recall 'fixing' SideKick so that if the user clicked between Assets in a
file, it would highlight the Asset immediately following the cursor
position.  This was an easy to highlight a method in a Java file then the
user clicked between methods (either on a blank line or on a comment
preceding a method since I don't treat comments as an individual asset)
without causing the highlight to go to the class name which was actually
the Asset currently containing the cursor location.  I don't think this
would cause a problem with PerlSideKick, although it seems that regardless
of where I click in the attached example, only the top/root node of the
tree is highlighted.

The problem with Perl (okay, no problems with Perl itself, it's a great
language) is that it's hard to adequately describe the end of an Asset. 
Finding the start seems to be easy enough.  Beats me how you might find the
end.  I think if you can find the end, and get SourceParser to set that
end, the problem would be fixed.

----------------------------------------------------------------------

Comment By: Matthieu Casanova (kpouer)
Date: 2007-05-08 22:43

Message:
Logged In: YES 
user_id=285591
Originator: NO

Ok I understand, so Sidekick must go through the entire asset tree to find
the good asset. But how to know which asset is the good one ? Is it the
shortest asset that match the caret offset ?
Anyway that doesn't change the fact that the parsers must provide a start
and an end offset, not only a start offset

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2007-05-08 22:19

Message:
Logged In: YES 
user_id=1477607
Originator: NO

CtagsSideKick is a SideKick plugin that shows a tree based on the output
of the Ctags tool (it's a descendant of CodeBrowser).
Sometimes, it is a lot more convenient (and sensible) for the user to see
a tree structure that does not reflect the structure of the buffer. For
example, a C++ header file (.h) might look like:
class A {
 void f();
 void g() {}
 ...
};
class B {
 void f();
 ...
};
void A::f() {
}
void B::f() {
}
... and many more ...
Even if I could present a tree that reflects exactly this structure (I
can't because of Ctags limitations), it is usually more convenient for the
user to have all methods of a class grouped under the class name, no matter
where they are located in the buffer, so that classes can be easily folded
and unfolded, and to make searching for a specific method easier. So, the
node representing the definition of A::f() is a child of the 'A' node, even
though in the buffer it is actually outside the class declaration (this is
optional in CtagsSideKick, btw, so if you don't want this behavior, you can
change it).

CtagsSideKick provides options to determine the layout of the tree, so the
tree is in many cases a "logical" one, which is more convenient to use, and
not a "physical" one that reflects the structure of the buffer. The
"logical" tree may contains nodes that do not have an associated asset at
all and are only there to improve the usability.
CtagsSideKick provides its own SideKickParsedData derivation to make it
work, and I also changed something in SideKick itself to support "logical"
trees, but I made sure my changes to SideKick did not affect any plugins
that do maintain the property of reflecting the structure of the buffer.
You can see the CtagsSideKick source code to learn how it works.

----------------------------------------------------------------------

Comment By: Matthieu Casanova (kpouer)
Date: 2007-05-08 20:51

Message:
Logged In: YES 
user_id=285591
Originator: NO

In fact the code assumes that the child node must be inside the parent
node.
Acording to you, it may be different and sometimes a node could be a child
of another node but could not be inside his parent in the buffer. In fact I
do not understand one thing : if an asset is not a child of another asset,
why is it a child in the Sidekick tree ? why not making a brother node
instead ?
I'm not very familiar with Perl, could you provide an example to make me
understand ?

And there is still another problem with PerlSidekick and maybe some other
sidekicks plugins :
the assets do not have a start and a end : all of them have the start and
end at the same position, so the caret can never be inside an asset. I
think this must be fixed too

----------------------------------------------------------------------

Comment By: Martin Raspe (hertzhaft)
Date: 2007-05-08 09:45

Message:
Logged In: YES 
user_id=1037294
Originator: NO

The problem is the same as before: The current code assumes that the
parent-child structure of nodes in SideKick must match the document order
of nodes. This is not the case in Perl and other languages where code
belonging to one package frequently is mixed with code belonging to other
packages. The sidekick tree is worthless for Perl if you can't group
together those code sections.

This tree structure has to be provided by SideKick (and it did so far).
Otherwise plugins have to roll their own SideKickParsedDate derivation.
BTW, the simplification was done by Matthieu (not Dale) and had since to be
fixed twice by Shlomy.

----------------------------------------------------------------------

Comment By: Alan Ezust (ezust)
Date: 2007-05-07 20:27

Message:
Logged In: YES 
user_id=935841
Originator: YES

just because it worked before, doesn't mean it was correct before. 

The getTreePathForPosition() was reworked by Dale Anson because it there
were lots of off-by-1 errors with folding before.

I don't want to blindly revert back to a previous version of that function
unless the SideKick folding is verified to be working correctly with latest
Java and XML plugins.

Dale, can you comment?




----------------------------------------------------------------------

Comment By: Martin Raspe (hertzhaft)
Date: 2007-05-07 19:01

Message:
Logged In: YES 
user_id=1037294
Originator: NO

I currently haven't got much time to delve more deeply into this matter,
but I think the bug must be caused by the "simplification" to
SideKickParsedData/getTreePathForPosition() in rev. 8591. I remember that I
reworked this method before because it had been over-simplified. In Perl
assets can't be parented according to document order, because you can have
subs belonging to different packages mixed all over the file (I commented
on this on March 21, 2006 on this list, see
http://article.gmane.org/gmane.editors.jedit.devel/9222/).

I propose to revert the simplification in
SideKickParsedData/getTreePathForPosition(). I think this shouldn't do any
harm, just restore the status quo ante. I can do it, if nobody objects.

----------------------------------------------------------------------

Comment By: Alan Ezust (ezust)
Date: 2007-05-07 08:15

Message:
Logged In: YES 
user_id=935841
Originator: YES

It is possible that some off by one errors were present in the previous
version of  SideKick which caused problems with Folding. I believe those
bugs were fixed, and the need to use the preceding line instead of the
actual start line is no longer necessary for the tree follows caret bug, I
THINK. but can you please try to make perlsidekick work properly with the
current SideKick, or else be more specific about what kind of bug you
consider a regression?



----------------------------------------------------------------------

Comment By: SourceForge Robot (sf-robot)
Date: 2007-05-05 04:20

Message:
Logged In: YES 
user_id=1312539
Originator: NO

This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).

----------------------------------------------------------------------

Comment By: Martin Raspe (hertzhaft)
Date: 2007-04-20 14:36

Message:
Logged In: YES 
user_id=1037294
Originator: NO

This seems to be a recent regression in SideKick. I didn't change the
corresponding code in the meantime, and PerlSideKick 0.6 works correctly
with SideKick 0.7.1. If you check the "show status window" option, you can
see that the "sub" nodes have the proper length (they have to start on the
preceding line after the last return character, otherwise the Sidekick tree
would not follow correctly if you were on the line of the sub just in front
of the "sub" keyword).

Has the asset API changed in the meantime? I couldn't find any
documentation and no obevious change that could have caused this. I vaguely
remember that we had a similar bug before, when the "tree follows caret"
algorithm was changed without taking care of the fact that the tree does
not necessarily reflect the document order of assets.



----------------------------------------------------------------------

Comment By: Alan Ezust (ezust)
Date: 2007-04-03 19:02

Message:
Logged In: YES 
user_id=935841
Originator: YES

consubs.pl is an example that shows the "sub" nodes do not have a valid
length.

Another observation I made is that while "use" directives correspond to
nodes in the perl sidekick,
those nodes also do not have a valid asset length.

You need sidekick's "tree follows caret selection" in sidekick's options
enabled to see this behavior.



File Added: consubs.pl

----------------------------------------------------------------------

Comment By: Martin Raspe (hertzhaft)
Date: 2007-04-03 10:28

Message:
Logged In: YES 
user_id=1037294
Originator: NO

Sorry, Alan, I don't see that behaviour here - could you post an example
file with an indication where it happens, please?

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=565475&aid=1693147&group_id=588