Hi Prahalad,
Changes in insertChar() are also fine.
I noticed a small correction in indentation for which there is no need for
another webrev.
At line number 674 & 738. We should maintain space between operands and
operator.
if (end - start != fChars.length+1) { -> if (end - start !=
fChars.length + 1) {
&
if (end - start != fChars.length-1) { -> if (end - start !=
fChars.length - 1) {
Thanks,
Jay
-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Monday, January 08, 2018 12:12 PM
To: Sergey Bylokhov; Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR: [JDK-5064835] TextMeasurer/deleteChar
function fails when deleting more than one characters
Hello Sergey & Jay
Thank you for your time in review & inputs.
Please find my views herewith:
. On Performance:
. TextMeasurer's Constructor invokes :
initAll(AttributedCharacterIterator text) method
. initAll(AttributedCharacterIterator ) method further instantiates
. fChars : new char[length]
. fBidi : new Bidi
. fParagraph : new StyledParagraph and so on.
. The concerned method- deleteChar (and insertChar), invoke the same
"initAll" method when number of characters deleted or inserted is more than one.
. So from performance perspective, there shouldn't be a major difference
between creating a new TextMeasurer and using deleteChar to delete more than
few characters.
. When methods invoke initAll, the text and its attributes are
re-initialized and we would lose the text attributes originally set on
TextMeasurer.
. This is the reason why the proposed fix doesn't allow re-initialization
and throws IllegalArgumentException when attempted to delete multiple chars.
. On Modifying insertChar method's behavior
. As Jay suggested, we should fix insertChar method's behavior to align
with deleteChar method.
. I was not sure whether we should make changes to insertChar as a
separate bug /or alongside the current fix.
. Nevertheless, I 've created a new webrev with the changes included for
insertChar as well.
. Impact from changes to insertChar are as follows-
. No new regressions seen with JTreg test cases.
. The change in insertChar introduces 4 JCK test failures making
total count of JCK test failures to 6
. JCK tests that failed are:
api/java_awt/Font/TextMeasurer/CharTest (Test cases: 1, 2,
5)
api/java_awt/Font/LineBreakTextMeasurer/CharTest (Test
cases: 1, 2, 4)
. All the above test cases can be easily fixed with specification updated
for the methods.
Kindly review the updated changes at your convenience.
Link: http://cr.openjdk.java.net/~pnarayanan/5064835/webrev.01/
Thank you once again
Have a good day
Prahalad N.
-----Original Message-----
From: Sergey Bylokhov
Sent: Saturday, January 06, 2018 3:04 PM
To: Jayathirth D V; Prahalad Kumar Narayanan; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-5064835] TextMeasurer/deleteChar
function fails when deleting more than one characters
Just small comment(for a discussion), about a performance of these methods. How
much faster these methods from the case when the TextMeasure is created from
scratch? I mean that if they are much faster and the insertChar() method works
already, then probably we can support this in deleteChar as well?
On 01/01/2018 22:53, Jayathirth D V wrote:
> Hi Prahalad,
>
> Please find my inputs:
>
> Since we are making changes to deleteChar() code and specification I think we
> should make similar changes to insertChar() to make them behave in same way.
> I observed Jane Wang comment in JBS and tested inserting multiple character
> using TextMeasurer.insertChar() and it doesn't through
> ArrayIndexOutOfBoundsException or any other exception, it just initializes
> the text parameters using the newParagraph.
>
> We can make changes corresponding to insertChar() in the same bug or we can
> raise separate bug, but its better if we make changes to insertChar() also in
> the same JBS as they are similar change corresponding to identical behavior
> in same Class.
>
> Regarding JCK test fail, since we are targeting it to JDK 11 I think we have
> time to update the same without any risk. But we should get input from
> Phil/Others regarding the same.
>
> In the test case may be we can remove the System.out.println("Test passed."),
> since we know that if we don't get any exception/errors the jtreg test is
> passed. I see that some test cases have this "Test Passed" output but I feel
> it is redundant.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Friday, December 29, 2017 4:30 PM
> To: 2d-dev
> Subject: [OpenJDK 2D-Dev] [11] RFR: [JDK-5064835]
> TextMeasurer/deleteChar function fails when deleting more than one
> characters
>
> Hello Everyone
>
> Good day to you.
>
> Request your time in reviewing the fix for the bug:
> JDK-5064835 TextMeasurer/deleteChar function fails when deleting more than
> one characters.
>
> Root Cause:
> . The spec clearly mentions that the concerned method is to be used to delete
> a single character.
> . However, the spec does not mention the outcome when the method is
> used to delete multiple characters (as reported in the bug)
>
> Solution Approaches:
> . Since the spec does not mention the outcome when multiple characters are
> deleted, the result is left to the implementation.
> . The solution can be approached in two perspectives-
> 1. Update the spec to explicitly mention the exception that would be
> thrown in such cases or
> 2. Re-initialize the TextMeasurer with the new text as present in the
> argument of the method.
>
> Solution:
> . I inspected feasibility/ risk of both the approaches and I 'm of the
> opinion that approach 1. is better.
> . Reason is that, with the second approach-
> . The re-initialization would reset all text attributes, and state
> variabes that were set on TextMeasurer (and internal StyledParagraph) object.
> . If re-initialization is required, one could create a new
> TextMeasurer using the modified text rather than invoking deleteChar method.
> . Thus in the proposed solution- I 've added a throws clause that explicitly
> mentions that IllegalArgumentException will be thrown when attempted to
> delete multiple characters.
>
> Other Info:
> . The fix was tested with existing jtreg test cases- No regressions were seen.
> . 2 JCK tests have been found to fail. They are-
> . java_awt/Font/TextMeasurer/CharTest (TestCase5)
> . java_awt/Font/LineBreakMeasurer/CharTest (TestCase4) . In both the
> failures, incorrect arguments are passed to deleteChar method- newParagraph
> (with multiple chars deleted) & beginIndex (-1) . While the test case expects
> index out of bounds exception for -ve index, the code now throws
> IllegalArgumentsException.
> . A minor correction to JCK test will fix the issue. I shall raise a JCK bug
> once the fix is approved & submitted.
>
> Kindly review the changes at your convenience & share your feedback:
> http://cr.openjdk.java.net/~pnarayanan/5064835/webrev.00/
>
> Note: I 've not raised a CSR for this bug yet.
> Based on review, I will create the CSR for change to the specification.
>
> Thank you for your time in review &
> Happy New Year
>
> Prahalad N.
>
--
Best regards, Sergey.
