Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 325438

Summary:

[typing] TextViewer.shift(...) does not shift last line, if it is empty.

Product:

[Eclipse Project] Platform

Reporter:

Markus Schorn <mschorn.eclipse>

Component:

Text

Assignee:

Markus Schorn <mschorn.eclipse>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

aleherb+eclipse, daniel_megert

Version:

3.6

Target Milestone:

3.7 M3

Hardware:

OS:

Windows XP

Whiteboard:

Bug Depends on:

Bug Blocks:

324838

Attachments:

Description	Flags
fix	none
updated fix	daniel_megert: iplog+, daniel_megert: review+

Description Markus Schorn

2010-09-16 07:15:14 EDT

The bug can be explored using the Java Editor or the C Editor:
Select a region, where the last line of the selection is an empty line, that does not even contain spaces or tabs. Using 'Toggle Comment' the last line of the selection is not commented. Using 'Toggle Comment' again the region is not uncommented, rather than that additional comments are added.

The reason for the behavior is that 'TextViewer.shift(...)' adjusts the selection via TextViewer.getTextBlockFromSelection(..), which drops the last line. The decision whether commenting or uncommenting has to be performed is done in the class ToggleCommentAction, which does check the last line.

While the fix could be done in ToggleCommentAction (of JDT and CDT), to me it looks like TextViewer.shift(..) is the faulty part.

Comment 1 Markus Schorn

2010-09-16 07:25:37 EDT

Created attachment 179021 [details]
fix

Comment 2 Dani Megert

2010-09-16 11:06:51 EDT

Thanks for tracking this down Markus. The patch goes into the right direction though I don't like coding by exception too much. Can you please handle that case in regular code and also add your credentials to the copyright
notice of the file in the following form:
name <e-mail> - bugzilla summary - bugzilla URL
e.g.
Dani Megert <dani@eclipse.org> - this is a bug - https://bugs.eclipse.org...

Thanks!

Comment 3 Markus Schorn

2010-09-16 11:50:08 EDT

Created attachment 179040 [details]
updated fix

The new patch no longer relies on a caught exception. In addition to that I have removed the outer catch clause in getTextBlockFromSelection(..), because returning 'null' would just trigger a NPE in the calling method, whereas the BadLocationException is handled there.
If you don't like it that way feel free to adjust my patch before applying it.

Comment 4 Dani Megert

2010-09-17 04:42:48 EDT

(In reply to comment #3)
> In addition to that I
> have removed the outer catch clause in getTextBlockFromSelection(..), because
> returning 'null' would just trigger a NPE in the calling method, whereas the
> BadLocationException is handled there.
Good catch!

Patch committed to HEAD.
Available in builds > N20100917-2000.

Comment 5 Dani Megert

2010-09-17 05:08:59 EDT

>Available in builds > N20100917-2000.
Available in builds >= N20100917-2000.

Comment 6 Markus Schorn

2010-11-04 04:52:15 EDT

Thanks, I have verified the fix with the CDT editor using 3.7M3.