Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 325438 - [typing] TextViewer.shift(...) does not shift last line, if it is empty.
Summary: [typing] TextViewer.shift(...) does not shift last line, if it is empty.
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 3.6   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.7 M3   Edit
Assignee: Markus Schorn CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 324838
  Show dependency tree
 
Reported: 2010-09-16 07:15 EDT by Markus Schorn CLA
Modified: 2010-11-04 04:52 EDT (History)
2 users (show)

See Also:

Attachments
fix (1.03 KB, patch)
2010-09-16 07:25 EDT, Markus Schorn CLA 		no flags Details | Diff
updated fix (1.99 KB, patch)
2010-09-16 11:50 EDT, Markus Schorn CLA 		daniel_megert: iplog+
daniel_megert: review+
Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Schorn CLA 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 CLA 2010-09-16 07:25:37 EDT 
Created attachment 179021 [details]
fix
Comment 2 Dani Megert CLA 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 CLA 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 CLA 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 CLA 2010-09-17 05:08:59 EDT 
>Available in builds > N20100917-2000.
Available in builds >= N20100917-2000.
Comment 6 Markus Schorn CLA 2010-11-04 04:52:15 EDT 
Thanks, I have verified the fix with the CDT editor using 3.7M3.