From: SourceForge.net <no...@so...> - 2007-05-09 07:54:56
|
Plugin Bugs item #1693147, was opened at 2007-04-02 21:40 Message generated for change (Comment added) made by shlomy 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: Shlomy Reinstein (shlomy) Date: 2007-05-09 10:54 Message: Logged In: YES user_id=1477607 Originator: NO In case the tree is "logical" and does not reflect the structure of the buffer, I think it should be up to the SideKick parser to specify the policy, either by overriding some basic methods (like in CtagsSideKick) or by providing a few options that the parser can pass to SideKick. In the case of CtagsSideKick, for example, each asset node has a unique asset - no two assets overlap (as far as I remember), so it would be enough to look for the first matching node. If assets can overlap, it should be up to the parser (depending on how it built the tree) to resolve the ambiguity. To address two other issues mentioned here: 1. CtagsSideKick provides some purely logical nodes - which are non-asset nodes. Clicking on them will not take you anywhere in the buffer, there is support for this in SideKick. 2. Ctags provides only start positions for objects, so CtagsSideKick simply uses the beginning of the next object to set the end position of the current one. ---------------------------------------------------------------------- Comment By: Martin Raspe (hertzhaft) Date: 2007-05-09 10: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 10: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 05: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 23: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 23: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 21: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 10: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 21: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 20: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 09: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 05: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 15: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 20: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 11: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 |