<Swing Dev> [9] Review request for 8048110: Using tables in JTextPane leads to infinite loop in FlowLayout.layoutRow

[Alexander Scherbatiy]

   The fix looks good to me.

   Thanks,
   Alexandr.

On 9/9/2014 1:53 PM, dmitry markov wrote:
> Hi Alexandr,
>
> I have added updateAfterChange() method as you suggested. New version 
> is located at http://cr.openjdk.java.net/~dmarkov/8048110/jdk9/webrev.03/
>
> Thanks,
> Dmitry
> On 08/09/2014 14:44, Alexander Scherbatiy wrote:
>> On 8/27/2014 2:44 PM, dmitry markov wrote:
>>> Hello Alexandr,
>>>
>>> Thank you for the review. I have updated the fix according to your 
>>> comments. Please find new version here - 
>>> http://cr.openjdk.java.net/~dmarkov/8048110/jdk9/webrev.02/
>>> Now FlowView.forwardUpdate() invokes super.forwardUpdate() to update 
>>> the view responsible for the changed element and then sends update 
>>> event to the GlyphViews followed by the changed place. It updates 
>>> the GlyphViews regardless the type of the first view. Also new views 
>>> are not updated twice anymore.
>>
>>    The FlowView checks directly the type of a view in the 
>> forwardUpdate() method. May be it is better to add method 
>> updateAfterChange() with package access to View which does nothing by 
>> default and update elements for  GlyphView?
>>
>>   Thanks,
>>   Alexandr.
>>
>>>
>>> Thanks,
>>> Dmitry
>>> On 26/08/2014 14:02, Alexander Scherbatiy wrote:
>>>>
>>>>   - The fix sends an update event to all views followed by the 
>>>> changed place only if the first updated view is GlyphView.
>>>>     Should the GlyphViews be updated if the first view is not the 
>>>> GlyphView?
>>>>
>>>>   - The javadoc for forwardUpdate() methods says: "If there were 
>>>> changes to the element this view is responsible for, that should be 
>>>> considered when forwarding (i.e. new child views should not get 
>>>> notified)."
>>>>     Does it mean that new views will be updated twice first in the 
>>>> FlowView.forwardUpdate()  method and second during initialization?
>>>>
>>>>   Thanks,
>>>>   Alexandr.
>>>>
>>>> On 8/21/2014 1:03 PM, dmitry markov wrote:
>>>>> Hello,
>>>>>
>>>>> Reminder. Could you review this, please?
>>>>>
>>>>> Thanks,
>>>>> Dmitry
>>>>> On 18/08/2014 12:10, dmitry markov wrote:
>>>>>> Hello Sergey,
>>>>>>
>>>>>> LogicalView.forwardUpdate() sends 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. This method was 
>>>>>> added under JDK-8024395 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8024395> and intended 
>>>>>> for the documents contained only text elements. The current 
>>>>>> implementation does not expect that non-text element, (e.g. a 
>>>>>> table) can be added to the document.
>>>>>> If the table is added to the document with flow layout, the 
>>>>>> invocation of LogicalView.forwardUpdate() will produce some 
>>>>>> negative visual effects (table will be displayed several times). 
>>>>>> In case of replacing the text by the table the situation much 
>>>>>> worse, since the FlowView will try to re-layout the removed text 
>>>>>> and hangs.
>>>>>>
>>>>>> We have to send the update events to all instances of GlyphView 
>>>>>> followed by changed place for proper text line breaks 
>>>>>> calculation, see JDK-8024395 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8024395>, JDK-8014863 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8014863>. That's why I 
>>>>>> added checks for GlyphView instance. I understand this might be 
>>>>>> not a good place, but only here we have information about all 
>>>>>> views in the document and can forward update to them, if any.
>>>>>> Please note another approach to solve the problems with line 
>>>>>> breaks calculation - re-calculate break spots any time when a 
>>>>>> view is laid out. As far as I know it is used in jdk6, but this 
>>>>>> solution is not suitable, since it causes serious performance 
>>>>>> problems in case of large documents.
>>>>>>
>>>>>> Also I have updated the fix. New version - 
>>>>>> http://cr.openjdk.java.net/~dmarkov/8048110/jdk9/webrev.01/
>>>>>> Now LogicalView.forwardUpdate() sends update only to GlyphView 
>>>>>> followed by the changed place. This resolves the problem when the 
>>>>>> table is located in the middle of the text line.
>>>>>>
>>>>>> Thanks,
>>>>>> Dmitry
>>>>>>
>>>>>> On 15/08/2014 12:56, Sergey Bylokhov wrote:
>>>>>>> Hello, Dmitry.
>>>>>>>>> The LogicalView.forwardUpdate() should send an update event to 
>>>>>>>>> the views
>>>>>>>>> followed by the changed place, only when an instance of 
>>>>>>>>> GlyphView has
>>>>>>>>> changed; otherwise it should invoke View.forwardUpdate() to 
>>>>>>>>> handle the
>>>>>>>>> update properly.
>>>>>>> Can you share additional information about the fix. Why 
>>>>>>> LogicalView should send an update if GlyphView was changed only. 
>>>>>>> Because before the fix LocalView knows nothing about GlyphView.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Dmitry
>>>>>>
>>>>>
>>>>
>>>
>>
>