Comment 1 Dani Megert 2004-03-16 04:02:34 EST

re: 1 - we know there's a problem with that shape (bug 39600, bug 39205)

2- what are the changes in SWT and what is the scenario that shows the bug? Can
you give us a preview? As you can see from the stack trace the intention is to
dispose the non-default caret and not the styled text's default caret. So it
would be good to have the scenario where we dispose the default one.

Note: Please one bug per report.

Comment 2 Dani Megert 2004-03-16 04:04:34 EST

Created bug 54942 for the BIDI problem.

Comment 3 Felipe Heidrich 2004-03-16 13:41:48 EST

Created attachment 8605 [details]
new StyledText implementation

Copy these files to:
org.eclipse.swt\Eclipse SWT Custom Widgets\common\org\eclipse\swt\custom
self-host eclipse, in tha java editor press insert.

Comment 4 Felipe Heidrich 2004-03-16 13:42:39 EST

We might be releasing this code today...

Comment 5 Dani Megert 2004-03-17 04:08:55 EST

Sure, go ahead. We can then grap the SWT plug-in from the N-build and
investigate the bug.

Note: when (at least we in Zurich ;-) talk about a preview then we mean the
whole plug-in.

Comment 6 Felipe Heidrich 2004-03-17 21:03:12 EST

Code released. Still, you will need to open the StyledText, go to line 6462 
and remove the code:

//workaround bug#54922 
if (updateImage) {
	if ((leftCaretBitmap != null && leftCaretBitmap.isDisposed()) ||
		(rightCaretBitmap != null && rightCaretBitmap.isDisposed())) {
		createCaretBitmaps();
	}
}

Comment 7 Dani Megert 2004-03-18 02:30:22 EST

I would assume it is much easier for you to give us a preview than for us
switching to source, isn't it?

Comment 8 Felipe Heidrich 2004-03-18 12:33:52 EST

You want a preview: load org.eclipse.swt from dev.eclipse.org, it does not 
require any other plugin, remove the workaround, selfhost eclipse and test it. 
It will not take you more than 3 minutes, very easy.

I already gave you the stacktrace, source code, I released the changes in the 
CVS... Please, look into this.

Comment 9 Dani Megert 2004-03-20 16:52:32 EST

I did the following as requested in comment 6:
1. checked out org.eclipse.swt from HEAD (20.12.2004, 22h00 MET)
2. commented out the following code in StyledText.java (rev. 1.202)
//if (updateImage) {
//	if ((leftCaretBitmap != null && leftCaretBitmap.isDisposed()) ||
//		(rightCaretBitmap != null && rightCaretBitmap.isDisposed())) {
//			createCaretBitmaps();
//	}
//}


What I see is:
1. no exceptions - neither in .log nor in console
2. works as expected

Comment 10 Felipe Heidrich 2004-04-07 15:21:10 EDT

build I20040407.

Steps
1. New workspace
2. Create Java project
3. Create Java File
4. Open Java File
5. Focus Java Editor
6. Press Insert key

Comment 11 Felipe Heidrich 2004-04-07 15:23:20 EDT

Created attachment 9303 [details]
log file

Although the exception happens inside SWT code it is caused by the application
who reach for StyledText's default caret and disposed its image.

Comment 12 Felipe Heidrich 2004-04-07 15:24:17 EDT

Note, in order to reproduce this bug your machine has to be Bidi Enabled (it 
means, you need to have Arabic or Hebrew input method installed).

Comment 13 Dani Megert 2004-04-08 12:02:10 EDT

