<Swing Dev> [8] Review request for 8024395: Improve fix for line break calculations

[dmitry markov]

You are right. I corrected the fix. The webrev is located at 
http://cr.openjdk.java.net/~dmarkov/8024395/jdk8/webrev.02/

Thanks,
Dmitry
On 10/09/2013 17:04, Alexander Scherbatiy wrote:
> On 9/10/2013 4:40 PM, dmitry markov wrote:
>> Please find answer below.
>
>    One more remark.
>
>   The original forwardUpdate method has final index0 assignment as: 
> index0 = Math.max(index0, 0);
>   The new calculateUpdateIndexes method has final line: 
> firstUpdateIndex = Math.max(firstUpdateIndex - 1, 0);
>   It seems that logic was to get 0 or unchanged  index0 but now it 
> gets 0 or decremented firstUpdateIndex value.
>
>   Thanks,
>   Alexandr.
>
>
>>
>> Thanks,
>> Dmitry
>> On 10/09/2013 13:49, Alexander Scherbatiy wrote:
>>> On 9/10/2013 12:44 PM, dmitry markov wrote:
>>>> Hi Alexandr,
>>>>
>>>> Thanks for comments. I have updated the fix. New webrev is located 
>>>> at http://cr.openjdk.java.net/~dmarkov/8024395/jdk8/webrev.01/
>>>> Please find my answer below.
>>>      Thank you for your updates.
>>>
>>>   - The BoxView class repaints some parts in the forwardUpdate 
>>> method in case  if they are not valid. Is it also applicable for 
>>> FlowView ?
>>
>> Yes, I think, it is applicable for FlowView. It seems, this is out of 
>> scope of the fix, since it overrides forwardUpdate() method for 
>> LogicalView class (not FlowView). LogicalView does not inherit 
>> BoxView. It inherits View through CompositeView.
>>>   - Could you run JCK and regression tests related to JTextComponent 
>>> and javax.swing.text package and check Notepad, Stylepad and 
>>> SwingSet2 demos that there are no regressions?
>>
>> I have just checked Notepad, Stylepad and SwingSet2 demos look OK. 
>> Some regression tests failed but the fail set is the same as before 
>> and after fix.
>>>
>>>   Thanks,
>>>   Alexandr.
>>>
>>>
>>>>
>>>> Thanks,
>>>> Dmitry
>>>> On 09/09/2013 16:59, Alexander Scherbatiy wrote:
>>>>> On 9/6/2013 6:38 PM, dmitry markov wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Could you review the fix:
>>>>>>   bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8024395
>>>>>>   webrev: 
>>>>>> http://cr.openjdk.java.net/~dmarkov/8024395/jdk8/webrev.00/
>>>>>>
>>>>>> The fix reverts the changes added for JDK-8014863 and override 
>>>>>> forwardUpdate() method for LogicalView class located at FlowView. 
>>>>>> In other words View.forwardUpdate() will go back to its original 
>>>>>> behavior and LogicalView.forwardUpdate() will send update event 
>>>>>> to all views followed by the changed place. This event will cause 
>>>>>> view to drop the cache and re-calculate its break points.
>>>>>
>>>>>     - The FlowView class extends BoxView and the BoxView calls 
>>>>> super.forwardUpdate(ec, e, a, f). Should the FlowView class also 
>>>>> have super call in the forwardUpdate method?
>>>> As you can see, the implementations of View.forwardUpdate() and 
>>>> LogicalView.forwardUpdate() are slightly different. I think, it is 
>>>> not necessary to do one thing twice. That is why 
>>>> LogicalView.forwardUpdate() does not have super call.
>>>>>     - It is usually not good to have duplicated codes in some 
>>>>> methods. Is it possible to unite the same parts of code of 
>>>>> forwardUpdatemethod from View and FlowView class to a method with 
>>>>> package access?
>>>> Done, see updated webrev
>>>>>     - It should be possible to use {@inheritDoc} tag for the 
>>>>> method doc.
>>>> Done, see updated webrev
>>>>>
>>>>>     Thanks,
>>>>>     Alexandr.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Dmitry
>>>>>
>>>>
>>>
>>
>