Bug 434135 - frame.charEnd ignored for SourceLookUpFacility.positionEditor
Summary: frame.charEnd ignored for SourceLookUpFacility.positionEditor
Status: REOPENED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 4.4   Edit
Hardware: PC Windows NT
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-Debug-Inbox CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords: helpwanted
Depends on:
Blocks: 434199
  Show dependency tree
 
Reported: 2014-05-05 13:39 EDT by Ed Willink CLA
Modified: 2021-12-27 05:28 EST (History)
2 users (show)

See Also:
daniel_megert: review-


Attachments
Selection Range provided for editor positioning (1.06 KB, patch)
2014-05-09 02:22 EDT, Sarika Sinha CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Willink CLA 2014-05-05 13:39:10 EDT
The Eclipse debugger has a nice ability to work with text ranges or lines as the current context.

However the support for text ranges is deficient in SourceLookUpFacility.positionEditor

Surely:

int charStart = frame.getCharStart();
if (charStart >= 0) {
	editor.selectAndReveal(charStart, 0);
	return;
}

should be

int charStart = frame.getCharStart();
if (charStart >= 0) {
	editor.selectAndReveal(charStart, frame.getCharEnd() - charStart);
	return;
}

so that the editor and outline can exploit the prevailing selection range?
Comment 1 Ed Willink CLA 2014-05-06 07:09:56 EDT
Confirmed:

With an XText-style Outline selection policy of the deepest child that covers the selection range, the failure to provide the true selection range cuases erroneous outline selections.

Providing the missing length and the outline tracks the selection.
Comment 2 Sarika Sinha CLA 2014-05-09 02:22:09 EDT
Created attachment 242880 [details]
Selection Range provided for editor positioning

Selection range is now passed as input to reveal the position of editor.

JDT Debug has no impact because of this as frame.getCharEnd() and frame.getCharStart() is set to -1 .
Comment 3 Dani Megert CLA 2014-05-09 03:20:07 EDT
The current code was done on purpose, see bug 263881 for details.

