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

[Alexander Scherbatiy]

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
>>>>
>>>
>>
>