harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ivanov, Alexey A" <alexey.a.iva...@intel.com>
Subject RE: [classlib][swing] compatibility: j.s.text.GapContent.replace() behaviour
Date Fri, 03 Nov 2006 11:33:17 GMT
>-----Original Message-----
>From: Stepan Mishura [mailto:stepan.mishura@gmail.com]
>Sent: Friday, November 03, 2006 1:49 PM
>To: harmony-dev@incubator.apache.org
>Subject: Re: [classlib][swing] compatibility:
j.s.text.GapContent.replace()
>behaviour
>
>On 11/3/06, Ivanov, Alexey A wrote:
>>
>> >-----Original Message-----
>> >From: Stepan Mishura
>> >Sent: Friday, November 03, 2006 9:43 AM
>> >To: harmony-dev@incubator.apache.org
>> >Subject: Re: [classlib][swing] compatibility:
>> j.s.text.GapContent.replace()
>> >behaviour
>> >
>> >On 11/2/06, Ivanov, Alexey A wrote:
>> >>
>> >> Hi all,
>> >>
>> >> I've started fixing HARMONY-1809. To remove throws clause from the
>> >> declaration of replace method, as it was proposed by Oleg in
>> >> HARMONY-1975, I placed removeItems() and insertItems() calls into
>> >> try-catch block. This would work OK for any valid arguments.
>> >>
>> <SNIP>
>> >>
>> >>
>> >> Any objections, comments, opinions?
>> >
>> >
>> >
>> >Hi Alexey,
>>
>>
>> Hi Stepan,
>>
>> >I'm not quite convinced by your evaluation (I'm not an expert in
Swing
>> API
>> >so I may be wrong). My experiments with GapContent showed that
Harmony
>>
>> Let me give a quick introduction what GapContent is then. This class
>> serves as the default storage for all Document implementations within
>> javax.swing.text. It stores text in char[] array... but with a gap in
>> it.
>> Using the default constructor the array will have length 10. And one
>> character will be placed into the buffer; the other 9 characters in
the
>> array will be the gap, and they are not considered to be portion of
the
>> content. This prevents frequent re-allocations of the underlying
storage
>> array where text is inserted or removed. Moving the gap is cheaper
than
>> re-allocating the array.
>>
>> When an insert or remove occurs, the gap is moved so that it starts
at
>> the position of insert or remove.
>
>
>
>Thanks for the explanation.

You're welcome.

>
>>initially created different object then RI, for example, if you create
>> an
>> >object with GapContent() constructor, Harmony will return start==0
and
>> >end==9 while RI will return start==1 and end==10. The next point
that
>>
>> It doesn't really matter where the gap is. This difference shows only
>> that the gap in the text buffer is located at different location.
Using
>> content.shiftGap(0) on RI, you'll get start = 0, end = 9. Accordingly
>> using content.shiftGap(1) on Harmony, you'll get start = 1, end = 10.
>
>
>
>IMO it might be compatibly issue. I can extend GapContent class and it
may
>depend on what getGapStart() and getGapEnd() return.

No, it can't. Because these methods report where the gap is, and no one
stops you from moving it to another location using GapContent protected
methods.

>
>What matters is that content.length() = 1 in both cases, and
>> content.getString(0, content.length()) returns "\n".
>>
>> Actually, setting the gap to be at 0 is more efficient because the
next
>> (or first) insert is likely to happen at position 0 rather than 1.
>> Therefore to insert text at position 0, the gap will be moved to 0 on
RI
>> (which involves array copying and some other operations). This won't
be
>> the case with Harmony because the gap is already at 0.
>
>
>
>If you do:
>GapContent gc = new GapContent();
>gc.insertString(0, "text");
>
>then gc.getGapStart() returns 4 on RI not 0 as you wrote.
>

Of course! Because you added four characters: "text". The gap was moved
to position 0, the characters were copied to the array, and the gap
position was updated. You can check it by getting the array with
getArray() and examining its contents. The first four chars will be
"text" and the last char at index 9 will be '\n'.