We can either close this bug here or move it to 4.5 for deeper investigation.
Comment 4 Ed Willink CLA 2014-05-09 03:36:18 EDT
(In reply to Dani Megert from comment #3)
> The current code was done on purpose, see bug 263881 for details.

I don't see the relevance of that bug which was about getCharStart() and the existence of a selection.

This bug is about getCharEnd() and the length of the selection.
Comment 5 Dani Megert CLA 2014-05-09 03:42:11 EDT
(In reply to Ed Willink from comment #4)
> (In reply to Dani Megert from comment #3)
> > The current code was done on purpose, see bug 263881 for details.
> 
> I don't see the relevance of that bug which was about getCharStart() and the
> existence of a selection.
> 
> This bug is about getCharEnd() and the length of the selection.

The fix for the other bug replaced the length with 0. The fix proposed in this bug here will revert that change, bringing back the other bug.
Comment 6 Ed Willink CLA 2014-05-09 05:28:08 EDT
(In reply to Dani Megert from comment #5)
> (In reply to Ed Willink from comment #4)
> > I don't see the relevance of that bug which was about getCharStart() and the
> > existence of a selection.

I assumed that the attachment was the fix. Wrong.

> The fix for the other bug replaced the length with 0. The fix proposed in
> this bug here will revert that change, bringing back the other bug.

I don't actually know what 'annotate' means in this context so I can only observe that stepping to some context selects that context which is why the selection propagates onward to influence the Outline which AFAIK responds only to the selection.

So it seems to me that the previous fix was inadequate, since it corrupts the selection passed to the Outline (and no doubt other listeners).
Comment 7 Dani Megert CLA 2014-05-09 05:35:27 EDT
(In reply to Ed Willink from comment #6)
> So it seems to me that the previous fix was inadequate, since it corrupts
> the selection passed to the Outline (and no doubt other listeners).

This might well be and hence needs a closer look as indicated in comment 3. But since the problem is not "new" we won't change this during RC*.
Comment 8 Michael Rennie CLA 2014-05-09 11:12:13 EDT
(In reply to Dani Megert from comment #7)
> (In reply to Ed Willink from comment #6)
> > So it seems to me that the previous fix was inadequate, since it corrupts
> > the selection passed to the Outline (and no doubt other listeners).
> 
> This might well be and hence needs a closer look as indicated in comment 3.
> But since the problem is not "new" we won't change this during RC*.

One solution here could be to make this a presentation option, that way each debugger could specify how they want the look up to select and reveal.

See IDebugModelPresentation and its variants.
Comment 9 Sarika Sinha CLA 2014-11-05 04:41:51 EST
(In reply to Michael Rennie from comment #8)

I agree with Mike, Unless we provide different ways to present , it will be difficult to  justify both the scenarios .
Comment 10 Dani Megert CLA 2014-11-11 13:11:33 EST
(In reply to Ed Willink from comment #6)
> I don't actually know what 'annotate' means in this context so I can only
> observe that stepping to some context selects that context which is why the
> selection propagates onward to influence the Outline which AFAIK responds
> only to the selection.

Hi Ed, the Debug framework creates an annotation for the current instruction pointer (the look can be configured on the 'Annotations' preference page) and that's what gets shown/highlighted in the editor. If we would set the selection, then that would just hide the annotation (see attached screen shot and read bug  263881). Do you not see that green annotation in your editor? If do, would it be OK to hide it with the selection? This could only be achieved with new API as already mentioned in comment 8.
Comment 11 Ed Willink CLA 2014-11-27 08:23:19 EST
(In reply to Dani Megert from comment #10)
Interesting to know about the preference. Yes I see it.

You are right that my simple fix causes selection and highlight to collide.

But we should be talking about different things. The editor view is fine, it's the outline view that's a problem.

The Outline listens to the selection in order to show correct content. But the character range selection has been corrupted, so the outline malfunctions.

e.g. if I have an parent outline node for characters 100 to 110 with a child outline node for 100 to 105, I can select 100 to 110 in the editor to display the parent. But the outline selection is truncated to 100 to 100 causing the child to be highlghted in the outline.

(In reply to Dani Megert from comment #3)
> The current code was done on purpose, see bug 263881 for details.

I don't fully understand the report, but it seems to me that the original report was erroneous.

No doubt that a line-based debugger that return a > -1 charStart malfunctions, but the solution is not to debilitate the range-based debugger to compensate. It seems to me that the erroneous > -1 charStart return from a line-based debugger should be corrected.

(In reply to Dani Megert from comment #3)
> We can either close this bug here or move it to 4.5 for deeper investigation.

Given that my simple fix causes selection and highlight to collide, it would seem that the failure to distinguish range-based and line-based debuggers is more deep seated. Deeper investigation clearly needed.
Comment 12 Ed Willink CLA 2014-11-27 09:14:06 EST
(In reply to Ed Willink from comment #11)
> But we should be talking about different things. The editor view is fine,
> it's the outline view that's a problem.

Perhaps I'm just wrong in thinking that since the editor selection normally moves the outline, then the debugger in moving the editor and so the outline should do so via the selection.

Should the outline view listen to both selection and AnnotationModel?
Comment 13 Dani Megert CLA 2015-01-12 07:17:23 EST
(In reply to Ed Willink from comment #12)
> (In reply to Ed Willink from comment #11)
> > But we should be talking about different things. The editor view is fine,
> > it's the outline view that's a problem.
> 
> Perhaps I'm just wrong in thinking that since the editor selection normally
> moves the outline, then the debugger in moving the editor and so the outline
> should do so via the selection.
> 
> Should the outline view listen to both selection and AnnotationModel?

In your case you could workaround the issue in your outline page by checking whether there's a debug annotation starting at column 0 and if so, extend the selection in your outline page.

At the moment I don't see us working on this bug.
Comment 14 Eclipse Genie CLA 2019-12-08 12:38:38 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 15 Sarika Sinha CLA 2019-12-08 23:40:58 EST
If someone wants to work on this bug.
Comment 16 Eclipse Genie CLA 2021-12-27 03:31:24 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug.

If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 17 Ed Willink CLA 2021-12-27 05:28:48 EST
Last comment from a committer was an invitation for contribution.

helpwanted, OPEN seems the appropriate status.