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

Bug 123895

Summary:	[api] Should DocumentEvent getText ever return null?
Product:	[Eclipse Project] Platform	Reporter:	David Williams <david_williams>
Component:	Text	Assignee:	Dani Megert <daniel_megert>
Status:	VERIFIED FIXED	QA Contact:
Severity:	minor
Priority:	P2	CC:	david_williams
Version:	3.1.1
Target Milestone:	3.3 M3
Hardware:	PC
OS:	All
Whiteboard:

Description David Williams

2006-01-14 22:40:09 EST

I think according to 
http://www.eclipse.org/articles/Article-API%20use/eclipse-api-usage-rules.html

methods should not return null unless their java doc says they can? 

ssems trivial to init the text field to "" (or, be explicit in the java doc)? 

we've seen it return null on "delete line" operation. 

my preference would be to return empty string, but, I'm just opening this 
to be obsessive :) 
Either way is fine.

Comment 1 Tom Hofmann

2006-01-16 02:46:08 EST

Neither IDocument::replace nor ITextStore::replaces spec null as an acceptable argument either... 

IMO: In order to be consistent (the DocumentEvent should carry the same information as provided to the call to replace()), and because we cannot tighten the API of IDocument, we should clarify the API and state that it accepts null / that the event's text may be null.

Comment 2 Dani Megert

2006-01-16 05:20:26 EST

The Javadoc is correct: the idea is that it should not contain null. We will fix the places in our code that cause the 'null'.

I agree with Tom, that we cannot convert a 'null' being (incorrectly) passed to replace(...) or set(...) into "" and for compatibility we can also not add an assert either ==> DocumentEvent.getText() might still return 'null' in the future if someone uses the IDocument.replace/set methods against the spec.

Comment 3 Dani Megert

2006-03-21 09:58:06 EST

Has to be shifted to 3.3.

Comment 4 David Williams

2006-10-12 15:46:55 EDT

Thanks for the clarifying remarks. 

As for the cases where someone incorrectly passes in null, I see your point that changing it to assert on null might be bad for compatibility, but you might want to consider a "debug option" where it would assert on null if the "assert_on_null" debug option was turned on. 

While this doesn't really help avoid the _possibility_ that getText might return null (since not everyone would be required to use that debug option) it might be useful to have so those that want to try and improve their code (including yourselves and ourselves, of course :) , could turn it on during development and self hosting to help catch cases where null was incorrrectly passed in. 

Just a thought. (Seems this would be a relatively common problem ... where the spec is desired to be tightened, but without literally breaking (past) violators).

Comment 5 Dani Megert

2006-10-13 02:18:59 EDT

>"debug option" where it would assert on null if the
>"assert_on_null" debug option was turned on. 
Too late ;-) I already added three debug options in my workspace for
DocumentEvent(...), IDocument*.replace(*) and IDocument*.set(*).

I found quite some places where 'null' is used in our tests and a handful of places in real code.

Comment 6 Dani Megert

2006-10-13 03:49:26 EDT

Fixed the places in our code and initialized the field to "". I only added the debug option for the document event because the other two places run through that code anyway.

Note that it is not a normal debug option that can be set via launcher but a system property that needs to be set to 'true'. This is done that way because eclipse.text doesn't depend on core.runtime.

Comment 7 Benno Baumgartner

2006-10-31 09:27:52 EST

verifying...

Comment 8 Benno Baumgartner

2006-10-31 09:56:38 EST

verified in I20061031-0656

Checked that the debug option works (I will keep it enabled) and that we do not directly instantiate DocumentEvent with a null text by looking at callers of the constructor.