To move gap, you should use shiftGap(). You can easily check that
insertString() calls shiftGap() to move the gap in the array.

Everything starting at getGapStart() index in the array up to
getGapEnd() (not inclusive if I remember correctly) is "garbage".

If there's not enough space in the gap (array) to store new text, the
array gets re-allocated which -- in its turn -- enlarges the gap. 

>>confuses me that the spec. says about position as "logical position in
>> the
>> >storage"...and... "This is not the location in the underlying
storage
>> >array". So negative value for position may be considered as valid.
>>
>> It's all about how the text is stored in GapContent. Because there's
a
>> gap modeled "the location in the underlying storage array" differs
from
>> "the logical position in storage". If you look at the spec of any
>> methods which throw BadLocationException, you'll see that position
(it's
>> called 'where') is to be >= 0. Since this method is used to perform
>> operations of insertString() and remove() in RI, negative positions
are
>> invalid. I believe they put no checks here because validation of
>> parameters is performed in those methods which call replace().
>
>
>
>As I understood the spec. it is true for methods that change content
but
>there is no such restriction for methods that work with gap (see
>shiftGapEndUp, shiftGapStartDown, shiftEnd, shiftGap)

Yes, that's true. Because this methods are protected, you *must*
understand what you do. Most likely you'll get
ArrayIndexOutOfBoundsException for an invalid parameter because any of
these methods copy the underlying array contents. I believe this is the
reason why replace doesn't have these checks too.

Any of these methods can be called as the result of insertString() or
remove() which do perform parameter validation. I see no reason to add
parameter checks there too. (There are no "validation" checks in Harmony
in these methods.)

>
>Another point is that replace() in never used in Harmony implementation
>> - it's called from tests only.
>
>
>But it can be used by some app. so it should be compatible.

Yes, it can. And after calling content.replace(-2, 2, null, 0), the gap
start is -2, which makes the following insertString(0, "1") - quite
legal - throw ArrayIndexOutOfBoundsException. Is it better?
(Look at the comments in HARMONY-1809 for details.)


As I proposed we could throw an Error, that is an unchecked exception
which is not required to be listed in throws clause, to make that some
application fail. This will raise the problem to app. developers. And
it'll be easier for Harmony community to resolve such bug, since
there'll be the required information. If we silently ignore
BadLocationException thrown, we'll face unpredictable behaviour of an
app. which is much harder to diagnose that the root cause was in
replace.


That's why I asked community for input rather than *quietly* selected
the right solution on my own.


Regards,
Alexey.


P.S. I don't mind repeating erroneous behaviour exposed by the RI.
However I see no reason for doing it so far.

>
>Thanks,
>Stepan.
>
>>
>>Thanks,
>>> Alexey.
>>>
>>>
>>> P.S. The related JIRA issues:
>>> https://issues.apache.org/jira/browse/HARMONY-1809
>>> https://issues.apache.org/jira/browse/HARMONY-1975
>>>
>>> GapContent Javadoc:
>>>
>http://java.sun.com/j2se/1.5.0/docs/api/javax/swing/text/GapContent.htm
l
>>> Description of GapContent.replace:
>>>
>http://java.sun.com/j2se/1.5.0/docs/api/javax/swing/text/GapContent.htm
l
>>> #replace(int,%20int,%20java.lang.Object,%20int)
>>>
>>>
>>> --
>>> Alexey A. Ivanov
>>> Intel Middleware Product Division
>>>
>>
>>
>>
>>--
>>Stepan Mishura
>>Intel Middleware Products Division
>>
>>------------------------------------------------------
>>Terms of use : http://incubator.apache.org/harmony/mailing.html
>>To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
>>For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>--
>Alexey A. Ivanov
>Intel Middleware Product Division
>
>
>
>
>--
>Stepan Mishura
>Intel Middleware Products Division
>
>------------------------------------------------------
>Terms of use : http://incubator.apache.org/harmony/mailing.html
>To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
>For additional commands, e-mail: harmony-dev-help@incubator.apache.org

--
Alexey A. Ivanov
Intel Middleware Product Division

Mime
View raw message