I now see the problem:
normally in SWT when someone creates a resource and then sets it has to dispose
it, right. We followed this approach in Text code with Caret as well. Which I
guess was wrong (i.e. caret doesn't need to be treated as resource and disposed).

Let me explain our scenario to give you some background:
st= new StyledText(...);
<... now happens code you can't control - assume someone calls:
c1= new Caret(st, 0);
In our case this means a client pre-configures StyledText with some caret
...>
now we get the StyledText and want to apply our caret(s)
initialCaret= st.getCaret();
c2= new Caret(st, 0);
c3= new Caret(st, 0);
Caching c2 for later use. Now assume the editor closes. st automatically
disposes c3 and I can dispose c2 but since normally ST disposes the caret I also
need to dispose the initial caret if it is not styled text's default. We assumed
that if the default caret is used getCaret returns null - this does not hold in
the BIDI case.

I guess we don't need to track and dispose the caret. We only need to ensure to
dispose the images that we create and the client is responsible for disposing
his image (if he created one). Am I on the right track?

Comment 14 Dani Megert 2004-04-08 13:20:38 EDT

I assume the Caret is not an OS resource, right? 

Another question is how we can set a caret without an image since StyledText
always sets an image if the direction is not default and our caret's image is null.

Comment 15 Felipe Heidrich 2004-04-08 17:48:15 EDT

"I assume the Caret is not an OS resource, right? "
Wrong, it is. Follow the rule: if you created and disposed.

"Another question is how we can set a caret without an image since StyledText
always sets an image if the direction is not default and our caret's image is 
null."

If you set a caret in the StyledText the StyledText should not change its 
image.

I will change the StyledText as follow:
StyledText will always create and dispose its internal Caret, therefore you 
don't need to disposed it even though you set a new caret in the StyledText.
You must dispose the carets you create yourself, don't relie that StyledText 
will dispose it for you.

Comment 16 Dani Megert 2004-04-12 13:15:48 EDT

Sounds good.

Comment 17 Felipe Heidrich 2004-04-12 13:57:23 EDT

I've released StyledText changes. It will be in tomorrow's integration build.

Comment 18 Felipe Heidrich 2004-04-12 14:00:15 EDT

Note: Using this approach one of the caret will be disposed twice: who created 
the caret always disposed it plus the framework in Canvas always assure that 
the current caret always gets disposed. This is not a problem. In SWT, we can 
call dispose on object multiple times.

Comment 19 Dani Megert 2004-04-13 10:50:31 EDT

Fixed the code on our side for I200404130800. There's just one remaining issue:
it would be nice if setCaret(null) would reset the StyledText's caret. In
I20040407 this is not the case due to:

	if (caret != null) {
		setCaretLocation();
	}

Q: how can I reset the StyledText's caret to default (especially in BIDI situation)?

Comment 20 Felipe Heidrich 2004-04-13 11:22:45 EDT

Daniel, I understand that setCaret(null) restores the StyledText default caret 
is convenient for you but I don't think I can do that because there is the 
case where the application actually really wants to set the caret to null 
(maybe to emulated a disable state or something). But maybe this case should 
not be allowed anyway. I'm going to ask Silenio or Steve if I can do that for 
you.

About the problem with BIDI I'm afraid the StyledText does not give you any 
support for that. Internally StyledText handle all bidi code but it does not 
expose any functionality.

Comment 21 Dani Megert 2004-04-13 11:27:10 EDT

I fully understand that it is problematic to setCaret(null). Couldn't StyledText
it offer resetCaret()?

Comment 22 Felipe Heidrich 2004-04-13 11:31:14 EDT

Silenio, StyledText API request see comment#21.
IMO, it is easy enough for the application to remember styled text default 
caret and set back whenever it is need.

Comment 23 Dani Megert 2004-04-13 11:55:29 EDT

>IMO, it is easy enough for the application to remember styled text default 
>caret and set back whenever it is need.
This is only true if styled text doesn't dynamically change the caret e.g. when
I change input locale.

Comment 24 Felipe Heidrich 2004-04-13 12:18:24 EDT

>This is only true if styled text doesn't dynamically change the caret e.g. 
>when I change input locale.
StyledText does not changed the caret when the input locale changes. It 
changes its image only. It is always the same instance of Caret.

Comment 25 Felipe Heidrich 2004-04-13 12:38:09 EDT

Daniel, I just downloaded and tested I200404130800.
The caret is a pinhead (x,2). I believe you relie that StyledText will set the 
size of the caret for you, right ?
I actually changed StyledText to only set location a user-defined caret 
preserving all others attributes (image, size).
I will release some code to always set (override) the height of the caret. 
this is fix the problem.
Other option would be the application to set height of the caret (using the 
API StyledText#getLineHeight() to figure out the height).


Other problem: I didn't see 'Insert Mode', Java editor only has 'Smart Insert' 
and 'Override' now ?

Comment 26 Dani Megert 2004-04-13 12:56:37 EDT

>The caret is a pinhead (x,2). I believe you rely that StyledText will set the 
>size of the caret for you, right ?
Yes, we did so (we set the height to caret.getSize().y). which used to work.

>I will release some code to always set (override) the height of the caret. 
>this is fix the problem.
Good. Though I would treat this as being our bug since we expected that a new
caret has the correct height. We just fixed our code.

>Other problem: I didn't see 'Insert Mode', Java editor only has 'Smart Insert' 
>and 'Override' now ?
Correct :-) We removed the three-state toggle since it confused people. It now
toggles between "Smart Insert" and "Overwrite". To get the 'raw' insert mode you
need to press Ctrl+Shift+Insert. After doing so it toggles between 'raw' Insert
and "Overwrite" mode. Ctrl+Shift+Insert brings back smartness.

Comment 27 Dani Megert 2004-04-13 13:00:02 EDT

re comment 24: I then agree with comment 22.